From 292a62348cb1f69b062f3846a8c3587fbb5e36dc Mon Sep 17 00:00:00 2001 From: Jacob MacDonald Date: Wed, 20 Sep 2023 09:41:24 -0700 Subject: [PATCH] Fix an issue where multiple AnalyzerResolvers instances lead to incorrect behavior (#3580) Deprecate the unnamed AnalyzerResolvers constructor, replace it with a static instance getter and `AnalyzerResolvers.custom` factory constructor. These new APIs guarantee a 1:1 relationship between the BuildAssetUriResolver and an analysis engine, which is necessary to ensure `changeFile` is always called on each analysis engine when needed. Note that the deprecated constructor does not uphold this invariant, it remains backwards compatible with its old implementation, and uses the same BuildAssetUriResolver as the shared resolver. --- build/CHANGELOG.md | 2 + build/pubspec.yaml | 4 +- build/test/builder/build_step_impl_test.dart | 20 ++++--- build_resolvers/CHANGELOG.md | 8 +++ .../lib/src/build_asset_uri_resolver.dart | 14 +++-- build_resolvers/lib/src/resolver.dart | 60 +++++++++++++++---- build_resolvers/pubspec.yaml | 2 +- build_runner_core/CHANGELOG.md | 4 +- .../lib/src/generate/options.dart | 2 +- build_runner_core/pubspec.yaml | 4 +- build_test/CHANGELOG.md | 2 + build_test/lib/src/resolve_source.dart | 6 +- build_test/lib/src/test_builder.dart | 5 +- build_test/pubspec.yaml | 2 +- 14 files changed, 97 insertions(+), 38 deletions(-) diff --git a/build/CHANGELOG.md b/build/CHANGELOG.md index 9f9b3e207..fc78c2d1b 100644 --- a/build/CHANGELOG.md +++ b/build/CHANGELOG.md @@ -1,3 +1,5 @@ +## 2.4.2-wip + ## 2.4.1 - Allow the latest analyzer (6.x.x). diff --git a/build/pubspec.yaml b/build/pubspec.yaml index c60894d36..8879e7c9a 100644 --- a/build/pubspec.yaml +++ b/build/pubspec.yaml @@ -1,5 +1,5 @@ name: build -version: 2.4.1 +version: 2.4.2-wip description: A package for authoring build_runner compatible code generators. repository: https://github.com/dart-lang/build/tree/master/build @@ -18,7 +18,7 @@ dependencies: path: ^1.8.0 dev_dependencies: - build_resolvers: ^2.0.0 + build_resolvers: ^2.4.0 build_test: ^2.0.0 dart_flutter_team_lints: ^1.0.0 test: ^1.16.0 diff --git a/build/test/builder/build_step_impl_test.dart b/build/test/builder/build_step_impl_test.dart index 760f17e95..fcdadac9e 100644 --- a/build/test/builder/build_step_impl_test.dart +++ b/build/test/builder/build_step_impl_test.dart @@ -35,7 +35,7 @@ void main() { primary = makeAssetId(); outputs = List.generate(5, (index) => makeAssetId()); buildStep = BuildStepImpl(primary, outputs, reader, writer, - AnalyzerResolvers(), resourceManager, _unsupported); + AnalyzerResolvers.custom(), resourceManager, _unsupported); }); test('doesnt allow non-expected outputs', () { @@ -93,7 +93,7 @@ void main() { [outputId], reader, writer, - AnalyzerResolvers(), + AnalyzerResolvers.custom(), resourceManager, _unsupported, ); @@ -121,7 +121,7 @@ void main() { var primary = makeAssetId('a|web/a.dart'); var buildStep = BuildStepImpl(primary, [], reader, writer, - AnalyzerResolvers(), resourceManager, _unsupported); + AnalyzerResolvers.custom(), resourceManager, _unsupported); var resolver = buildStep.resolver; var aLib = await resolver.libraryFor(primary); @@ -150,8 +150,14 @@ void main() { assetWriter = SlowAssetWriter(); outputId = makeAssetId('a|test.txt'); outputContent = '$outputId'; - buildStep = BuildStepImpl(primary, [outputId], StubAssetReader(), - assetWriter, AnalyzerResolvers(), resourceManager, _unsupported); + buildStep = BuildStepImpl( + primary, + [outputId], + StubAssetReader(), + assetWriter, + AnalyzerResolvers.custom(), + resourceManager, + _unsupported); }); test('Completes only after writes finish', () async { @@ -199,7 +205,7 @@ void main() { primary = makeAssetId(); output = makeAssetId(); buildStep = BuildStepImpl(primary, [output], reader, writer, - AnalyzerResolvers(), resourceManager, _unsupported, + AnalyzerResolvers.custom(), resourceManager, _unsupported, stageTracker: NoOpStageTracker.instance); }); @@ -214,7 +220,7 @@ void main() { var writer = StubAssetWriter(); var unused = {}; var buildStep = BuildStepImpl(makeAssetId(), [], reader, writer, - AnalyzerResolvers(), resourceManager, _unsupported, + AnalyzerResolvers.custom(), resourceManager, _unsupported, reportUnusedAssets: unused.addAll); var reported = [ makeAssetId(), diff --git a/build_resolvers/CHANGELOG.md b/build_resolvers/CHANGELOG.md index 8e02102b1..5dce1de75 100644 --- a/build_resolvers/CHANGELOG.md +++ b/build_resolvers/CHANGELOG.md @@ -1,3 +1,11 @@ +## 2.4.0-wip + +- Deprecate the unnamed `AnalyzerResolvers` constructor, an replace it with the + `AnalyzerResolvers.sharedInstance` getter and `AnalyzerResolvers.custom` + constructor. These new apis enforce a 1:1 relationship between the + `BuildAssetUriResolver` instances and `AnalyzerResolvers` instances, which is + necessary to ensure correct builds in the presence of multiple resolvers. + ## 2.3.2 - Skip file delete for SDK summary and deps file. This will only impact behavior diff --git a/build_resolvers/lib/src/build_asset_uri_resolver.dart b/build_resolvers/lib/src/build_asset_uri_resolver.dart index c25a71aba..4f6e8e402 100644 --- a/build_resolvers/lib/src/build_asset_uri_resolver.dart +++ b/build_resolvers/lib/src/build_asset_uri_resolver.dart @@ -54,10 +54,16 @@ class BuildAssetUriResolver extends UriResolver { /// input, subsequent calls to a resolver, or a transitive import thereof. final _buildStepTransitivelyResolvedAssets = >{}; - /// Use the [instance] getter to get an instance. - BuildAssetUriResolver._(); + /// Use the [sharedInstance] getter to get the shared instance, which is what + /// real builds should do. There should always be a 1:1 relationship between + /// [BuildAssetUriResolver]s and `AnalyzerResolvers`. + BuildAssetUriResolver(); - static final BuildAssetUriResolver instance = BuildAssetUriResolver._(); + /// The instance used by the shared `AnalyzerResolvers` instance, which is + /// what should be used by normal builds. + /// + /// This is not used within testing contexts or similar custom contexts. + static final BuildAssetUriResolver sharedInstance = BuildAssetUriResolver(); /// Updates [resourceProvider] and the analysis driver given by /// `withDriverResource` with updated versions of [entryPoints]. @@ -261,7 +267,7 @@ Set _parseDependencies(String content, AssetId from) => HashSet.of( /// directives, and cache the results if they weren't already cached. Future?> dependenciesOf( AssetId id, BuildStep buildStep) async => - (await BuildAssetUriResolver.instance + (await BuildAssetUriResolver.sharedInstance ._updateCachedAssetState(id, buildStep)) ?.dependencies; diff --git a/build_resolvers/lib/src/resolver.dart b/build_resolvers/lib/src/resolver.dart index 0fc532946..e24dcc3f1 100644 --- a/build_resolvers/lib/src/resolver.dart +++ b/build_resolvers/lib/src/resolver.dart @@ -381,7 +381,8 @@ class AnalyzerResolvers implements Resolvers { // Lazy, all access must be preceded by a call to `_ensureInitialized`. late final AnalyzerResolver _resolver; - BuildAssetUriResolver? _uriResolver; + + final BuildAssetUriResolver _uriResolver; /// Nullable, should not be accessed outside of [_ensureInitialized]. Future>? _initialized; @@ -402,30 +403,65 @@ class AnalyzerResolvers implements Resolvers { /// **NOTE**: The [_packageConfig] is not used for path resolution, it is /// primarily used to get the language versions. Any other data (including /// extra data), may be passed to the analyzer on an as needed basis. - AnalyzerResolvers( - [AnalysisOptions? analysisOptions, - Future Function()? sdkSummaryGenerator, - this._packageConfig]) - : _analysisOptions = analysisOptions ?? + factory AnalyzerResolvers.custom({ + AnalysisOptions? analysisOptions, + Future Function()? sdkSummaryGenerator, + PackageConfig? packageConfig, + }) => + AnalyzerResolvers._( + analysisOptions: analysisOptions, + sdkSummaryGenerator: sdkSummaryGenerator, + packageConfig: packageConfig, + // Custom resolvers get their own asset uri resolver, as there should + // always be a 1:1 relationship between them. + uriResolver: BuildAssetUriResolver()); + + /// See [AnalyzerResolvers.custom] for docs. + @Deprecated('Use either the AnalyzerResolvers.custom constructor or the ' + 'AnalyzerResolvers.sharedInstance static getter to get an instance.') + factory AnalyzerResolvers([ + AnalysisOptions? analysisOptions, + Future Function()? sdkSummaryGenerator, + PackageConfig? packageConfig, + ]) => + AnalyzerResolvers._( + analysisOptions: analysisOptions, + sdkSummaryGenerator: sdkSummaryGenerator, + packageConfig: packageConfig, + // For backwards compatibility we use the shared instance here. + uriResolver: BuildAssetUriResolver.sharedInstance); + + /// See [AnalyzerResolvers.custom] for docs. + AnalyzerResolvers._({ + AnalysisOptions? analysisOptions, + PackageConfig? packageConfig, + Future Function()? sdkSummaryGenerator, + required BuildAssetUriResolver uriResolver, + }) : _analysisOptions = analysisOptions ?? (AnalysisOptionsImpl() ..contextFeatures = _featureSet(enableExperiments: enabledExperiments)), + _packageConfig = packageConfig, _sdkSummaryGenerator = - sdkSummaryGenerator ?? defaultSdkSummaryGenerator; + sdkSummaryGenerator ?? defaultSdkSummaryGenerator, + _uriResolver = uriResolver; + + /// The instance that most real build systems should use. + static final AnalyzerResolvers sharedInstance = + AnalyzerResolvers._(uriResolver: BuildAssetUriResolver.sharedInstance); /// Create a Resolvers backed by an `AnalysisContext` using options /// [_analysisOptions]. Future _ensureInitialized() { return Result.release(_initialized ??= Result.capture(() async { _warnOnLanguageVersionMismatch(); - final uriResolver = _uriResolver = BuildAssetUriResolver.instance; final loadedConfig = _packageConfig ??= await loadPackageConfigUri((await Isolate.packageConfig)!); - var driver = await analysisDriver(uriResolver, _analysisOptions, + var driver = await analysisDriver(_uriResolver, _analysisOptions, await _sdkSummaryGenerator(), loadedConfig); - _resolver = - AnalyzerResolver(driver, _driverPool, _readAndWritePool, uriResolver); + _resolver = AnalyzerResolver( + driver, _driverPool, _readAndWritePool, _uriResolver); }())); } @@ -439,7 +475,7 @@ class AnalyzerResolvers implements Resolvers { /// Must be called between each build. @override void reset() { - _uriResolver?.reset(); + _uriResolver.reset(); } } diff --git a/build_resolvers/pubspec.yaml b/build_resolvers/pubspec.yaml index e3a280112..dc5c06166 100644 --- a/build_resolvers/pubspec.yaml +++ b/build_resolvers/pubspec.yaml @@ -1,5 +1,5 @@ name: build_resolvers -version: 2.3.2 +version: 2.4.0-wip description: Resolve Dart code in a Builder repository: https://github.com/dart-lang/build/tree/master/build_resolvers diff --git a/build_runner_core/CHANGELOG.md b/build_runner_core/CHANGELOG.md index fae430c04..2ae514aea 100644 --- a/build_runner_core/CHANGELOG.md +++ b/build_runner_core/CHANGELOG.md @@ -1,7 +1,9 @@ -## 7.2.11-dev +## 7.2.11-wip - Add better error messages for incomplete package configs when creating a package graph. +- Update to build_resolvers 2.4.0 and use the shared analyzer resolvers + instance. ## 7.2.10 diff --git a/build_runner_core/lib/src/generate/options.dart b/build_runner_core/lib/src/generate/options.dart index b49c8ef52..71bfe5a64 100644 --- a/build_runner_core/lib/src/generate/options.dart +++ b/build_runner_core/lib/src/generate/options.dart @@ -208,7 +208,7 @@ feature, you may need to run `dart run build_runner clean` and then rebuild. } trackPerformance = true; } - resolvers ??= AnalyzerResolvers(); + resolvers ??= AnalyzerResolvers.sharedInstance; return BuildOptions._( debounceDelay: debounceDelay, diff --git a/build_runner_core/pubspec.yaml b/build_runner_core/pubspec.yaml index 0ee3e5d42..3fa8c7044 100644 --- a/build_runner_core/pubspec.yaml +++ b/build_runner_core/pubspec.yaml @@ -1,5 +1,5 @@ name: build_runner_core -version: 7.2.11-dev +version: 7.2.11-wip description: Core tools to organize the structure of a build and run Builders. repository: https://github.com/dart-lang/build/tree/master/build_runner_core @@ -15,7 +15,7 @@ dependencies: async: ^2.5.0 build: ^2.4.0 build_config: ^1.0.0 - build_resolvers: ^2.0.0 + build_resolvers: ^2.4.0 collection: ^1.15.0 convert: ^3.0.0 crypto: ^3.0.0 diff --git a/build_test/CHANGELOG.md b/build_test/CHANGELOG.md index 2ce9d9074..4c1d10573 100644 --- a/build_test/CHANGELOG.md +++ b/build_test/CHANGELOG.md @@ -1,6 +1,8 @@ ## 2.2.1-dev - Avoid passing a nullable value to `Future.value`. +- Update to build_resolvers 2.4.0, which resolves some race conditions when + using multiple resolvers instances. ## 2.2.0 diff --git a/build_test/lib/src/resolve_source.dart b/build_test/lib/src/resolve_source.dart index dbce4f301..a2dcab8a2 100644 --- a/build_test/lib/src/resolve_source.dart +++ b/build_test/lib/src/resolve_source.dart @@ -15,8 +15,6 @@ import 'in_memory_writer.dart'; import 'multi_asset_reader.dart'; import 'package_reader.dart'; -final defaultResolvers = AnalyzerResolvers(); - /// Marker constant that may be used in combination with [resolveSources]. /// /// Use of this string means instead of using the contents of the string as the @@ -199,8 +197,8 @@ Future _resolveAssets( // Use the default resolver if no experiments are enabled. This is much // faster. resolvers ??= packageConfig == null && enabledExperiments.isEmpty - ? defaultResolvers - : AnalyzerResolvers(null, null, resolvedConfig); + ? AnalyzerResolvers.sharedInstance + : AnalyzerResolvers.custom(packageConfig: resolvedConfig); // We don't care about the results of this build, but we also can't await // it because that would block on the `tearDown` of the `resolveBuilder`. diff --git a/build_test/lib/src/test_builder.dart b/build_test/lib/src/test_builder.dart index b4363c21e..916f48f6b 100644 --- a/build_test/lib/src/test_builder.dart +++ b/build_test/lib/src/test_builder.dart @@ -14,7 +14,6 @@ import 'assets.dart'; import 'in_memory_reader.dart'; import 'in_memory_writer.dart'; import 'multi_asset_reader.dart'; -import 'resolve_source.dart'; import 'written_asset_reader.dart'; AssetId _passThrough(AssetId id) => id; @@ -171,8 +170,8 @@ Future testBuilder( var logger = Logger('testBuilder'); var logSubscription = logger.onRecord.listen(onLog); var resolvers = packageConfig == null && enabledExperiments.isEmpty - ? defaultResolvers - : AnalyzerResolvers(null, null, packageConfig); + ? AnalyzerResolvers.sharedInstance + : AnalyzerResolvers.custom(packageConfig: packageConfig); for (var input in inputIds) { // create another writer spy and reader for each input. This prevents writes diff --git a/build_test/pubspec.yaml b/build_test/pubspec.yaml index 29c3cb446..fe624c1ae 100644 --- a/build_test/pubspec.yaml +++ b/build_test/pubspec.yaml @@ -10,7 +10,7 @@ dependencies: async: ^2.5.0 build: ^2.1.0 build_config: ^1.0.0 - build_resolvers: ^2.0.0 + build_resolvers: ^2.4.0 crypto: ^3.0.0 glob: ^2.0.0 html: ^0.15.0