Cleanup

Author: jathak

lib/src/migrators/module.dart

@@ -24,25 +24,13 @@ class ModuleMigrator extends Migrator {
   final name = "module";
   final description = "Migrates stylesheets to the new module system.";
 
-  /// Global variables defined at any time during the migrator run.
-  final _variables = normalizedMap<List<VariableDeclaration>>();
-
-  /// Global mixins defined at any time during the migrator run.
-  final _mixins = normalizedMap<MixinRule>();
-
-  /// Global functions defined at any time during the migrator run.
-  final _functions = normalizedMap<FunctionRule>();
-
-  /// Global variables declared with !default that could be configured.
-  ///final _configurableVariables = normalizedMap<VariableDeclaration>();
-
   /// Runs the module migrator on [entrypoint] and its dependencies and returns
   /// a map of migrated contents.
   ///
   /// If [migrateDependencies] is false, the migrator will still be run on
   /// dependencies, but they will be excluded from the resulting map.
   Map<Uri, String> migrateFile(Uri entrypoint) {
-    var migrated = _ModuleMigrationVisitor(this).run(entrypoint);
+    var migrated = _ModuleMigrationVisitor().run(entrypoint);
     if (!migrateDependencies) {
       migrated.removeWhere((url, contents) => url != entrypoint);
     }
@@ -51,7 +39,17 @@ class ModuleMigrator extends Migrator {
 }
 
 class _ModuleMigrationVisitor extends MigrationVisitor {
-  final ModuleMigrator migrator;
+  /// Global variables defined at any time during the migrator run.
+  ///
+  /// We store all declarations instead of just the most recent one for use in
+  /// detecting configurable variables.
+  final _globalVariables = normalizedMap<List<VariableDeclaration>>();
+
+  /// Global mixins defined at any time during the migrator run.
+  final _globalMixins = normalizedMap<MixinRule>();
+
+  /// Global functions defined at any time during the migrator run.
+  final _globalFunctions = normalizedMap<FunctionRule>();
 
   /// Namespaces of modules used in this stylesheet.
   Map<Uri, String> _namespaces = {};
@@ -79,7 +77,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
   /// Note: We always set [migratedDependencies] to true since the module
   /// migrator needs to always run on dependencies. The `migrateFile` method of
   /// the module migrator will filter out the dependencies' migration results.
-  _ModuleMigrationVisitor(this.migrator) : super(migrateDependencies: true);
+  _ModuleMigrationVisitor() : super(migrateDependencies: true);
 
   /// Returns the migrated contents of this stylesheet, based on [patches] and
   /// [_additionalUseRules], or null if the stylesheet does not change.
@@ -127,7 +125,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
   @override
   void visitFunctionExpression(FunctionExpression node) {
     visitInterpolation(node.name);
-    patchNamespaceForFunction(node.name.asPlain, (name, namespace) {
+    _patchNamespaceForFunction(node.name.asPlain, (name, namespace) {
       addPatch(Patch(node.name.span, "$namespace.$name"));
     });
     visitArgumentInvocation(node.arguments);
@@ -142,7 +140,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
         return;
       }
       var fnName = nameArgument as StringExpression;
-      patchNamespaceForFunction(fnName.text.asPlain, (name, namespace) {
+      _patchNamespaceForFunction(fnName.text.asPlain, (name, namespace) {
         var span = fnName.span;
         if (fnName.hasQuotes) {
           span = span.file.span(span.start.offset + 1, span.end.offset - 1);
@@ -162,13 +160,13 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
   /// when namespaced, and the namespace itself. The name will match the name
   /// provided to the outer function except for built-in functions whose name
   /// within a module differs from its original name.
-  void patchNamespaceForFunction(
+  void _patchNamespaceForFunction(
       String name, void patcher(String name, String namespace)) {
     if (name == null) return;
     if (_localScope?.isLocalFunction(name) ?? false) return;
 
-    var namespace = migrator._functions.containsKey(name)
-        ? namespaceForNode(migrator._functions[name])
+    var namespace = _globalFunctions.containsKey(name)
+        ? _namespaceForNode(_globalFunctions[name])
         : null;
 
     if (namespace == null) {
@@ -183,7 +181,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
   /// Declares the function within the current scope before visiting it.
   @override
   void visitFunctionRule(FunctionRule node) {
-    declareFunction(node);
+    _declareFunction(node);
     super.visitFunctionRule(node);
   }
 
@@ -209,19 +207,33 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
     visitDependency(Uri.parse(import.url), _currentUrl);
     _namespaces[_lastUrl] = namespaceForPath(import.url);
 
-    var overrides = [];
-    for (var name in migrator._variables.keys) {
-      var declarations = migrator._variables[name];
+    var configured = _findConfiguredVariables(node, import);
+    var config = "";
+    if (configured.isNotEmpty) {
+      config = " with (\n  " + configured.join(',\n  ') + "\n)";
+    }
+    addPatch(Patch(node.span, '@use ${import.span.text}$config'));
+  }
+
+  /// Finds all configured variables from the given import.
+  ///
+  /// This returns a list of "$var: <expression>" strings that can be used to
+  /// construct the configuration for a use rule.
+  ///
+  /// If a variable was configured in a downstream stylesheet, this will instead
+  /// add a forward rule to make it accessible.
+  List<String> _findConfiguredVariables(ImportRule node, Import import) {
+    var configured = <String>[];
+    for (var name in _globalVariables.keys) {
+      var declarations = _globalVariables[name];
       VariableDeclaration lastNonDefault = declarations[0];
       for (var i = 1; i < declarations.length; i++) {
         var declaration = declarations[i];
         if (!declaration.isGuarded) {
           lastNonDefault = declaration;
         } else if (declaration.span.sourceUrl == _lastUrl) {
           if (lastNonDefault.span.sourceUrl == _currentUrl) {
-            // This configurable variable was set within the current stylesheet,
-            // so add to overrides.
-            overrides.add("\$$name: ${lastNonDefault.expression}");
+            configured.add("\$$name: ${lastNonDefault.expression}");
           } else if (lastNonDefault.span.sourceUrl != _lastUrl) {
             // A downstream stylesheet configures this variable, so forward it.
             var semicolon = _currentUrl.path.endsWith('.sass') ? '' : ';';
@@ -235,20 +247,16 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
         }
       }
     }
-    var config = "";
-    if (overrides.isNotEmpty) {
-      config = " with (\n  " + overrides.join(',\n  ') + "\n)";
-    }
-    addPatch(Patch(node.span, '@use ${import.span.text}$config'));
+    return configured;
   }
 
   /// Adds a namespace to any mixin include that requires it.
   @override
   void visitIncludeRule(IncludeRule node) {
     super.visitIncludeRule(node);
     if (_localScope?.isLocalMixin(node.name) ?? false) return;
-    if (!migrator._mixins.containsKey(node.name)) return;
-    var namespace = namespaceForNode(migrator._mixins[node.name]);
+    if (!_globalMixins.containsKey(node.name)) return;
+    var namespace = _namespaceForNode(_globalMixins[node.name]);
     if (namespace == null) return;
     var endName = node.arguments.span.start.offset;
     var startName = endName - node.name.length;
@@ -259,7 +267,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
   /// Declares the mixin within the current scope before visiting it.
   @override
   void visitMixinRule(MixinRule node) {
-    declareMixin(node);
+    _declareMixin(node);
     super.visitMixinRule(node);
   }
 
@@ -276,63 +284,59 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
     if (_localScope?.isLocalVariable(node.name) ?? false) {
       return;
     }
-    if (!migrator._variables.containsKey(node.name)) return;
-    var lastRealDeclaration = migrator._variables[node.name]
+    if (!_globalVariables.containsKey(node.name)) return;
+
+    /// Declarations where the expression is null are fake ones used to track
+    /// configured variables within indirect dependencies.
+    /// See [_findConfiguredVariables].
+    var lastRealDeclaration = _globalVariables[node.name]
         .lastWhere((node) => node.expression != null);
-    var namespace = namespaceForNode(lastRealDeclaration);
+    var namespace = _namespaceForNode(lastRealDeclaration);
     if (namespace == null) return;
     addPatch(Patch(node.span, "\$$namespace.${node.name}"));
   }
 
   /// Declares a variable within the current scope before visiting it.
   @override
   void visitVariableDeclaration(VariableDeclaration node) {
-    declareVariable(node);
+    _declareVariable(node);
     super.visitVariableDeclaration(node);
   }
 
   /// Declares a variable within this stylesheet, in the current local scope if
   /// it exists, or as a global variable otherwise.
-  void declareVariable(VariableDeclaration node) {
+  void _declareVariable(VariableDeclaration node) {
     if (_localScope == null || node.isGlobal) {
-      /*if (node.isGuarded) {
-        migrator._configurableVariables[node.name] = node;
-
-        // Don't override if variable already exists.
-        migrator._variables.putIfAbsent(node.name, () => node);
-      } else {
-        migrator._variables[node.name] = node;
-      }*/
-      migrator._variables.putIfAbsent(node.name, () => []);
-      migrator._variables[node.name].add(node);
+      _globalVariables.putIfAbsent(node.name, () => []);
+      _globalVariables[node.name].add(node);
     } else {
       _localScope.variables.add(node.name);
     }
   }
 
   /// Declares a mixin within this stylesheet, in the current local scope if
   /// it exists, or as a global mixin otherwise.
-  void declareMixin(MixinRule node) {
+  void _declareMixin(MixinRule node) {
     if (_localScope == null) {
-      migrator._mixins[node.name] = node;
+      _globalMixins[node.name] = node;
     } else {
       _localScope.mixins.add(node.name);
     }
   }
 
   /// Declares a function within this stylesheet, in the current local scope if
   /// it exists, or as a global function otherwise.
-  void declareFunction(FunctionRule node) {
+  void _declareFunction(FunctionRule node) {
     if (_localScope == null) {
-      migrator._functions[node.name] = node;
+      _globalFunctions[node.name] = node;
     } else {
       _localScope.functions.add(node.name);
     }
   }
 
   /// Finds the namespace for the stylesheet containing [node], adding a new use
   /// rule if necessary.
-  String namespaceForNode(SassNode node) {
+  String _namespaceForNode(SassNode node) {
     if (node.span.sourceUrl == _currentUrl) return null;
     if (!_namespaces.containsKey(node.span.sourceUrl)) {
       /// Add new use rule for indirect dependency

test/migrator_utils.dart

@@ -49,9 +49,10 @@ _testHrx(File hrxFile, String migrator) async {
             migrated = await MigratorRunner().run(arguments);
           }, getCurrentDirectory: () => Directory(d.sandbox)),
       prints(files.expectedLog?.replaceAll("\$TEST_DIR", d.sandbox) ?? ""));
-  var canonicalize = FilesystemImporter(d.sandbox).canonicalize;
+  var importer = FilesystemImporter(d.sandbox);
   for (var file in files.input.keys) {
-    expect(migrated[canonicalize(Uri.parse(file))], equals(files.output[file]),
+    expect(migrated[importer.canonicalize(Uri.parse(file))],
+        equals(files.output[file]),
         reason: 'Incorrect migration of $file.');
   }
 }