From 86b9a0b4d8247c7109deb96167df929f7db60d3b Mon Sep 17 00:00:00 2001 From: Paul Johnston Date: Mon, 15 Jun 2026 20:41:36 -0600 Subject: [PATCH] fix(ext): _ShimAnalyzerResolver.libraries should yield cross-package Resolver.libraries previously yielded only same-package staged files (the input + each --dep). Cross-package public re-export libraries - e.g. package:stacked_services/stacked_services.dart - were never yielded, even though they're reachable through the analyzer's session and import directly from the input under analysis. build_runner's Resolver yields every library the analysis session has loaded. Generators that walk Resolver.libraries to look up a type's public re-exporting library (notably stacked_generator's ImportResolver) silently fail under the narrower set, computing wrong import paths or returning null for elements that ARE reachable. Yield each package's conventional package:/.dart library from the synthesized PackageConfig in addition to the same-package staged entries. The lazy analyzer load keeps the cost negligible - libraryFor only materializes packages the consumer iterates over. Adds four tests covering: baseline same-package yield, cross-package yield (using an on-disk fake foreign package), no-double-yield dedup, and tolerance of packages with non-conventional layouts. --- dart/ext/BUILD.bazel | 1 + dart/ext/lib/builder_shim.dart | 36 ++++- dart/ext/test/builder_shim_test.dart | 199 +++++++++++++++++++++++++++ 3 files changed, 235 insertions(+), 1 deletion(-) diff --git a/dart/ext/BUILD.bazel b/dart/ext/BUILD.bazel index 1360808..252c505 100644 --- a/dart/ext/BUILD.bazel +++ b/dart/ext/BUILD.bazel @@ -38,6 +38,7 @@ dart_test( main = "test/builder_shim_test.dart", deps = [ ":builder_shim", + "@ext_pub_deps//:analyzer", "@ext_pub_deps//:build", "@ext_pub_deps//:path", "@ext_pub_deps//:test", diff --git a/dart/ext/lib/builder_shim.dart b/dart/ext/lib/builder_shim.dart index 1c77dd8..cd5b80a 100644 --- a/dart/ext/lib/builder_shim.dart +++ b/dart/ext/lib/builder_shim.dart @@ -1186,13 +1186,47 @@ class _ShimAnalyzerResolver implements Resolver { @override Stream get libraries async* { + // Yield same-package staged libraries first — the input + each + // `--dep` file the shim copies into the staging dir. These appear + // directly in _assetIdToPath. + final yielded = {}; for (final id in _assetIdToPath.keys) { try { - yield await libraryFor(id); + final lib = await libraryFor(id); + if (yielded.add(lib.firstFragment.source.uri)) yield lib; } on NonLibraryAssetException { // Skip non-library assets (e.g. plain text deps). } } + + // Plus cross-package public re-export libraries — `package:/.dart` + // for every package in the synthesized PackageConfig. Without this, the + // stream is bounded by `_assetIdToPath` (same-package siblings only) and + // misses any third-party library the input transitively imports. + // + // build_runner's Resolver yields every library the analysis session has + // loaded, including the transitive import closure. Generators that walk + // `Resolver.libraries` to locate elements via their public re-exporting + // library (e.g. stacked_generator's ImportResolver) silently fail under + // the narrower set, either returning null for reachable elements or + // computing wrong import paths. + // + // Enumerating each package's conventional main library covers the + // re-export use case at negligible cost (libraryFor lazily materializes + // only the packages the consumer iterates). + for (final pkg in _packageConfig.packages) { + final id = AssetId(pkg.name, 'lib/${pkg.name}.dart'); + try { + final lib = await libraryFor(id); + if (yielded.add(lib.firstFragment.source.uri)) yield lib; + } on NonLibraryAssetException { + // Package has no conventional main library (e.g. CLI-only tools + // shipping `bin/` only, packages with non-standard layouts). + } on AssetNotFoundException { + // Package declared in package_config but no file at the + // conventional path — e.g. workspace packages with custom layouts. + } + } } @override diff --git a/dart/ext/test/builder_shim_test.dart b/dart/ext/test/builder_shim_test.dart index 7717b91..21cd30b 100644 --- a/dart/ext/test/builder_shim_test.dart +++ b/dart/ext/test/builder_shim_test.dart @@ -2,6 +2,7 @@ import 'dart:async'; import 'dart:convert'; import 'dart:io'; +import 'package:analyzer/dart/element/element.dart'; import 'package:bazel_worker/bazel_worker.dart'; import 'package:build/build.dart'; import 'package:package_config/package_config.dart'; @@ -31,6 +32,11 @@ List _baseArgs({ return [for (final e in args.entries) ...[e.key, e.value]]; } +/// Stable URI string for a LibraryElement. Used by the Resolver.libraries +/// tests to compare yielded libraries by `package:foo/bar.dart` shape. +String _libUriString(LibraryElement lib) => + lib.firstFragment.source.uri.toString(); + ShimArgs _shimArgs({ required String inputPath, required String inputAssetPath, @@ -40,6 +46,7 @@ ShimArgs _shimArgs({ List<({String exec, String asset})> depPaths = const [], Map config = const {}, LanguageVersion? rootLanguageVersion, + String? packageConfigPath, }) => ShimArgs( inputPath: inputPath, @@ -49,8 +56,53 @@ ShimArgs _shimArgs({ depPaths: depPaths, config: config, rootLanguageVersion: rootLanguageVersion ?? LanguageVersion(3, 11), + packageConfigUri: packageConfigPath, ); +/// Writes a fake foreign Dart package under [root] named [name] with a +/// single library file `lib/.dart` (the conventional public +/// re-export location). Returns the package directory. +/// +/// Used by the Resolver.libraries tests to exercise the cross-package +/// fallback without depending on whatever pub graph the test runner's +/// isolate happens to expose. +Directory _writeForeignPackage({ + required Directory root, + required String name, + String libBody = 'class Foo {}\n', +}) { + final pkgDir = Directory(p.join(root.path, '${name}_pkg')) + ..createSync(recursive: true); + Directory(p.join(pkgDir.path, 'lib')).createSync(recursive: true); + File(p.join(pkgDir.path, 'lib', '$name.dart')).writeAsStringSync(libBody); + return pkgDir; +} + +/// Writes a `package_config.json` at [root]/package_config.json that +/// includes [packages] (each a Directory whose name is `_pkg`). +/// Returns the file path. +String _writePackageConfig({ + required Directory root, + required List packages, +}) { + final entries = >[]; + for (final pkg in packages) { + final name = p.basename(pkg.path).replaceAll('_pkg', ''); + entries.add({ + 'name': name, + 'rootUri': Uri.directory(pkg.path).toString(), + 'packageUri': 'lib/', + 'languageVersion': '3.0', + }); + } + final file = File(p.join(root.path, 'package_config.json')); + file.writeAsStringSync(jsonEncode({ + 'configVersion': 2, + 'packages': entries, + })); + return file.path; +} + void main() { group('stagedPath', () { test('joins a POSIX asset under a native Windows dir, no mixed separators', @@ -365,6 +417,153 @@ void main() { }); }); + group('Resolver.libraries', () { + late Directory tmp; + + setUp(() async { + tmp = await Directory.systemTemp.createTemp('shim_resolver_libs_'); + }); + + tearDown(() async { + await tmp.delete(recursive: true); + }); + + test('yields same-package staged libraries (input + each --dep)', + () async { + // Baseline: the input file and every --dep file in the same package + // must appear in Resolver.libraries. Guards the pre-fix behavior. + final input = File(p.join(tmp.path, 'src.dart')) + ..writeAsStringSync('class Foo {}'); + final dep = File(p.join(tmp.path, 'sibling.dart')) + ..writeAsStringSync('class Bar {}'); + final output = File(p.join(tmp.path, 'src.g.dart')); + + Set uris = {}; + await runShimWithArgs( + _shimArgs( + inputPath: input.path, + inputAssetPath: 'lib/src.dart', + outputPath: output.path, + depPaths: [(exec: dep.path, asset: 'lib/sibling.dart')], + ), + (_) => _CapturingBuilder((step) async { + final libs = await step.resolver.libraries.toList(); + uris = libs.map(_libUriString).toSet(); + }), + ); + + // Same-package staged files exist as `package:fixture/...` URIs in + // the synthetic root; just assert at least one is yielded — the + // analyzer may canonicalise either input.dart or sibling.dart first. + expect( + uris.any((u) => u.startsWith('package:fixture/')), + isTrue, + reason: 'expected at least one package:fixture/ library, got $uris', + ); + }); + + test('yields cross-package public re-export libraries from PackageConfig', + () async { + // The fix under test: a third-party package whose conventional main + // library exists (`package:/.dart`) is yielded even + // though it never appears in _assetIdToPath. Pre-fix, only same- + // package staged libraries were yielded and generators like + // stacked_generator's ImportResolver missed reachable types. + final fooPkg = _writeForeignPackage(root: tmp, name: 'foo'); + final pkgConfig = _writePackageConfig(root: tmp, packages: [fooPkg]); + + final input = File(p.join(tmp.path, 'src.dart')) + ..writeAsStringSync( + "import 'package:foo/foo.dart';\nclass Bar extends Foo {}\n", + ); + final output = File(p.join(tmp.path, 'src.g.dart')); + + Set uris = {}; + await runShimWithArgs( + _shimArgs( + inputPath: input.path, + inputAssetPath: 'lib/src.dart', + outputPath: output.path, + packageConfigPath: pkgConfig, + ), + (_) => _CapturingBuilder((step) async { + final libs = await step.resolver.libraries.toList(); + uris = libs.map(_libUriString).toSet(); + }), + ); + + expect( + uris, + contains('package:foo/foo.dart'), + reason: 'expected package:foo/foo.dart to be yielded, got $uris', + ); + }); + + test('does not double-yield a library covered by both paths', () async { + // If a library is reachable both via `_assetIdToPath` and via the + // PackageConfig fallback, it should be yielded once. Guards against + // the analyzer caller observing duplicate elements. + final fooPkg = _writeForeignPackage(root: tmp, name: 'foo'); + final pkgConfig = _writePackageConfig(root: tmp, packages: [fooPkg]); + + final input = File(p.join(tmp.path, 'src.dart')) + ..writeAsStringSync("import 'package:foo/foo.dart';\nclass Bar {}\n"); + final output = File(p.join(tmp.path, 'src.g.dart')); + + List uriList = []; + await runShimWithArgs( + _shimArgs( + inputPath: input.path, + inputAssetPath: 'lib/src.dart', + outputPath: output.path, + packageConfigPath: pkgConfig, + ), + (_) => _CapturingBuilder((step) async { + final libs = await step.resolver.libraries.toList(); + uriList = libs.map(_libUriString).toList(); + }), + ); + + // Set length == list length means no duplicates. + expect(uriList.length, equals(uriList.toSet().length), + reason: 'duplicate URIs in libraries: $uriList'); + }); + + test('tolerates packages with no conventional main library', () async { + // Packages may lack `lib/.dart` (CLI tools shipping `bin/` + // only, packages with non-standard layouts). The new loop must + // skip them — not throw — so iteration continues to packages that + // do have one. + // + // Set up a package_config with a package whose root directory + // exists but has no `lib/.dart` file. Draining `libraries` + // must complete without an exception. + final emptyPkgDir = Directory(p.join(tmp.path, 'empty_pkg')) + ..createSync(recursive: true); + Directory(p.join(emptyPkgDir.path, 'lib')).createSync(recursive: true); + final pkgConfig = _writePackageConfig(root: tmp, packages: [emptyPkgDir]); + + final input = File(p.join(tmp.path, 'src.dart')) + ..writeAsStringSync('class Foo {}'); + final output = File(p.join(tmp.path, 'src.g.dart')); + + await runShimWithArgs( + _shimArgs( + inputPath: input.path, + inputAssetPath: 'lib/src.dart', + outputPath: output.path, + packageConfigPath: pkgConfig, + ), + (_) => _CapturingBuilder((step) async { + // Just drain the stream — assert no exception escapes. + await step.resolver.libraries.toList(); + }), + ); + + expect(output.existsSync(), isTrue); + }); + }); + group('runShim diagnostic sink', () { late Directory tmp; setUp(() async {