From bb9ce9cd2abeef8d72e95b19b608fa573843632c Mon Sep 17 00:00:00 2001 From: pq Date: Thu, 1 Feb 2024 21:28:40 +0000 Subject: [PATCH] ensure a defined options file overrides discovered ones (Note an additional pre-existing test in `context_locatator_test:test_locateRoots_nested_options_overriddenOptions`.) Fixes https://github.com/dart-lang/sdk/issues/54791 Change-Id: Ie67e9d840bec1caaa3548a8803f3a9b452807249 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/349625 Reviewed-by: Brian Wilkerson Commit-Queue: Phil Quitslund Reviewed-by: Konstantin Shcheglov --- .../analysis/analysis_context_collection.dart | 1 + .../src/dart/analysis/context_builder.dart | 26 ++- .../analysis_context_collection_test.dart | 165 +++++++++++++++++- pkg/analyzer_cli/test/driver_test.dart | 27 +++ 4 files changed, 209 insertions(+), 10 deletions(-) diff --git a/pkg/analyzer/lib/src/dart/analysis/analysis_context_collection.dart b/pkg/analyzer/lib/src/dart/analysis/analysis_context_collection.dart index b5a9885702d..1650b1771f0 100644 --- a/pkg/analyzer/lib/src/dart/analysis/analysis_context_collection.dart +++ b/pkg/analyzer/lib/src/dart/analysis/analysis_context_collection.dart @@ -104,6 +104,7 @@ class AnalysisContextCollectionImpl implements AnalysisContextCollection { var context = contextBuilder.createContext( byteStore: byteStore, contextRoot: root, + definedOptionsFile: optionsFile != null, declaredVariables: DeclaredVariables.fromMap(declaredVariables ?? {}), drainStreams: drainStreams, enableIndex: enableIndex, diff --git a/pkg/analyzer/lib/src/dart/analysis/context_builder.dart b/pkg/analyzer/lib/src/dart/analysis/context_builder.dart index 1333b7c3a10..826838fdbcd 100644 --- a/pkg/analyzer/lib/src/dart/analysis/context_builder.dart +++ b/pkg/analyzer/lib/src/dart/analysis/context_builder.dart @@ -60,6 +60,7 @@ class ContextBuilderImpl implements ContextBuilder { DriverBasedAnalysisContext createContext({ ByteStore? byteStore, required ContextRoot contextRoot, + bool definedOptionsFile = false, DeclaredVariables? declaredVariables, bool drainStreams = true, bool enableIndex = false, @@ -115,15 +116,24 @@ class ContextBuilderImpl implements ContextBuilder { summaryData?.addBundle(null, sdk.bundle); } + var optionsFile = contextRoot.optionsFile; var sourceFactory = workspace.createSourceFactory(sdk, summaryData); - var analysisOptionsMap = ContextLocatorImpl.singleOptionContexts - ? AnalysisOptionsMap.forSharedOptions(_getAnalysisOptions( - contextRoot, sourceFactory, sdk, updateAnalysisOptions2)) - // TODO(pq): verify that options maps handle contextRoot.optionsFile overriding - // https://github.com/dart-lang/sdk/issues/54791 - : _createOptionsMap( - contextRoot, sourceFactory, updateAnalysisOptions2, sdk); + var analysisOptionsMap = + // If there's an options file defined (as, e.g. passed into the + // AnalysisContextCollection), use a shared options map based on it. + ((definedOptionsFile && optionsFile != null) || + ContextLocatorImpl.singleOptionContexts) + ? AnalysisOptionsMap.forSharedOptions(_getAnalysisOptions( + contextRoot, + optionsFile, + sourceFactory, + sdk, + updateAnalysisOptions2)) + // Else, create one from the options file mappings stored in the + // context root. + : _createOptionsMap( + contextRoot, sourceFactory, updateAnalysisOptions2, sdk); final analysisContext = DriverBasedAnalysisContext(resourceProvider, contextRoot); @@ -282,6 +292,7 @@ class ContextBuilderImpl implements ContextBuilder { // TODO(scheglov): We have already loaded it once in [ContextLocatorImpl]. AnalysisOptionsImpl _getAnalysisOptions( ContextRoot contextRoot, + File? optionsFile, SourceFactory sourceFactory, DartSdk sdk, void Function( @@ -290,7 +301,6 @@ class ContextBuilderImpl implements ContextBuilder { required DartSdk sdk})? updateAnalysisOptions, ) { - var optionsFile = contextRoot.optionsFile; var options = AnalysisOptionsImpl(file: optionsFile); if (optionsFile != null) { diff --git a/pkg/analyzer/test/src/dart/analysis/analysis_context_collection_test.dart b/pkg/analyzer/test/src/dart/analysis/analysis_context_collection_test.dart index 7fd3d7b8259..f434ee8ab49 100644 --- a/pkg/analyzer/test/src/dart/analysis/analysis_context_collection_test.dart +++ b/pkg/analyzer/test/src/dart/analysis/analysis_context_collection_test.dart @@ -342,6 +342,110 @@ workspaces '''); } + test_pubWorkspace_multipleAnalysisOptions_overridingOptions() async { + final workspaceRootPath = '/home'; + final testPackageRootPath = '$workspaceRootPath/test'; + final testPackageLibPath = '$testPackageRootPath/lib'; + + newPubspecYamlFile(testPackageRootPath, r''' +name: test +'''); + + newSinglePackageConfigJsonFile( + packagePath: testPackageRootPath, + name: 'test', + ); + + var rootOptionsFile = newAnalysisOptionsYamlFile(testPackageRootPath, ''); + newFile('$testPackageLibPath/a.dart', ''); + + final nestedPath = '$testPackageLibPath/nested'; + newAnalysisOptionsYamlFile(nestedPath, ''); + newFile('$nestedPath/b.dart', ''); + + // Verify that despite the nested options file + // (/home/test/nested/analysis_options.yaml), the nested file gets analyzed + // with the outer one (/home/test/analysis_options.yaml) as passed into + // the AnalysisContextCollection. + _assertWorkspaceCollectionText( + workspaceRootPath, optionsFile: rootOptionsFile, r''' +contexts + /home/test + packagesFile: /home/test/.dart_tool/package_config.json + workspace: workspace_0 + analyzedFiles + /home/test/lib/a.dart + uri: package:test/a.dart + analysisOptions_0 + workspacePackage_0_0 + /home/test/lib/nested/b.dart + uri: package:test/nested/b.dart + analysisOptions_0 + workspacePackage_0_0 +analysisOptions + analysisOptions_0: /home/test/analysis_options.yaml +workspaces + workspace_0: PubWorkspace + root: /home/test + pubPackages + workspacePackage_0_0: PubWorkspacePackage + root: /home/test +'''); + } + + test_pubWorkspace_multipleAnalysisOptions_overridingOptions_outsideWorspaceRoot() async { + final workspaceRootPath = '/home'; + final testPackageRootPath = '$workspaceRootPath/test'; + final testPackageLibPath = '$testPackageRootPath/lib'; + + newPubspecYamlFile(testPackageRootPath, r''' +name: test +'''); + + newSinglePackageConfigJsonFile( + packagePath: testPackageRootPath, + name: 'test', + ); + + var definedOptionsFile = newAnalysisOptionsYamlFile('/outside', ''); + + newFile('$testPackageLibPath/a.dart', ''); + + final nestedPath = '$testPackageLibPath/nested'; + newAnalysisOptionsYamlFile(nestedPath, ''); + newFile('$nestedPath/b.dart', ''); + + // Verify that despite the nested options file + // (/home/test/nested/analysis_options.yaml), the nested file gets analyzed + // with the defined one which is outside the workspace + // (/outside/analysis_options.yaml) as passed into the + // AnalysisContextCollection. + _assertWorkspaceCollectionText( + workspaceRootPath, optionsFile: definedOptionsFile, r''' +contexts + /home/test + packagesFile: /home/test/.dart_tool/package_config.json + workspace: workspace_0 + analyzedFiles + /home/test/lib/a.dart + uri: package:test/a.dart + analysisOptions_0 + workspacePackage_0_0 + /home/test/lib/nested/b.dart + uri: package:test/nested/b.dart + analysisOptions_0 + workspacePackage_0_0 +analysisOptions + analysisOptions_0: /outside/analysis_options.yaml +workspaces + workspace_0: PubWorkspace + root: /home/test + pubPackages + workspacePackage_0_0: PubWorkspacePackage + root: /home/test +'''); + } + test_pubWorkspace_multiplePackageConfigs() async { final workspaceRootPath = '/home'; final testPackageRootPath = '$workspaceRootPath/test'; @@ -551,14 +655,19 @@ workspaces /// workspace path, without any excludes. void _assertWorkspaceCollectionText( String workspaceRootPath, - String expected, - ) { + String expected, { + File? optionsFile, + }) { + if (optionsFile != null) { + expect(optionsFile.exists, isTrue); + } final collection = AnalysisContextCollectionImpl( resourceProvider: resourceProvider, sdkPath: sdkRoot.path, includedPaths: [ getFolder(workspaceRootPath).path, ], + optionsFile: optionsFile?.path, ); _assertCollectionText(collection, expected); @@ -641,6 +750,58 @@ workspaces '''); } + @override + test_pubWorkspace_multipleAnalysisOptions_overridingOptions() async { + final workspaceRootPath = '/home'; + final testPackageRootPath = '$workspaceRootPath/test'; + final testPackageLibPath = '$testPackageRootPath/lib'; + + newPubspecYamlFile(testPackageRootPath, r''' +name: test +'''); + + newSinglePackageConfigJsonFile( + packagePath: testPackageRootPath, + name: 'test', + ); + + var rootOptionsFile = newAnalysisOptionsYamlFile(testPackageRootPath, ''); + newFile('$testPackageLibPath/a.dart', ''); + + final nestedPath = '$testPackageLibPath/nested'; + newAnalysisOptionsYamlFile(nestedPath, ''); + newFile('$nestedPath/b.dart', ''); + + // Verify that despite the nested options file + // (/home/test/nested/analysis_options.yaml), the nested file gets analyzed + // with the outer one (/home/test/analysis_options.yaml) as passed into + // the AnalysisContextCollection. + _assertWorkspaceCollectionText( + workspaceRootPath, optionsFile: rootOptionsFile, r''' +contexts + /home/test + packagesFile: /home/test/.dart_tool/package_config.json + workspace: workspace_0 + analyzedFiles + /home/test/lib/a.dart + uri: package:test/a.dart + analysisOptions_0 + workspacePackage_0_0 + /home/test/lib/nested/b.dart + uri: package:test/nested/b.dart + analysisOptions_0 + workspacePackage_0_0 +analysisOptions + analysisOptions_0: /home/test/analysis_options.yaml +workspaces + workspace_0: PubWorkspace + root: /home/test + pubPackages + workspacePackage_0_0: PubWorkspacePackage + root: /home/test +'''); + } + @override test_pubWorkspace_singleAnalysisOptions_multipleContexts() async { final workspaceRootPath = '/home'; diff --git a/pkg/analyzer_cli/test/driver_test.dart b/pkg/analyzer_cli/test/driver_test.dart index 19527b1ef99..8766d8192c4 100644 --- a/pkg/analyzer_cli/test/driver_test.dart +++ b/pkg/analyzer_cli/test/driver_test.dart @@ -8,6 +8,7 @@ import 'package:analyzer/error/error.dart'; import 'package:analyzer/source/error_processor.dart'; import 'package:analyzer/source/source.dart'; import 'package:analyzer/src/analysis_options/analysis_options_provider.dart'; +import 'package:analyzer/src/dart/analysis/context_locator.dart'; import 'package:analyzer/src/error/codes.dart'; import 'package:analyzer/src/generated/engine.dart'; import 'package:analyzer/src/util/file_paths.dart' as file_paths; @@ -28,6 +29,7 @@ void main() { defineReflectiveTests(LinterTest); defineReflectiveTests(NonDartFilesTest); defineReflectiveTests(OptionsTest); + defineReflectiveTests(OptionsTest_SingleOptionsPerContext); }, name: 'Driver'); } @@ -338,13 +340,30 @@ flutter: @reflectiveTest class OptionsTest extends BaseTest { + /// Cached state to restore on test tearDown. + final _singleOptionsContextsDefault = ContextLocatorImpl.singleOptionContexts; + String get analysisOptionsYaml => file_paths.analysisOptionsYaml; + bool get enableSingleOptionContexts => false; + List get processors => analysisOptions.errorProcessors; ErrorProcessor processorFor(AnalysisError error) => processors.firstWhere((p) => p.appliesTo(error)); + @override + void setUp() { + super.setUp(); + ContextLocatorImpl.singleOptionContexts = enableSingleOptionContexts; + } + + @override + void tearDown() { + ContextLocatorImpl.singleOptionContexts = _singleOptionsContextsDefault; + super.tearDown(); + } + /// If a file is specified explicitly, it should be analyzed, even if /// it is excluded. Excludes work when an including directory is specified. Future test_analysisOptions_excluded_requested() async { @@ -467,6 +486,14 @@ class OptionsTest extends BaseTest { } } +/// To be removed when `singleOptionContexts` defaults to false. +@reflectiveTest +// ignore: camel_case_types +class OptionsTest_SingleOptionsPerContext extends OptionsTest { + @override + bool get enableSingleOptionContexts => true; +} + class TestSource implements Source { TestSource();