[beta] [flutter_tools] Resolve includes recursively in AnalysisOptionsMigration - #191531
Conversation
…ion (flutter#191082) ## Description `AnalysisOptionsMigration` was ignoring `include:` directives in `analysis_options.yaml` files, causing it to repeatedly rewrite files that already inherited the required platform and build directory exclusions (such as `build/**`, `android/**`, etc.) from included files (either relative paths or `package:` URIs). This PR updates `AnalysisOptionsMigration` to: - Recursively resolve and collect analyzer `exclude:` patterns from `include:` directives. - Support both relative paths and `package:` URIs for includes (using `PackageConfig`). - Support multiple includes (when `include` is specified as a YAML list). - Guard against infinite recursion on cyclic includes using canonical normalized paths. - Safely extract exclude patterns to prevent crashes on malformed files. - Pass `PackageConfig` from `ensureReadyForPlatformSpecificTooling` to `AnalysisOptionsMigration`. ## Related Issues Fixes flutter#191056 ## Tests - Added unit tests in `analysis_options_migration_test.dart`: - `skipped if exclusions are inherited via relative include` - `skipped if exclusions are inherited via package include` - `skipped if exclusions are inherited via multiple includes` - `handles cyclic includes without crashing` - `handles invalid exclude types without crashing`
|
@AlexV525 please fill out the PR description above, afterwards the release team will review this request. |
github-actions
Bot
added
the
tool
|
This pull request was opened from and to a release candidate branch. This should only be done as part of the official Flutter release process. If you are attempting to make a regular contribution to the Flutter project, please close this PR and follow the instructions at Tree Hygiene for detailed instructions on contributing to Flutter. Reviewers: Use caution before merging pull requests to release branches. Ensure the proper procedure has been followed. |
![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 AnalysisOptionsMigration to recursively collect analyzer exclude: patterns from analysis_options.yaml and any included configuration files, resolving both relative paths and package: URIs while preventing infinite recursion on cyclic includes. Feedback suggests caching the Future<PackageConfig?> instead of the resolved PackageConfig? to avoid redundant loading attempts when the configuration is missing or invalid. Additionally, a test case for cyclic includes with different casing needs to be corrected because MemoryFileSystem is case-sensitive, meaning the cycle is currently not being traversed.
| final PackageConfig? _packageConfig; | ||
| PackageConfig? _loadedPackageConfig; |
There was a problem hiding this comment.
To avoid redundant loading attempts when the package config is missing or invalid (which returns null), we should cache the Future<PackageConfig?> instead of the resolved PackageConfig?. This prevents repeatedly trying to find and load the package config file on every package: include when it fails to load once.
| final PackageConfig? _packageConfig; | |
| PackageConfig? _loadedPackageConfig; | |
| final PackageConfig? _packageConfig; | |
| Future<PackageConfig?>? _packageConfigFuture; |
| Future<PackageConfig?> _getPackageConfig() async { | ||
| if (_packageConfig != null) { | ||
| return _packageConfig; | ||
| } | ||
| if (_loadedPackageConfig != null) { | ||
| return _loadedPackageConfig; | ||
| } | ||
| final File? configFile = findPackageConfigFile(_project.directory); | ||
| if (configFile == null) { | ||
| return null; | ||
| } | ||
| try { | ||
| _loadedPackageConfig = await loadPackageConfigWithLogging( | ||
| configFile, | ||
| logger: logger, | ||
| throwOnError: false, | ||
| ); | ||
| return _loadedPackageConfig; | ||
| } on Exception { | ||
| // Ignore errors loading package config during migration checks. | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Update _getPackageConfig to use the cached Future<PackageConfig?> to prevent redundant file system checks and parsing if loading fails or returns null.
Future<PackageConfig?> _getPackageConfig() {
return _packageConfigFuture ??= _loadPackageConfig();
}
Future<PackageConfig?> _loadPackageConfig() async {
if (_packageConfig != null) {
return _packageConfig;
}
final File? configFile = findPackageConfigFile(_project.directory);
if (configFile == null) {
return null;
}
try {
return await loadPackageConfigWithLogging(
configFile,
logger: logger,
throwOnError: false,
);
} on Exception {
// Ignore errors loading package config during migration checks.
return null;
}
}| context.analysisOptionsFile.writeAsStringSync(analysisOptionsContents); | ||
| context.memoryFileSystem.file('shared_options.yaml').writeAsStringSync(sharedOptionsContents); |
There was a problem hiding this comment.
In MemoryFileSystem (which is case-sensitive), SHARED_OPTIONS.YAML does not exist by default, so file.existsSync() returns false and the cycle is never actually traversed. To properly test cycle detection under different casing, we should write both files to the memory file system so that the cycle is actually traversed.
| context.analysisOptionsFile.writeAsStringSync(analysisOptionsContents); | |
| context.memoryFileSystem.file('shared_options.yaml').writeAsStringSync(sharedOptionsContents); | |
| context.analysisOptionsFile.writeAsStringSync(analysisOptionsContents); | |
| context.memoryFileSystem.file('shared_options.yaml').writeAsStringSync(sharedOptionsContents); | |
| context.memoryFileSystem.file('SHARED_OPTIONS.YAML').writeAsStringSync('include: shared_options.yaml'); |
References
- Verify test validity: Confirm that new or modified tests effectively catch the issue being fixed and would fail if the fix were reverted. (link)
AlexV525
changed the title
2 participants
This pull request is created by automatic cherry pick workflow
Please fill in the form below, and a flutter domain expert will evaluate this cherry pick request.
Issue Link:
#191056
Impact Description:
AnalysisOptionsMigrationignoredinclude:directives inanalysis_options.yamlfiles, causingflutter pub getand build commands to repeatedly rewriteanalysis_options.yamlfiles that already inherited the required analyzerexclude:patterns (such asbuild/**,android/**,ios/**) via relative orpackage:includes.Changelog Description:
[flutter/191056] Recursively resolve includes in AnalysisOptionsMigration to prevent redundant rewrites of analysis_options.yaml.
Workaround:
Explicitly duplicate exclude patterns in the top-level
analysis_options.yamlfile instead of relying on included configs.Risk:
What is the risk level of this cherry-pick?
Test Coverage:
Are you confident that your fix is well-tested by automated tests?
Validation Steps:
analysis_options.yamlincludes another options file defining platform exclusions.flutter pub get.analysis_options.yamlis not unnecessarily modified.packages/flutter_tools/test/general.shard/analysis_options_migration_test.dart.