Support --forward=prefixed across migrator runs

CHANGELOG.md

@@ -2,6 +2,13 @@
 
 * Add support for glob inputs on the command line.
 
+### Module Migrator
+
+* Make `--remove-prefix=<prefix> --forward=prefixed` forward members that
+  previously started with `<prefix>` and were unprefixed by a previous migrator
+  run. This includes cases where the previously removed prefix is longer than
+  the prefix for the current migrator run.
+
 ## 1.0.1
 
 ### Module Migrator

lib/src/migrators/module.dart

@@ -98,8 +98,8 @@ class ModuleMigrator extends Migrator {
             (argResults['remove-prefix'] as String)?.replaceAll('_', '-'),
         forward: forward);
     var migrated = visitor.run(stylesheet, importer);
-    _filesWithRenamedDeclarations.addAll(visitor.renamedMembers.keys
-        .map((declaration) => declaration.sourceUrl));
+    _filesWithRenamedDeclarations.addAll(
+        {for (var member in visitor.renamedMembers.keys) member.sourceUrl});
     return migrated;
   }
 }
@@ -162,6 +162,12 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
   /// Whether @use and @forward are allowed in the current context.
   var _useAllowed = true;
 
+  /// Whether an import-only stylesheet should be generated.
+  ///
+  /// This will be set to true if [prefixToRemove] is removed from any member
+  /// visible at the entrypoint.
+  var _needsImportOnly = false;
+
   /// Set of variables declared outside the current stylesheet that overrode
   /// `!default` variables within the current stylesheet.
   Set<MemberDeclaration<VariableDeclaration>> _configuredVariables;
@@ -210,9 +216,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
     references.globalDeclarations.forEach(_renameDeclaration);
     var migrated = super.run(stylesheet, importer);
 
-    // If a prefix was removed from any members, add an import-only stylesheet
-    // that forwards the entrypoint with that prefix.
-    if (prefixToRemove != null && renamedMembers.isNotEmpty) {
+    if (prefixToRemove != null && _needsImportOnly) {
       var url = stylesheet.span.sourceUrl;
       var importOnlyUrl = getImportOnlyUrl(url);
       var tuple = _absoluteUrlToDependency(url, relativeTo: importOnlyUrl);
@@ -266,6 +270,8 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
   /// with `-` or `_` and are referenced outside the stylesheet they were
   /// declared in.
   void _renameDeclaration(MemberDeclaration declaration) {
+    if (declaration.forward != null) return;
+
     var name = declaration.name;
     if (name.startsWith('-') &&
         references.referencedOutsideDeclaringStylesheet(declaration)) {
@@ -274,7 +280,10 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
       name = name.substring(1);
     }
     name = _unprefix(name);
-    if (name != declaration.name) renamedMembers[declaration] = name;
+    if (name != declaration.name) {
+      renamedMembers[declaration] = name;
+      if (_upstreamStylesheets.isEmpty) _needsImportOnly = true;
+    }
   }
 
   /// Returns a semicolon unless the current stylesheet uses the indented
@@ -308,13 +317,15 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
     var loadPathForwards = <String>[];
     var relativeForwards = <String>[];
     for (var url in references.globalDeclarations
-        .map((declaration) => declaration.sourceUrl)
+        .map((declaration) => declaration.isImportOnly
+            ? declaration.forwardedUrl
+            : declaration.sourceUrl)
         .toSet()) {
       if (url == currentUrl || _forwardedUrls.contains(url)) continue;
       var tuple = _makeForwardRule(url);
       if (tuple == null) continue;
-      (tuple.item2 ? relativeForwards : loadPathForwards)
-          .add('${tuple.item1}$_semicolonIfNotIndented\n');
+      (tuple.item2 ? relativeForwards : loadPathForwards).addAll(
+          [for (var rule in tuple.item1) '$rule$_semicolonIfNotIndented\n']);
     }
     var forwards = [...loadPathForwards..sort(), ...relativeForwards..sort()];
     return forwards.isEmpty ? '' : '\n' + forwards.join('');
@@ -838,7 +849,8 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
         var tuple = _makeForwardRule(resolvedUrl);
         if (tuple != null) {
           _forwardedUrls.add(resolvedUrl);
-          addPatch(Patch.insert(importStart, tuple.item1));
+          addPatch(Patch.insert(
+              importStart, tuple.item1.join('$_semicolonIfNotIndented\n')));
           return;
         }
       }
@@ -849,38 +861,69 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
   }
 
   /// If [url] contains any member declarations that should be forwarded from
