Fine. Add URI resolution to library manifest signature.
Change-Id: I22de286eda1c3c89224baa99814800225b890330
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/450746
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
diff --git a/pkg/analyzer/lib/src/dart/analysis/file_state.dart b/pkg/analyzer/lib/src/dart/analysis/file_state.dart
index 6a9d6c6..e6101c8 100644
--- a/pkg/analyzer/lib/src/dart/analysis/file_state.dart
+++ b/pkg/analyzer/lib/src/dart/analysis/file_state.dart
@@ -1880,9 +1880,10 @@
LibraryCycle get libraryCycle {
if (_libraryCycle == null) {
computeLibraryCycle(
- file._fsState.withFineDependencies,
- file._fsState._saltForElements,
- this,
+ withFineDependencies: file._fsState.withFineDependencies,
+ saltForElements: file._fsState._saltForElements,
+ sourceFactory: file._fsState._sourceFactory,
+ file: this,
);
}
diff --git a/pkg/analyzer/lib/src/dart/analysis/library_graph.dart b/pkg/analyzer/lib/src/dart/analysis/library_graph.dart
index 77c6ce0..95adb35 100644
--- a/pkg/analyzer/lib/src/dart/analysis/library_graph.dart
+++ b/pkg/analyzer/lib/src/dart/analysis/library_graph.dart
@@ -8,18 +8,25 @@
as graph
show DependencyWalker, Node;
import 'package:analyzer/src/dart/analysis/file_state.dart';
+import 'package:analyzer/src/dart/sdk/sdk.dart';
+import 'package:analyzer/src/generated/source.dart';
import 'package:analyzer/src/summary/api_signature.dart';
import 'package:analyzer/src/utilities/extensions/collection.dart';
import 'package:collection/collection.dart';
/// Ensure that the `FileState._libraryCycle` for the [file] and anything it
/// depends on is computed.
-void computeLibraryCycle(
- bool withFineDependencies,
- Uint32List salt,
- LibraryFileKind file,
-) {
- var libraryWalker = _LibraryWalker(withFineDependencies, salt);
+void computeLibraryCycle({
+ required bool withFineDependencies,
+ required Uint32List saltForElements,
+ required SourceFactory sourceFactory,
+ required LibraryFileKind file,
+}) {
+ var libraryWalker = _LibraryWalker(
+ withFineDependencies: withFineDependencies,
+ saltForElements: saltForElements,
+ sourceFactory: sourceFactory,
+ );
libraryWalker.walk(libraryWalker.getNode(file));
}
@@ -36,6 +43,9 @@
/// The URIs of [libraries].
final Set<Uri> libraryUris;
+ /// The transitive set of package names that this cycle references.
+ final Set<String> transitivePackages;
+
/// The library cycles that this cycle references directly.
final Set<LibraryCycle> directDependencies;
@@ -72,6 +82,7 @@
required this.withFineDependencies,
required this.libraries,
required this.libraryUris,
+ required this.transitivePackages,
required this.directDependencies,
required this.apiSignature,
required this.manifestSignature,
@@ -153,10 +164,15 @@
/// sorted [LibraryCycle]s.
class _LibraryWalker extends graph.DependencyWalker<_LibraryNode> {
final bool withFineDependencies;
- final Uint32List _salt;
+ final Uint32List saltForElements;
+ final SourceFactory sourceFactory;
final Map<LibraryFileKind, _LibraryNode> nodesOfFiles = {};
- _LibraryWalker(this.withFineDependencies, this._salt);
+ _LibraryWalker({
+ required this.withFineDependencies,
+ required this.saltForElements,
+ required this.sourceFactory,
+ });
@override
void evaluate(_LibraryNode v) {
@@ -166,7 +182,7 @@
@override
void evaluateScc(List<_LibraryNode> scc) {
var apiSignature = ApiSignature();
- apiSignature.addUint32List(_salt);
+ apiSignature.addUint32List(saltForElements);
// Sort libraries to produce stable signatures.
scc.sort((first, second) {
@@ -205,12 +221,23 @@
}
}
+ var transitivePackages = {
+ ...directDependencies.expand(
+ (dependency) => dependency.transitivePackages,
+ ),
+ ...libraries
+ .map((library) => library.file.uriProperties.packageName)
+ .nonNulls,
+ };
+
String manifestSignature;
String nonTransitiveApiSignature;
{
var manifestBuilder = ApiSignature();
var apiSignatureBuilder = ApiSignature();
- manifestBuilder.addUint32List(_salt);
+ manifestBuilder.addBytes(saltForElements);
+ _addUriResolutionToSignature(manifestBuilder, transitivePackages);
+
var sortedFiles = libraries
.expand((library) => library.files)
.sortedBy((file) => file.path);
@@ -228,6 +255,7 @@
withFineDependencies: withFineDependencies,
libraries: libraries.toFixedList(),
libraryUris: libraryUris,
+ transitivePackages: transitivePackages,
directDependencies: directDependencies,
apiSignature: apiSignature.toHex(),
manifestSignature: manifestSignature,
@@ -244,6 +272,55 @@
return nodesOfFiles.putIfAbsent(file, () => _LibraryNode(this, file));
}
+ /// Add URI resolution environment to [signature].
+ ///
+ /// Library manifests are reused across analysis contexts. If two contexts
+ /// resolve the same `package:` URI to different file system locations, the
+ /// manifests **must not** be considered interchangeable - otherwise we can
+ /// end up reusing a manifest built for a different package layout and get
+ /// mismatched element IDs.
+ ///
+ /// To make the manifest key sensitive to the resolution environment (without
+ /// over-invalidating), we:
+ ///
+ /// * include the SDK root path (when using a folder-based SDK), and
+ /// * include the file system paths of the **transitively referenced**
+ /// packages only - i.e. the packages in [packageNames], not every package
+ /// visible in the analysis context.
+ ///
+ /// This design strikes a balance:
+ /// * Different package configurations that map a dependency to different
+ /// locations produce different keys, forcing a rebuild where reuse would
+ /// be detrimental to element ID stability.
+ /// * Contexts whose *overall* resolution differs but that map the relevant
+ /// packages identically can still reuse manifests.
+ ///
+ /// Trade-off: a dependency package (or the SDK) moving on disk changes the
+ /// key for all cycles that depend on it, even if the package's *API* hasn't
+ /// changed. This is intentional - package dependencies change rarely, but
+ /// different package of the same (not well configured) workspace would
+ /// constantly churn new element IDs for unfortunate shared library manifest.
+ void _addUriResolutionToSignature(
+ ApiSignature signature,
+ Set<String> packageNames,
+ ) {
+ var sdk = sourceFactory.dartSdk;
+ if (sdk is FolderBasedDartSdk) {
+ signature.addString(sdk.directory.path);
+ }
+
+ var packageMap = sourceFactory.packageMap;
+ if (packageMap != null) {
+ var packagePaths = packageNames
+ .map((packageName) => packageMap[packageName])
+ .nonNulls
+ .expand((folders) => folders)
+ .map((folder) => folder.path)
+ .sorted();
+ signature.addStringList(packagePaths);
+ }
+ }
+
void _appendDirectlyReferenced(
Set<LibraryCycle> directDependencies,
ApiSignature apiSignature,
diff --git a/pkg/analyzer/test/src/dart/analysis/driver_test.dart b/pkg/analyzer/test/src/dart/analysis/driver_test.dart
index b531989..6f8e8db 100644
--- a/pkg/analyzer/test/src/dart/analysis/driver_test.dart
+++ b/pkg/analyzer/test/src/dart/analysis/driver_test.dart
@@ -79530,6 +79530,245 @@
);
}
+ test_manifest_key_differentPackages() async {
+ newFile('/packages/foo_v1/lib/foo.dart', r'''
+class A {}
+''');
+
+ newFile('/packages/foo_v2/lib/foo.dart', r'''
+class A extends A2 {}
+class A2 {}
+''');
+
+ newFile('/packages/bar/lib/bar.dart', r'''
+import 'package:foo/foo.dart';
+class B extends A {}
+''');
+
+ writePackageConfig(
+ '$workspaceRootPath/test_1',
+ PackageConfigFileBuilder()
+ ..add(name: 'foo', rootPath: '/packages/foo_v1')
+ ..add(name: 'bar', rootPath: '/packages/bar')
+ ..add(name: 'test', rootPath: '$workspaceRootPath/test_1'),
+ );
+
+ writePackageConfig(
+ '$workspaceRootPath/test_2',
+ PackageConfigFileBuilder()
+ ..add(name: 'foo', rootPath: '/packages/foo_v2')
+ ..add(name: 'bar', rootPath: '/packages/bar')
+ ..add(name: 'test', rootPath: '$workspaceRootPath/test_2'),
+ );
+
+ var testFile1 = newFile('$workspaceRootPath/test_1/lib/test.dart', r'''
+import 'package:bar/bar.dart';
+class X extends B {}
+''');
+
+ var testFile2 = newFile('$workspaceRootPath/test_2/lib/test.dart', r'''
+import 'package:bar/bar.dart';
+class X extends B {}
+''');
+
+ configuration
+ ..withGetLibraryByUri = false
+ ..withLibraryManifest = true
+ ..withLinkLibraryCycle = true
+ ..withSchedulerStatus = false;
+
+ {
+ var driver = driverFor(testFile1);
+ var collector = DriverEventCollector(driver, idProvider: idProvider);
+
+ var libraryUri = Uri.parse('package:bar/bar.dart');
+ collector.getLibraryByUri('T1', '$libraryUri');
+
+ await assertEventsText(collector, r'''
+[operation] linkLibraryCycle SDK
+[operation] linkLibraryCycle
+ package:foo/foo.dart
+ declaredClasses
+ A: #M0
+ interface: #M1
+ exportMapId: #M2
+ exportMap
+ A: #M0
+[operation] linkLibraryCycle
+ package:bar/bar.dart
+ declaredClasses
+ B: #M3
+ interface: #M4
+ exportMapId: #M5
+ exportMap
+ B: #M3
+''');
+ }
+
+ {
+ var driver = driverFor(testFile2);
+ var collector = DriverEventCollector(driver, idProvider: idProvider);
+
+ var libraryUri = Uri.parse('package:bar/bar.dart');
+ collector.getLibraryByUri('T2', '$libraryUri');
+
+ // Note: new library manifests.
+ // We don't try to reuse `package:bar` library bundle.
+ // Because URI resolution is different in T2, keys are different.
+ await assertEventsText(collector, r'''
+[operation] reuseLinkedBundle SDK
+[operation] linkLibraryCycle
+ package:foo/foo.dart
+ declaredClasses
+ A: #M6
+ interface: #M7
+ A2: #M8
+ interface: #M9
+ exportMapId: #M10
+ exportMap
+ A: #M6
+ A2: #M8
+[operation] linkLibraryCycle
+ package:bar/bar.dart
+ declaredClasses
+ B: #M11
+ interface: #M12
+ exportMapId: #M13
+ exportMap
+ B: #M11
+''');
+ }
+
+ await disposeAnalysisContextCollection();
+
+ {
+ var driver = driverFor(testFile1);
+ var collector = DriverEventCollector(driver, idProvider: idProvider);
+
+ var libraryUri = Uri.parse('package:bar/bar.dart');
+ collector.getLibraryByUri('T3', '$libraryUri');
+
+ // We used different keys in T1 and T2, so both can be used.
+ // This is important, because they use different `package:foo`.
+ await assertEventsText(collector, r'''
+[operation] reuseLinkedBundle SDK
+[operation] reuseLinkedBundle
+ package:foo/foo.dart
+[operation] reuseLinkedBundle
+ package:bar/bar.dart
+''');
+ }
+
+ {
+ var driver = driverFor(testFile2);
+ var collector = DriverEventCollector(driver, idProvider: idProvider);
+
+ var libraryUri = Uri.parse('package:bar/bar.dart');
+ collector.getLibraryByUri('T4', '$libraryUri');
+
+ // We used different keys in T1 and T2, so both can be used.
+ // This is important, because they use different `package:foo`.
+ await assertEventsText(collector, r'''
+[operation] reuseLinkedBundle SDK
+[operation] reuseLinkedBundle
+ package:foo/foo.dart
+[operation] reuseLinkedBundle
+ package:bar/bar.dart
+''');
+ }
+ }
+
+ test_manifest_key_samePackages() async {
+ newFile('/packages/foo/lib/foo.dart', r'''
+class A {}
+''');
+
+ newFile('/packages/bar/lib/bar.dart', r'''
+import 'package:foo/foo.dart';
+class B extends A {}
+''');
+
+ writePackageConfig(
+ '$workspaceRootPath/test_1',
+ PackageConfigFileBuilder()
+ ..add(name: 'foo', rootPath: '/packages/foo')
+ ..add(name: 'test', rootPath: '$workspaceRootPath/test_1'),
+ );
+
+ writePackageConfig(
+ '$workspaceRootPath/test_2',
+ PackageConfigFileBuilder()
+ ..add(name: 'foo', rootPath: '/packages/foo')
+ ..add(name: 'bar', rootPath: '/packages/bar')
+ ..add(name: 'test', rootPath: '$workspaceRootPath/test_2'),
+ );
+
+ var testFile1 = newFile('$workspaceRootPath/test_1/lib/test.dart', r'''
+import 'package:foo/foo.dart';
+''');
+
+ var testFile2 = newFile('$workspaceRootPath/test_2/lib/test.dart', r'''
+import 'package:foo/foo.dart';
+import 'package:bar/bar.dart';
+''');
+
+ configuration
+ ..withGetLibraryByUri = false
+ ..withLibraryManifest = true
+ ..withLinkLibraryCycle = true
+ ..withSchedulerStatus = false;
+
+ {
+ var driver = driverFor(testFile1);
+ var collector = DriverEventCollector(driver, idProvider: idProvider);
+
+ var libraryUri = Uri.parse('package:test/test.dart');
+ collector.getLibraryByUri('T1', '$libraryUri');
+
+ await assertEventsText(collector, r'''
+[operation] linkLibraryCycle SDK
+[operation] linkLibraryCycle
+ package:foo/foo.dart
+ declaredClasses
+ A: #M0
+ interface: #M1
+ exportMapId: #M2
+ exportMap
+ A: #M0
+[operation] linkLibraryCycle
+ package:test/test.dart
+ exportMapId: #M3
+''');
+ }
+
+ {
+ var driver = driverFor(testFile2);
+ var collector = DriverEventCollector(driver, idProvider: idProvider);
+
+ var libraryUri = Uri.parse('package:test/test.dart');
+ collector.getLibraryByUri('T2', '$libraryUri');
+
+ // Note, we reuse `package:foo`, even though T2 has different total
+ // URI resolution rules. But for `package:foo` everything it the same.
+ await assertEventsText(collector, r'''
+[operation] reuseLinkedBundle SDK
+[operation] reuseLinkedBundle
+ package:foo/foo.dart
+[operation] linkLibraryCycle
+ package:bar/bar.dart
+ declaredClasses
+ B: #M4
+ interface: #M5
+ exportMapId: #M6
+ exportMap
+ B: #M4
+[operation] linkLibraryCycle
+ package:test/test.dart
+ exportMapId: #M7
+''');
+ }
+ }
+
test_manifest_library_metadata_add() async {
await _runLibraryManifestScenario(
initialCode: r'''
