diff --git a/build_resolvers/lib/src/analysis_driver_model.dart b/build_resolvers/lib/src/analysis_driver_model.dart index d1550aa1f..c1f12a7d5 100644 --- a/build_resolvers/lib/src/analysis_driver_model.dart +++ b/build_resolvers/lib/src/analysis_driver_model.dart @@ -34,16 +34,24 @@ class AnalysisDriverModel { final MemoryResourceProvider resourceProvider = MemoryResourceProvider(context: p.posix); + /// The import graph of all sources needed for analysis. + final _graph = _Graph(); + + /// Assets that have been synced into the in-memory filesystem + /// [resourceProvider]. + final _syncedOntoResourceProvider = {}; + /// Notifies that [step] has completed. /// /// All build steps must complete before [reset] is called. void notifyComplete(BuildStep step) { - // TODO(davidmorgan): add test coverage, fix implementation. + // This implementation doesn't keep state per `BuildStep`, nothing to do. } /// Clear cached information specific to an individual build. void reset() { - // TODO(davidmorgan): add test coverage, fix implementation. + _graph.clear(); + _syncedOntoResourceProvider.clear(); } /// Attempts to parse [uri] into an [AssetId] and returns it if it is cached. @@ -82,66 +90,85 @@ class AnalysisDriverModel { FutureOr Function(AnalysisDriverForPackageBuild)) withDriverResource, {required bool transitive}) async { - /// TODO(davidmorgan): add test coverage for whether transitive - /// sources are read when [transitive] is false, fix the implementation - /// here. - /// TODO(davidmorgan): add test coverage for whether - /// `.transitive_deps` files cut off the reporting of deps to the - /// [buildStep], fix the implementation here. - - // Find transitive deps, this also informs [buildStep] of all inputs). - final ids = await _expandToTransitive(buildStep, entryPoints); - - // Apply changes to in-memory filesystem. - for (final id in ids) { - if (await buildStep.canRead(id)) { - final content = await buildStep.readAsString(id); - - /// TODO(davidmorgan): add test coverage for when a file is - /// modified rather than added, fix the implementation here. - resourceProvider.newFile(id.asPath, content); - } else { - if (resourceProvider.getFile(id.asPath).exists) { - resourceProvider.deleteFile(id.asPath); - } - } - } - - // Notify the analyzer of changes. + // Immediately take the lock on `driver` so that the whole class state, + // `_graph` and `_readForAnalyzir`, is only mutated by one build step at a + // time. Otherwise, interleaved access complicates processing significantly. await withDriverResource((driver) async { - for (final id in ids) { - // TODO(davidmorgan): add test coverage for over-notification of - // changes, fix the implementaion here. - driver.changeFile(id.asPath); - } - await driver.applyPendingFileChanges(); + return _performResolve(driver, buildStep, entryPoints, withDriverResource, + transitive: transitive); }); } - /// Walks the import graph from [ids], returns full transitive deps. - Future> _expandToTransitive( - AssetReader reader, Iterable ids) async { - final result = ids.toSet(); - final nextIds = Queue.of(ids); - while (nextIds.isNotEmpty) { - final nextId = nextIds.removeFirst(); + Future _performResolve( + AnalysisDriverForPackageBuild driver, + BuildStep buildStep, + List entryPoints, + Future Function( + FutureOr Function(AnalysisDriverForPackageBuild)) + withDriverResource, + {required bool transitive}) async { + var idsToSyncOntoResourceProvider = entryPoints; + Iterable inputIds = entryPoints; - // Skip if not readable. Note that calling `canRead` still makes it a - // dependency of the `BuildStep`. - if (!await reader.canRead(nextId)) continue; + // If requested, find transitive imports. + if (transitive) { + await _graph.load(buildStep, entryPoints); + idsToSyncOntoResourceProvider = _graph.nodes.keys.toList(); + inputIds = _graph.inputsFor(entryPoints); - final content = await reader.readAsString(nextId); - final deps = _parseDependencies(content, nextId); + // Check for inputs that were missing when the directive graph was read + // but have since been written by another build action. + for (final id in inputIds + .where((id) => !id.path.endsWith(_transitiveDigestExtension))) { + if (_graph.nodes[id]!.isMissing) { + if (await buildStep.canRead(id)) { + idsToSyncOntoResourceProvider.add(id); + _syncedOntoResourceProvider.remove(id); + } + } + } + } - // For each dep, if it's not in `result` yet, it's newly-discovered: - // add it to `nextIds`. - for (final dep in deps) { - if (result.add(dep)) { - nextIds.add(dep); + // Notify [buildStep] of its inputs. + for (final id in inputIds) { + await buildStep.canRead(id); + } + + // Sync changes onto the "URI resolver", the in-memory filesystem. + final changedIds = []; + for (final id in idsToSyncOntoResourceProvider) { + if (!_syncedOntoResourceProvider.add(id)) continue; + final content = + await buildStep.canRead(id) ? await buildStep.readAsString(id) : null; + final inMemoryFile = resourceProvider.getFile(id.asPath); + final inMemoryContent = + inMemoryFile.exists ? inMemoryFile.readAsStringSync() : null; + + if (content != inMemoryContent) { + if (content == null) { + // TODO(davidmorgan): per "globallySeenAssets" in + // BuildAssetUriResolver, deletes should only be applied at the end + // of the build, in case the file is actually there but not visible + // to the current reader. + resourceProvider.deleteFile(id.asPath); + changedIds.add(id); + } else { + if (inMemoryContent == null) { + resourceProvider.newFile(id.asPath, content); + } else { + resourceProvider.modifyFile(id.asPath, content); + } + changedIds.add(id); } } } - return result; + + // Notify the analyzer of changes and wait for it to update its internal + // state. + for (final id in changedIds) { + driver.changeFile(id.asPath); + } + await driver.applyPendingFileChanges(); } } @@ -167,3 +194,109 @@ extension _AssetIdExtensions on AssetId { /// Asset path for the in-memory filesystem. String get asPath => AnalysisDriverModelUriResolver.assetPath(this); } + +/// The directive graph of all known sources. +/// +/// Also tracks whether there is a `.transitive_digest` file next to each source +/// asset, and tracks missing files. +class _Graph { + final Map nodes = {}; + + /// Walks the import graph from [ids] loading into [nodes]. + Future load(AssetReader reader, Iterable ids) async { + // TODO(davidmorgan): check if List is faster. + final nextIds = Queue.of(ids); + while (nextIds.isNotEmpty) { + final nextId = nextIds.removeFirst(); + + // Skip if already seen. + if (nodes.containsKey(nextId)) continue; + + final hasTransitiveDigestAsset = + await reader.canRead(nextId.addExtension(_transitiveDigestExtension)); + + // Skip if not readable. + if (!await reader.canRead(nextId)) { + nodes[nextId] = _Node.missing( + id: nextId, hasTransitiveDigestAsset: hasTransitiveDigestAsset); + continue; + } + + final content = await reader.readAsString(nextId); + final deps = _parseDependencies(content, nextId); + nodes[nextId] = _Node( + id: nextId, + deps: deps, + hasTransitiveDigestAsset: hasTransitiveDigestAsset); + nextIds.addAll(deps.where((id) => !nodes.containsKey(id))); + } + } + + void clear() { + nodes.clear(); + } + + /// The inputs for a build action analyzing [entryPoints]. + /// + /// This is transitive deps, but cut off by the presence of any + /// `.transitive_digest` file next to an asset. + Set inputsFor(Iterable entryPoints) { + final result = entryPoints.toSet(); + final nextIds = Queue.of(entryPoints); + + while (nextIds.isNotEmpty) { + final nextId = nextIds.removeFirst(); + final node = nodes[nextId]!; + + // Add the transitive digest file as an input. If it exists, skip deps. + result.add(nextId.addExtension(_transitiveDigestExtension)); + if (node.hasTransitiveDigestAsset) { + continue; + } + + // Skip if there are no deps because the file is missing. + if (node.isMissing) continue; + + // For each dep, if it's not in `result` yet, it's newly-discovered: + // add it to `nextIds`. + for (final dep in node.deps) { + if (result.add(dep)) { + nextIds.add(dep); + } + } + } + return result; + } + + @override + String toString() => nodes.toString(); +} + +/// A node in the directive graph. +class _Node { + final AssetId id; + final List deps; + final bool isMissing; + final bool hasTransitiveDigestAsset; + + _Node( + {required this.id, + required this.deps, + required this.hasTransitiveDigestAsset}) + : isMissing = false; + + _Node.missing({required this.id, required this.hasTransitiveDigestAsset}) + : isMissing = true, + deps = const []; + + @override + String toString() => '$id:' + '${hasTransitiveDigestAsset ? 'digest:' : ''}' + '${isMissing ? 'missing' : deps}'; +} + +// Transitive digest files are built next to source inputs. As the name +// suggests, they contain the transitive digest of all deps of the file. +// So, establishing a dependency on a transitive digest file is equivalent +// to establishing a dependency on all deps of the file. +const _transitiveDigestExtension = '.transitive_digest'; diff --git a/build_resolvers/test/resolver_test.dart b/build_resolvers/test/resolver_test.dart index e4310f620..89ac10ff9 100644 --- a/build_resolvers/test/resolver_test.dart +++ b/build_resolvers/test/resolver_test.dart @@ -26,7 +26,9 @@ import 'package:test/test.dart'; void main() { for (final resolversFactory in [ BuildAssetUriResolversFactory(), - AnalysisDriverModelFactory() + SharedBuildAssetUriResolversFactory(), + AnalysisDriverModelFactory(), + SharedAnalysisDriverModelFactory() ]) { group('$resolversFactory', () { runTests(resolversFactory); @@ -38,6 +40,7 @@ void runTests(ResolversFactory resolversFactory) { final entryPoint = AssetId('a', 'web/main.dart'); Resolvers createResolvers({PackageConfig? packageConfig}) => resolversFactory.create(packageConfig: packageConfig); + final isSharedResolvers = resolversFactory.isInstanceShared; test('should handle initial files', () { return resolveSources({ @@ -92,6 +95,147 @@ void runTests(ResolversFactory resolversFactory) { }, resolvers: createResolvers()); }); + test('informs buildStep that transitive imports were read', () { + return resolveSources({ + 'a|web/main.dart': ''' + import 'a.dart'; + main() { + } ''', + 'a|web/a.dart': ''' + library a; + import 'b.dart'; + ''', + 'a|web/b.dart': ''' + library b; + ''', + }, (resolver) async { + await resolver.libraryFor(entryPoint); + }, assetReaderChecks: (reader) { + expect(reader.assetsRead, { + AssetId('a', 'web/main.dart'), + AssetId('a', 'web/main.dart.transitive_digest'), + AssetId('a', 'web/a.dart'), + AssetId('a', 'web/a.dart.transitive_digest'), + AssetId('a', 'web/b.dart'), + AssetId('a', 'web/b.dart.transitive_digest'), + }); + }, resolvers: createResolvers()); + }); + + test('transitive_digest file cuts off deps reported to buildStep', + // This test requires a new resolvers instance. + skip: isSharedResolvers, () async { + // BuildAssetUriResolver cuts off a buildStep's deps tree if it encounters + // a `.transitive_digest` file, because that file stands in for the + // transitive deps from that point. + // + // But, the _first_ time it is reading files it continues to read them, + // because the analyzer still needs all the files. So the first build + // action will see all the files as deps. + final resolvers = createResolvers(); + final sources = { + 'a|web/main.dart': ''' + import 'a.dart'; + main() { + } ''', + 'a|web/a.dart': ''' + library a; + import 'b.dart'; + ''', + 'a|web/a.dart.transitive_digest': ''' + library a; + import 'b.dart'; + ''', + 'a|web/b.dart': ''' + library b; + ''', + }; + // First action sees all the files as inputs. + await resolveSources(sources, (resolver) async { + await resolver.libraryFor(entryPoint); + }, assetReaderChecks: (reader) { + expect(reader.assetsRead, { + AssetId('a', 'web/main.dart'), + AssetId('a', 'web/main.dart.transitive_digest'), + AssetId('a', 'web/a.dart'), + AssetId('a', 'web/a.dart.transitive_digest'), + AssetId('a', 'web/b.dart'), + AssetId('a', 'web/b.dart.transitive_digest'), + }); + }, resolvers: resolvers); + // Second action has inputs cut off at the `transitive_digest` file. + await resolveSources(sources, (resolver) async { + await resolver.libraryFor(entryPoint); + }, assetReaderChecks: (reader) { + expect(reader.assetsRead, { + AssetId('a', 'web/main.dart'), + AssetId('a', 'web/main.dart.transitive_digest'), + AssetId('a', 'web/a.dart'), + AssetId('a', 'web/a.dart.transitive_digest'), + }); + }, resolvers: resolvers); + }); + + test('updates following a change to source', () async { + var resolvers = createResolvers(); + await resolveSources({ + 'a|web/main.dart': ''' + class A {} + ''', + }, (resolver) async { + var lib = await resolver.libraryFor(entryPoint); + expect(lib.getClass('A'), isNotNull); + expect(lib.getClass('B'), isNull); + }, resolvers: resolvers); + + // Only allow changes to source between builds. + await _resetResolvers(resolvers); + + await resolveSources({ + 'a|web/main.dart': ''' + class B {} + ''', + }, (resolver) async { + var lib = await resolver.libraryFor(entryPoint); + expect(lib.getClass('A'), isNull); + expect(lib.getClass('B'), isNotNull); + }, resolvers: resolvers); + }); + + test('updates following a change to import graph', () async { + var resolvers = createResolvers(); + await resolveSources({ + 'a|web/main.dart': ''' + part 'main.part1.dart'; + ''', + 'a|web/main.part1.dart': ''' + part of 'main.dart'; + class A {} + ''', + }, (resolver) async { + var lib = await resolver.libraryFor(entryPoint); + expect(lib.getClass('A'), isNotNull); + expect(lib.getClass('B'), isNull); + }, resolvers: resolvers); + + // Only allow changes to source between builds. + await _resetResolvers(resolvers); + + await resolveSources({ + 'a|web/main.dart': ''' + part 'main.part1.dart'; + ''', + 'a|web/main.part1.dart': ''' + part of 'main.dart'; + class B {} + ''', + }, (resolver) async { + var lib = await resolver.libraryFor(entryPoint); + expect(lib.getClass('A'), isNull); + expect(lib.getClass('B'), isNotNull); + }, resolvers: resolvers); + }); + test('should still crawl transitively after a call to isLibrary', () { return resolveSources({ 'a|web/main.dart': ''' @@ -340,16 +484,7 @@ void runTests(ResolversFactory resolversFactory) { expect(clazz.interfaces.first.getDisplayString(), 'B'); }, resolvers: resolvers); - // `resolveSources` actually completes prior to the build step being - // done, which causes this `reset` call to fail. After a few microtasks - // it succeeds though. - var tries = 0; - while (tries++ < 5) { - await Future.value(null); - try { - resolvers.reset(); - } catch (_) {} - } + await _resetResolvers(resolvers); await resolveSources({ 'a|web/main.dart': ''' @@ -953,26 +1088,76 @@ final _skipOnPreRelease = : null; abstract class ResolversFactory { + /// Whether [create] returns a shared instance that persists between tests. + bool get isInstanceShared; Resolvers create({PackageConfig? packageConfig}); } class BuildAssetUriResolversFactory implements ResolversFactory { + @override + bool get isInstanceShared => false; + @override Resolvers create({PackageConfig? packageConfig}) => AnalyzerResolvers.custom( packageConfig: packageConfig, - analysisDriverModel: BuildAssetUriResolver.sharedInstance); + analysisDriverModel: BuildAssetUriResolver()); @override String toString() => 'Resolver'; } +class SharedBuildAssetUriResolversFactory implements ResolversFactory { + @override + bool get isInstanceShared => true; + + @override + Resolvers create({PackageConfig? packageConfig}) => AnalyzerResolvers.custom( + packageConfig: packageConfig, + analysisDriverModel: BuildAssetUriResolver.sharedInstance); + + @override + String toString() => 'Shared resolver'; +} + class AnalysisDriverModelFactory implements ResolversFactory { - final AnalysisDriverModel sharedInstance = AnalysisDriverModel(); + @override + bool get isInstanceShared => false; @override Resolvers create({PackageConfig? packageConfig}) => AnalyzerResolvers.custom( - packageConfig: packageConfig, analysisDriverModel: sharedInstance); + packageConfig: packageConfig, analysisDriverModel: AnalysisDriverModel()); @override String toString() => 'New resolver'; } + +class SharedAnalysisDriverModelFactory implements ResolversFactory { + static final AnalysisDriverModel sharedInstance = AnalysisDriverModel(); + + @override + bool get isInstanceShared => true; + + @override + Resolvers create({PackageConfig? packageConfig}) { + final result = AnalyzerResolvers.custom( + packageConfig: packageConfig, analysisDriverModel: sharedInstance); + addTearDown(result.reset); + return result; + } + + @override + String toString() => 'Shared new resolver'; +} + +Future _resetResolvers(Resolvers resolvers) async { + // `resolveSources` actually completes prior to the build step being + // done, which causes this `reset` call to fail. After a few microtasks + // it succeeds though. + var tries = 0; + while (tries++ < 5) { + await Future.value(null); + try { + resolvers.reset(); + } catch (_) {} + } +} diff --git a/build_test/CHANGELOG.md b/build_test/CHANGELOG.md index 1dc2670ab..c9c9b86ab 100644 --- a/build_test/CHANGELOG.md +++ b/build_test/CHANGELOG.md @@ -1,5 +1,7 @@ ## 2.2.4-wip +- Support checks on reader state after a build acion in `resolveSources`. + ## 2.2.3 - Bump the min sdk to 3.6.0. diff --git a/build_test/lib/src/resolve_source.dart b/build_test/lib/src/resolve_source.dart index a2dcab8a2..3dc132e20 100644 --- a/build_test/lib/src/resolve_source.dart +++ b/build_test/lib/src/resolve_source.dart @@ -114,12 +114,16 @@ Future resolveSource( /// **NOTE**: All `package` dependencies are resolved using [PackageAssetReader] /// - by default, [PackageAssetReader.currentIsolate]. A custom [packageConfig] /// may be provided to map files not visible to the current package's runtime. +/// +/// [assetReaderChecks], if provided, runs after the action completes and can be +/// used to add expectations on the reader state. Future resolveSources( Map inputs, FutureOr Function(Resolver resolver) action, { PackageConfig? packageConfig, String? resolverFor, String? rootPackage, + FutureOr Function(InMemoryAssetReader)? assetReaderChecks, Future? tearDown, Resolvers? resolvers, }) { @@ -132,6 +136,7 @@ Future resolveSources( action, packageConfig: packageConfig, resolverFor: AssetId.parse(resolverFor ?? inputs.keys.first), + assetReaderChecks: assetReaderChecks, tearDown: tearDown, resolvers: resolvers, ); @@ -169,6 +174,7 @@ Future _resolveAssets( FutureOr Function(Resolver resolver) action, { PackageConfig? packageConfig, AssetId? resolverFor, + FutureOr Function(InMemoryAssetReader)? assetReaderChecks, Future? tearDown, Resolvers? resolvers, }) async { @@ -215,7 +221,11 @@ Future _resolveAssets( InMemoryAssetWriter(), resolvers, ).catchError((_) {})); - return resolveBuilder.onDone.future; + final result = await resolveBuilder.onDone.future; + if (assetReaderChecks != null) { + await assetReaderChecks(inMemory); + } + return result; } /// A [Builder] that is only used to retrieve a [Resolver] instance.