-  /// the entrypoint, returns a tuple of the `@forward` rule that should be used
-  /// and a boolean that is true when the URL of this rule is relative.
+  /// the entrypoint, returns a tuple of the `@forward` rule(s) that should be
+  /// used and a boolean that is true when the URL of these rules is relative.
   ///
   /// If nothing from [url] should be forwarded, returns null.
-  Tuple2<String, bool> _makeForwardRule(Uri url) {
-    var shown = <String>{};
+  Tuple2<List<String>, bool> _makeForwardRule(Uri url) {
+    var shownByPrefix = <String, Set<String>>{};
     var hidden = <String>{};
 
-    // Divide all global members from dependencies into sets based on whether
-    // they should be forwarded or not.
+    // Divide all global members from dependencies into sets based on their
+    // subprefix (if any) and whether they should be forwarded or not.
     for (var declaration in references.globalDeclarations) {
-      if (declaration.sourceUrl != url) continue;
+      var expectedUrl = declaration.isImportOnly
+          ? declaration.forwardedUrl
+          : declaration.sourceUrl;
+      if (expectedUrl != url) continue;
 
       var newName = renamedMembers[declaration] ?? declaration.name;
-      if (declaration.member is VariableDeclaration) newName = "\$$newName";
-
+      String importOnlyPrefix;
+      if (declaration.isImportOnly && declaration.forward.prefix != null) {
+        importOnlyPrefix = declaration.forward.prefix;
+        newName = declaration.name.substring(importOnlyPrefix.length);
+      }
+      var formattedNewName =
+          declaration.member is VariableDeclaration ? '\$$newName' : newName;
       if (_shouldForward(declaration.name) &&
           !declaration.name.startsWith('-')) {
-        shown.add(newName);
-      } else if (!newName.startsWith('-') && !newName.startsWith(r'$-')) {
-        hidden.add(newName);
+        var subprefix = "";
+        if (prefixToRemove != null &&
+            declaration.name.startsWith(prefixToRemove) &&
+            importOnlyPrefix != null) {
+          subprefix = importOnlyPrefix.substring(prefixToRemove.length);
+        }
+        if (declaration.name != newName) _needsImportOnly = true;
+        shownByPrefix.putIfAbsent(subprefix, () => {}).add(formattedNewName);
+      } else if (!newName.startsWith('-')) {
+        hidden.add(formattedNewName);
       }
     }
-    if (shown.isEmpty) return null;
-
+    if (shownByPrefix.isEmpty) return null;
     var tuple = _absoluteUrlToDependency(url);
-    var forward = '@forward "${tuple.item1}"';
-    if (hidden.isNotEmpty) {
-      var hiddenMembers = hidden.toList()..sort();
-      forward += ' hide ${hiddenMembers.join(", ")}';
+    var forwardBase = '@forward "${tuple.item1}"';
+    var forwards = <String>[];
+    for (var subprefix in shownByPrefix.keys.toList()..sort()) {
+      var allHidden = {
+        ...hidden,
+        for (var other in shownByPrefix.keys)
+          if (other != subprefix) ...shownByPrefix[other]
+      }.toList()
+        ..sort();
+      var forward = forwardBase;
+      if (subprefix.isNotEmpty) {
+        forward += ' as $subprefix*';
+        allHidden = [
+          for (var item in allHidden)
+            item.startsWith(r'$')
+                ? '\$$subprefix${item.substring(1)}'
+                : '$subprefix$item'
+        ];
+      }
+      if (allHidden.isNotEmpty) forward += ' hide ${allHidden.join(", ")}';
+      forwards.add(forward);
     }
-    return Tuple2(forward, tuple.item2);
+    return Tuple2(forwards, tuple.item2);
   }
 
   /// Adds a namespace to any mixin include that requires it.
@@ -977,26 +1020,11 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
       return;
     }
 
-    if (_isPrefixedImportOnly(declaration)) {
+    if (declaration.isImportOnly && declaration.forward?.prefix != null) {
       addPatch(patchDelete(span, end: declaration.forward.prefix.length));
     }
   }
 
-  /// Returns true if [declaration] was forwarded from a regular stylesheet by
-  /// an import-only stylesheet of the same name.
-  bool _isPrefixedImportOnly(MemberDeclaration declaration) {
-    if (declaration.forward?.prefix == null) return false;
-    var containing = declaration.sourceUrl.toString();
-    var forwarded = declaration.forwardedUrl.toString();
-    if (!p.url.equals(p.url.dirname(containing), p.url.dirname(forwarded))) {
-      return false;
-    }
-    return p.url.basename(containing) ==
-        p.url.withoutExtension(p.url.basename(forwarded)) +
-            ".import" +
-            p.url.extension(containing);
-  }
-
   /// Returns [name] with [prefixToRemove] removed.
   String _unprefix(String name) {
     if (prefixToRemove == null || prefixToRemove.length > name.length) {

lib/src/migrators/module/member_declaration.dart

@@ -9,6 +9,8 @@
 // https://github.com/sass/dart-sass/issues/236.
 import 'package:sass/src/ast/sass.dart';
 
+import '../../utils.dart';
+
 /// A wrapper class for nodes that declare a variable, function, or mixin.
 ///
 /// The member this class wraps will always be a [VariableDeclaration],
@@ -62,6 +64,12 @@ class MemberDeclaration<T extends SassNode> {
         name = '${forward.prefix ?? ""}${forwarding.name}',
         sourceUrl = forward.span.sourceUrl;
 
+  /// Returns true if this declaration is the result of a `@forward` rule within
+  /// an import-only stylesheet.
+  bool get isImportOnly =>
+      forward != null &&
+      forward.span.sourceUrl == getImportOnlyUrl(forwardedUrl);
+
   operator ==(other) =>
       other is MemberDeclaration &&
       member == other.member &&

lib/src/migrators/module/references.dart

@@ -255,6 +255,9 @@ class _ReferenceVisitor extends RecursiveAstVisitor {
     _declarationSources = {};
     _moduleSources[stylesheet.span.sourceUrl] = _declarationSources;
     visitStylesheet(stylesheet);
+    _globalDeclarations.addAll(_scope.variables.values);
+    _globalDeclarations.addAll(_scope.mixins.values);
+    _globalDeclarations.addAll(_scope.functions.values);
     _checkUnresolvedReferences(_scope);
     _resolveBuiltInFunctionReferences();
     return References._(
@@ -425,30 +428,33 @@ class _ReferenceVisitor extends RecursiveAstVisitor {
         throw StateError(
             "Arguments should not be present in a module's global scope");
       }
-      if (_visibleThroughForward(
-          declaration.name, node.shownVariables, node.hiddenVariables)) {
+      if (_visibleThroughForward(declaration.name, node.prefix,
+          node.shownVariables, node.hiddenVariables)) {
         _forwardMember(declaration, node, canonicalUrl, _scope.variables);
       }
     }
     for (var declaration in moduleScope.mixins.values) {
-      if (_visibleThroughForward(declaration.name, node.shownMixinsAndFunctions,
-          node.hiddenMixinsAndFunctions)) {
+      if (_visibleThroughForward(declaration.name, node.prefix,
+          node.shownMixinsAndFunctions, node.hiddenMixinsAndFunctions)) {
         _forwardMember(declaration, node, canonicalUrl, _scope.mixins);
       }
     }
     for (var declaration in moduleScope.functions.values) {
-      if (_visibleThroughForward(declaration.name, node.shownMixinsAndFunctions,
-          node.hiddenMixinsAndFunctions)) {
+      if (_visibleThroughForward(declaration.name, node.prefix,
+          node.shownMixinsAndFunctions, node.hiddenMixinsAndFunctions)) {
         _forwardMember(declaration, node, canonicalUrl, _scope.functions);
       }
     }
   }
 
-  /// Returns true if [name] should be shown based on [shown] and [hidden] from
-  /// a `@forward` rule.
+  /// Returns true if [name] should be shown based on [prefix], [shown], and
+  /// [hidden] from a `@forward` rule.
   bool _visibleThroughForward(
-          String name, Set<String> shown, Set<String> hidden) =>
-      (shown?.contains(name) ?? true) && !(hidden?.contains(name) ?? false);
+      String name, String prefix, Set<String> shown, Set<String> hidden) {
+    if (prefix != null) name = '$prefix$name';
+    return (shown?.contains(name) ?? true) &&
+        !(hidden?.contains(name) ?? false);
+  }
 
   /// Forwards [forwarding] into [declarations], adding the forwarded
   /// declaration to [_declarationSources].
@@ -572,7 +578,6 @@ class _ReferenceVisitor extends RecursiveAstVisitor {
     var previous = scope.variables[node.name];
     if (previous == node) return;
     scope.variables[node.name] = member;
-    if (scope.isGlobal) _globalDeclarations.add(member);
     var original = _variableReassignments[previous] ?? previous;
     if (original != null) _variableReassignments[member] = original;
   }
@@ -601,7 +606,6 @@ class _ReferenceVisitor extends RecursiveAstVisitor {
     _declarationSources[member] = CurrentSource(_currentUrl);
     _registerLibraryUrl(member);
     _scope.mixins[node.name] = member;
-    if (_scope.isGlobal) _globalDeclarations.add(member);
   }
 
   /// Visits an `@include` rule, storing the mixin reference.
@@ -629,7 +633,6 @@ class _ReferenceVisitor extends RecursiveAstVisitor {
     _declarationSources[member] = CurrentSource(_currentUrl);
     _registerLibraryUrl(member);
     _scope.functions[node.name] = member;
-    if (_scope.isGlobal) _globalDeclarations.add(member);
   }
 
   /// Visits a function call, storing it if it is a user-defined function.

pubspec.lock

@@ -93,12 +93,12 @@ packages:
     source: hosted
     version: "0.1.26"
   glob:
-    dependency: transitive
+    dependency: "direct main"
     description:
       name: glob
       url: "https://pub.dartlang.org"
     source: hosted
-    version: "1.1.7"
+    version: "1.2.0"
   grinder:
     dependency: "direct dev"
     description:

test/migrators/module/partial_migration/forward_already_unprefixed.hrx

@@ -0,0 +1,27 @@
+<==> arguments
+--migrate-deps --remove-prefix=lib- --forward=prefixed
+
+<==> input/entrypoint.scss
+@import "library";
+
+a {
+  color: $lib-variable;
+}
+
+<==> input/_library.scss
+$variable: green;
+
+<==> input/_library.import.scss
+@forward "library" as lib-*;
+
+<==> output/entrypoint.scss
+@use "library";
+
+@forward "library";
+
+a {
+  color: library.$variable;
+}
+
+<==> output/entrypoint.import.scss
+@forward "entrypoint" as lib-*;

test/migrators/module/partial_migration/remove_different_prefix.hrx

@@ -0,0 +1,31 @@
+<==> arguments
+--migrate-deps --remove-prefix=prefix- --forward=prefixed
+
+<==> input/entrypoint.scss
+@import "library";
+
+$prefix-variable: blue;
+
+a {
+  color: $lib-variable;
+  background: $prefix-variable;
+}
+
+<==> input/_library.scss
+$variable: green;
+
+<==> input/_library.import.scss
+@forward "library" as lib-*;
+
+<==> output/entrypoint.scss
+@use "library";
+
+$variable: blue;
+
+a {
+  color: library.$variable;
+  background: $variable;
+}
+
+<==> output/entrypoint.import.scss
+@forward "entrypoint" as prefix-*;