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.

Author: jakemac53

build/CHANGELOG.md

@@ -1,3 +1,5 @@
+## 2.4.2-wip
+
 ## 2.4.1
 
 - Allow the latest analyzer (6.x.x).

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

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 = <AssetId>{};
     var buildStep = BuildStepImpl(makeAssetId(), [], reader, writer,
-        AnalyzerResolvers(), resourceManager, _unsupported,
+        AnalyzerResolvers.custom(), resourceManager, _unsupported,
         reportUnusedAssets: unused.addAll);
     var reported = [
       makeAssetId(),

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

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 = <BuildStep, HashSet<AssetId>>{};
 
-  /// 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<AssetId> _parseDependencies(String content, AssetId from) => HashSet.of(
 /// directives, and cache the results if they weren't already cached.
 Future<Iterable<AssetId>?> dependenciesOf(
         AssetId id, BuildStep buildStep) async =>
-    (await BuildAssetUriResolver.instance
+    (await BuildAssetUriResolver.sharedInstance
             ._updateCachedAssetState(id, buildStep))
         ?.dependencies;
 

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<Result<void>>? _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<String> Function()? sdkSummaryGenerator,
-      this._packageConfig])
-      : _analysisOptions = analysisOptions ??
+  factory AnalyzerResolvers.custom({
+    AnalysisOptions? analysisOptions,
+    Future<String> 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<String> 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<String> 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<void> _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();
   }
 }
 

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
 

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
 

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,

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

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
 

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<T> _resolveAssets<T>(
   // 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`.

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

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