Load members from load paths and node_modules using original URLs

Rather than loading the stylesheets in which these members were literally defined, load stylesheets that transitively import those stylesheets. Since we aren't migrating these files, the transitive imports will continue to work, and this avoids potentially importing from within a package's private filesystem structure.
sass · Oct 1, 2019 · e151498 · e151498
1 parent 755afd0
commit e151498
Show file tree

Hide file tree

Showing 6 changed files with 100 additions and 30 deletions.
diff --git a/lib/src/migrators/module.dart b/lib/src/migrators/module.dart
@@ -933,8 +933,17 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
   /// new `@use` rule if necessary.
   String _namespaceForDeclaration(MemberDeclaration declaration) {
     if (declaration == null) return null;
+
     var url = declaration.sourceUrl;
     if (url == currentUrl) return null;
+
+    // If we can load [declaration] from a library entrypoint URL, do so. Choose
+    // the shortest one if there are multiple options.
+    var libraryUrls = references.libraries[declaration];
+    if (libraryUrls != null) {
+      url = minBy(libraryUrls, (url) => url.pathSegments.length);
+    }
+
     if (!_usedUrls.contains(url)) {
       // Add new `@use` rule for indirect dependency
       var simplePath = _absoluteUrlToDependency(url);

diff --git a/lib/src/migrators/module/references.dart b/lib/src/migrators/module/references.dart
@@ -21,14 +21,8 @@ import '../../exception.dart';
 import '../../util/bidirectional_map.dart';
 import '../../util/unmodifiable_bidirectional_map_view.dart';
 import '../../utils.dart';
-import 'member_declaration.dart';
-import 'scope.dart';
-
-import 'built_in_functions.dart';
-import 'reference_source.dart';
-import 'scope.dart';
-
 import 'built_in_functions.dart';
+import 'member_declaration.dart';
 import 'reference_source.dart';
 import 'scope.dart';
 
@@ -86,6 +80,10 @@ class References {
   /// scope of a stylesheet.
   final Set<MemberDeclaration> globalDeclarations;
 
+  /// An unmodifiable map from member declarations to the library URLs those
+  /// members can be loaded from.
+  final Map<MemberDeclaration, Set<Uri>> libraries;
+
   /// A mapping from member references to their source.
   ///
   /// This includes references to built-in functions, but it does not include
@@ -138,6 +136,7 @@ class References {
       BidirectionalMap<FunctionExpression, MemberDeclaration<FunctionRule>>
           getFunctionReferences,
       Set<MemberDeclaration> globalDeclarations,
+      Map<MemberDeclaration, Set<Uri>> libraries,
       Map<SassNode, ReferenceSource> sources)
       : variables = UnmodifiableBidirectionalMapView(variables),
         variableReassignments =
@@ -149,6 +148,8 @@ class References {
         getFunctionReferences =
             UnmodifiableBidirectionalMapView(getFunctionReferences),
         globalDeclarations = UnmodifiableSetView(globalDeclarations),
+        libraries = UnmodifiableMapView(
+            mapMap(libraries, value: (_, urls) => UnmodifiableSetView(urls))),
         sources = UnmodifiableMapView(sources);
 
   /// Constructs a new [References] object based on a [stylesheet] (imported by
@@ -173,6 +174,7 @@ class _ReferenceVisitor extends RecursiveAstVisitor {
   final _getFunctionReferences =
       BidirectionalMap<FunctionExpression, MemberDeclaration<FunctionRule>>();
   final _globalDeclarations = <MemberDeclaration>{};
+  final _libraries = <MemberDeclaration, Set<Uri>>{};
   final _sources = <SassNode, ReferenceSource>{};
 
   /// The current global scope.
@@ -209,7 +211,15 @@ class _ReferenceVisitor extends RecursiveAstVisitor {
   /// It doesn't include namespaces for to-be-migrated imports.
   Map<String, Uri> _namespaces;
 
-  /// URL of the stylesheet currently being migrated.
+  /// The URL of the root of the current library being visited.
+  ///
+  /// For stylesheets imported relative to the entrypoint, this is `null`. For
+  /// stylesheets loaded from a load path or from `node_modules`, this is the
+  /// canonical URL of the last import that was handled with a different
+  /// importer than the entrypoint's.
+  Uri _libraryUrl;
+
+  /// The URL of the stylesheet currently being migrated.
   Uri _currentUrl;
 
   /// The importer that's currently being used to resolve relative imports.
@@ -242,6 +252,7 @@ class _ReferenceVisitor extends RecursiveAstVisitor {
         _functions,
         _getFunctionReferences,
         _globalDeclarations,
+        _libraries,
         _sources);
   }
 
@@ -311,8 +322,11 @@ class _ReferenceVisitor extends RecursiveAstVisitor {
 
         var oldImporter = _importer;
         _importer = result.item1;
-        visitStylesheet(result.item2);
+        var oldLibraryUrl = _libraryUrl;
         var url = result.item2.span.sourceUrl;
+        if (_importer != oldImporter) _libraryUrl ??= url;
+
+        visitStylesheet(result.item2);
         var importSource = ImportSource(url, import);
         for (var declaration in _declarationSources.keys.toList()) {
           var source = _declarationSources[declaration];
@@ -322,6 +336,9 @@ class _ReferenceVisitor extends RecursiveAstVisitor {
           }
           _importer = oldImporter;
         }
+
+        _libraryUrl = oldLibraryUrl;
+        _importer = oldImporter;
       }
     }
   }
@@ -362,21 +379,24 @@ class _ReferenceVisitor extends RecursiveAstVisitor {
 
     var stylesheet = result.item2;
     var canonicalUrl = stylesheet.span.sourceUrl;
-    if (!_moduleScopes.containsKey(canonicalUrl)) {
-      var oldScope = _scope;
-      _scope = Scope();
-      _moduleScopes[canonicalUrl] = _scope;
-      var oldSources = _declarationSources;
-      _declarationSources = {};
-      _moduleSources[canonicalUrl] = _declarationSources;
-      var oldImporter = _importer;
-      _importer = result.item1;
-      visitStylesheet(stylesheet);
-      _checkUnresolvedReferences(_scope);
-      _importer = oldImporter;
-      _scope = oldScope;
-      _declarationSources = oldSources;
-    }
+    if (_moduleScopes.containsKey(canonicalUrl)) return canonicalUrl;
+
+    var oldScope = _scope;
+    _scope = Scope();
+    _moduleScopes[canonicalUrl] = _scope;
+    var oldSources = _declarationSources;
+    _declarationSources = {};
+    _moduleSources[canonicalUrl] = _declarationSources;
+    var oldImporter = _importer;
+    _importer = result.item1;
+    var oldLibraryUrl = _libraryUrl;
+    _libraryUrl = null;
+    visitStylesheet(stylesheet);
+    _checkUnresolvedReferences(_scope);
+    _libraryUrl = oldLibraryUrl;
+    _importer = oldImporter;
+    _scope = oldScope;
+    _declarationSources = oldSources;
     return canonicalUrl;
   }
 
@@ -426,6 +446,7 @@ class _ReferenceVisitor extends RecursiveAstVisitor {
       Map<String, MemberDeclaration<T>> declarations) {
     var declaration =
         MemberDeclaration<T>.forward(forwarding, forward, forwardedUrl);
+    _registerLibraryUrl(declaration);
     var prefix = forward.prefix ?? '';
     declarations['$prefix${forwarding.name}'] = declaration;
     _declarationSources[declaration] =
@@ -524,6 +545,7 @@ class _ReferenceVisitor extends RecursiveAstVisitor {
     super.visitVariableDeclaration(node);
     var member = MemberDeclaration(node);
     _declarationSources[member] = CurrentSource(_currentUrl);
+    _registerLibraryUrl(member);
 
     var scope = _scopeForNamespace(node.namespace);
     if (node.isGlobal) scope = scope.global;
@@ -564,6 +586,7 @@ class _ReferenceVisitor extends RecursiveAstVisitor {
     super.visitMixinRule(node);
     var member = MemberDeclaration(node);
     _declarationSources[member] = CurrentSource(_currentUrl);
+    _registerLibraryUrl(member);
     _scope.mixins[node.name] = member;
     if (_scope.isGlobal) _globalDeclarations.add(member);
   }
@@ -591,6 +614,7 @@ class _ReferenceVisitor extends RecursiveAstVisitor {
     super.visitFunctionRule(node);
     var member = MemberDeclaration(node);
     _declarationSources[member] = CurrentSource(_currentUrl);
+    _registerLibraryUrl(member);
     _scope.functions[node.name] = member;
     if (_scope.isGlobal) _globalDeclarations.add(member);
   }
@@ -634,6 +658,13 @@ class _ReferenceVisitor extends RecursiveAstVisitor {
     }
   }
 
+  /// Registers the current library as a location from which [declaration] can
+  /// be loaded.
+  void _registerLibraryUrl(MemberDeclaration<SassNode> declaration) {
+    if (_libraryUrl == null) return;
+    _libraries.putIfAbsent(declaration, () => {}).add(_libraryUrl);
+  }
+
   /// Returns true if [declaration] is from a `@forward` rule in the current
   /// stylesheet.
   bool _fromForwardRuleInCurrent(MemberDeclaration declaration) =>

diff --git a/test/migrators/module/doesnt_migrate_through_load_path.hrx b/test/migrators/module/doesnt_migrate_through_load_path.hrx
@@ -15,9 +15,8 @@ a {
 $variable: green;
 
 <==> output/entrypoint.scss
-@use "other";
 @use "dependency";
 
 a {
-  color: other.$variable;
+  color: dependency.$variable;
 }
diff --git a/test/migrators/module/doesnt_migrate_through_node_modules.hrx b/test/migrators/module/doesnt_migrate_through_node_modules.hrx
@@ -15,9 +15,8 @@ a {
 $variable: green;
 
 <==> output/entrypoint.scss
-@use "~module/other";
 @use "~module/dependency";
 
 a {
-  color: other.$variable;
+  color: dependency.$variable;
 }
diff --git a/test/migrators/module/load_path_indirect.hrx b/test/migrators/module/load_path_indirect.hrx
@@ -18,8 +18,7 @@ a {
 $variable: blue;
 
 <==> output/entrypoint.scss
-@use "indirect";
 @use "direct";
 a {
-  color: indirect.$variable;
+  color: direct.$variable;
 }
diff --git a/test/migrators/module/loads_indirect_member_from_library_entrypoint.hrx b/test/migrators/module/loads_indirect_member_from_library_entrypoint.hrx
@@ -0,0 +1,33 @@
+<==> arguments
+--migrate-deps
+
+<==> README
+We have to generate a new `@use` for the variable reference in `_other.scss`. We
+should generate one that uses the original dependency URL, rather than the
+direct URL to the file in which the name is defined, since that file structure
+is likely not to be part of the library's public API.
+
+<==> input/entrypoint.scss
+@import "~module/dependency";
+@import "other";
+
+<==> input/_other.scss
+a {
+  color: $variable;
+}
+
+<==> input/node_modules/module/_dependency.scss
+@import "other";
+
+<==> input/node_modules/module/_other.scss
+$variable: green;
+
+<==> output/entrypoint.scss
+@use "~module/dependency";
+@use "other";
+
+<==> output/_other.scss
+@use "~module/dependency";
+a {
+  color: dependency.$variable;
+}