[pigeon] Add support for multiple part files#11664
[pigeon] Add support for multiple part files#11664rohitsangwan01 wants to merge 5 commits intoflutter:mainfrom
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Pigeon library to support Dart 'part' files by identifying part paths and merging their content for processing. Feedback indicates that the current concatenation approach causes incorrect line number reporting in RootBuilder. Additionally, it is recommended to handle file-reading errors more gracefully to prevent CLI crashes and to refactor the parsing logic to eliminate redundant AST generation across multiple helper methods.
|
Thanks for the contribution! In the future, please do not delete the checklist that is in the PR template; it is there for a reason. This PR is missing required elements described in the checklist, which need to be addressed before it moves forward with review. I am marking the PR as a Draft. Please re-add and complete the checklist, updating the PR as appropriate, and when that’s complete please feel free to mark the PR as ready for review. |
There was a problem hiding this comment.
Code Review
This pull request enables Pigeon to support Dart part files by merging the contents of the main file and its parts into a single string for parsing. It introduces helper methods for resolving part paths and stripping directives, and adds tests for multi-file configurations. Feedback indicates that the concatenation approach causes a regression in error reporting line numbers, fails to support nested part files, and should include LibraryDirective handling to prevent generating invalid Dart code.
| final rootBuilder = RootBuilder(rootInputString); | ||
| final dart_ast.CompilationUnit mergedUnit = parseString( | ||
| content: rootInputString, | ||
| path: normalizedInputPath, | ||
| throwIfDiagnostics: false, | ||
| ).unit; | ||
| mergedUnit.accept(rootBuilder); |
There was a problem hiding this comment.
Merging the contents of the main file and its parts into a single rootInputString causes a regression in error reporting. Any errors identified by RootBuilder (e.g., invalid types, unsupported constructs) will report line numbers relative to this concatenated string and attribute them to the main file path, making it difficult for users to locate the actual issue in their source files. Consider a strategy that preserves the mapping to original files or avoids string concatenation by visiting multiple CompilationUnits while providing the correct source context for each.
| List<String> _getPartPaths( | ||
| Iterable<dart_ast.Directive> directives, { | ||
| required String sourcePath, | ||
| }) { | ||
| final parts = <String>[]; | ||
| for (final directive in directives) { | ||
| if (directive is dart_ast.PartDirective) { | ||
| final String? uri = directive.uri.stringValue; | ||
| if (uri != null) { | ||
| parts.add( | ||
| path.absolute( | ||
| path.normalize(path.join(path.dirname(sourcePath), uri)), | ||
| ), | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| return parts; | ||
| } |
There was a problem hiding this comment.
The current implementation of _getPartPaths only retrieves part files referenced directly in the main Pigeon file. It does not support nested part files (i.e., a part file that itself contains part directives), which is valid Dart. To fully support the part mechanism, this should recursively resolve part paths.
| } else if (directive is dart_ast.PartDirective || | ||
| directive is dart_ast.PartOfDirective) { | ||
| removalRanges.add((start: directive.offset, end: directive.end)); | ||
| } |
There was a problem hiding this comment.
The _stripPartsAndCollectImports function does not handle LibraryDirectives. If the main Pigeon file contains a library directive, it will remain in the body while imports are moved to the top in _getInputString. This results in invalid Dart code (as library must precede import), which may cause the subsequent parseString call or RootBuilder to behave unexpectedly. Consider stripping LibraryDirective as well.
} else if (directive is dart_ast.PartDirective ||
directive is dart_ast.PartOfDirective ||
directive is dart_ast.LibraryDirective) {
removalRanges.add((start: directive.offset, end: directive.end));
}
|
@stuartmorgan-g Thank you for the feedback. I’ve re-added the checklist and completed the required items in the PR description. The PR is now updated and ready for review again. |
Could you point me to where you've done this step? I'm not seeing either of the two options in the PR. |
Ahh, I marked it by mistake. I thought CHANGELOG and Version update would be the last step, I’ve now updated the CHANGELOG and Version |
2 participants
Adds support for defining Pigeon APIs across files using Dart part / part of directives. Pigeon now parses the main input file together with its part files so split definitions are handled correctly.
Partially addresses flutter/flutter#66550.
What changed
Pre-Review Checklist
[shared_preferences]///).