Reuse single MigrationVisitor + fix config vars

A single MigrationVisitor is now reused instead of constructing a new
instance for each dependency.

This should also fix the migration of configurable variables when the
variable being configured is in an indirect dependency.

Author: jathak

lib/runner.dart

@@ -7,6 +7,7 @@
 import 'dart:io';
 
 import 'package:args/command_runner.dart';
+import 'package:path/path.dart' as p;
 
 import 'src/migrators/module.dart';
 
@@ -43,7 +44,7 @@ class MigratorRunner extends CommandRunner<Map<Uri, String>> {
     if (argResults['dry-run']) {
       print('Dry run. Logging migrated files instead of overwriting...\n');
       for (var url in migrated.keys) {
-        print('$url');
+        print(p.prettyUri(url));
         if (argResults['verbose']) {
           print('=' * 80);
           print(migrated[url]);
@@ -52,9 +53,8 @@ class MigratorRunner extends CommandRunner<Map<Uri, String>> {
       }
     } else {
       for (var url in migrated.keys) {
-        if (url.scheme != null && url.scheme != "file") {
-          print("Cannot write to $url");
-        }
+        assert(url.scheme == null || url.scheme == "file",
+            "$url is not a file path.");
         if (argResults['verbose']) print("Overwriting $url...");
         File(url.toFilePath()).writeAsStringSync(migrated[url]);
       }

lib/src/migration_visitor.dart

@@ -28,44 +28,48 @@ import 'utils.dart';
 /// new instance of itself (using [newInstance]) each time it encounters an
 /// `@import` or `@use` rule.
 abstract class MigrationVisitor extends RecursiveAstVisitor {
-  /// The stylesheet being migrated.
-  final Stylesheet stylesheet;
-
   /// A mapping from URLs to migrated contents for stylesheets already migrated.
-  final Map<Uri, String> migrated;
+  final _migrated = <Uri, String>{};
 
   /// True if dependencies should be migrated as well.
   final bool migrateDependencies;
 
   /// The patches to be applied to the stylesheet being migrated.
   UnmodifiableListView<Patch> get patches => UnmodifiableListView(_patches);
-  final List<Patch> _patches = [];
-
-  /// Constructs a new migration visitor, parsing the stylesheet at [url].
-  MigrationVisitor(Uri url, this.migrateDependencies,
-      {Map<Uri, String> migrated})
-      : stylesheet = parseStylesheet(url),
-        this.migrated = migrated ?? {};
+  List<Patch> _patches = [];
 
-  /// Returns a new instance of this MigrationVisitor with the same [migrated]
-  /// and [migrateDependencies] and the new [url].
-  @protected
-  MigrationVisitor newInstance(Uri url);
+  MigrationVisitor({this.migrateDependencies = true});
 
-  /// Adds a new patch that should be applied as part of this migration.
-  @protected
-  void addPatch(Patch patch) {
-    _patches.add(patch);
+  /// Runs a new migration on [url] (and its dependencies, if
+  /// [migrateDependencies] is true) and returns a map of migrated contents.
+  Map<Uri, String> run(Uri url) {
+    visitStylesheet(parseStylesheet(url));
+    return _migrated;
   }
 
-  /// Runs the migrator and returns the map of migrated contents.
-  Map<Uri, String> run() {
-    visitStylesheet(stylesheet);
+  /// Visits stylesheet starting with an empty [_patches], adds the migrated
+  /// contents (if any) to [_migrated], and then restores the previous value of
+  /// [_patches].
+  ///
+  /// Migrators with per-file state should override this to store the current
+  /// file's state before calling the super method and restore it afterwards.
+  @override
+  void visitStylesheet(Stylesheet node) {
+    var oldPatches = _patches;
+    _patches = [];
+    super.visitStylesheet(node);
     var results = getMigratedContents();
     if (results != null) {
-      migrated[stylesheet.span.file.url] = results;
+      _migrated[node.span.sourceUrl] = results;
     }
-    return migrated;
+    _patches = oldPatches;
+  }
+
+  /// Visits the stylesheet at [dependency], resolved relative to [source].
+  @protected
+  void visitDependency(Uri dependency, Uri source) {
+    var stylesheet = parseStylesheet(source.resolveUri(dependency));
+    visitStylesheet(stylesheet);
   }
 
   /// Returns the migrated contents of this file, or null if the file does not
@@ -78,10 +82,11 @@ abstract class MigrationVisitor extends RecursiveAstVisitor {
       ? Patch.applyAll(patches.first.selection.file, patches)
       : null;
 
-  /// Resolves [relativeUrl] relative to the current stylesheet's URL.
+  /// Adds a new patch that should be applied as part of this migration.
   @protected
-  Uri resolveRelativeUrl(Uri relativeUrl) =>
-      stylesheet.span.file.url.resolveUri(relativeUrl);
+  void addPatch(Patch patch) {
+    _patches.add(patch);
+  }
 
   /// If [migrateDependencies] is enabled, any dynamic imports within
   /// this [node] will be migrated before continuing.
@@ -91,7 +96,7 @@ abstract class MigrationVisitor extends RecursiveAstVisitor {
     if (migrateDependencies) {
       for (var import in node.imports) {
         if (import is DynamicImport) {
-          newInstance(resolveRelativeUrl(Uri.parse(import.url))).run();
+          visitDependency(Uri.parse(import.url), node.span.sourceUrl);
         }
       }
     }
@@ -103,7 +108,7 @@ abstract class MigrationVisitor extends RecursiveAstVisitor {
   visitUseRule(UseRule node) {
     super.visitUseRule(node);
     if (migrateDependencies) {
-      newInstance(resolveRelativeUrl(node.url)).run();
+      visitDependency(node.url, node.span.sourceUrl);
     }
   }
 }

lib/src/migrators/module.dart

@@ -25,21 +25,24 @@ class ModuleMigrator extends Migrator {
   final description = "Migrates stylesheets to the new module system.";
 
   /// Global variables defined at any time during the migrator run.
-  final _variables = normalizedMap<VariableDeclaration>();
+  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, entrypoint).run();
+    var migrated = _ModuleMigrationVisitor(this).run(entrypoint);
     if (!migrateDependencies) {
       migrated.removeWhere((url, contents) => url != entrypoint);
     }
@@ -50,46 +53,61 @@ class ModuleMigrator extends Migrator {
 class _ModuleMigrationVisitor extends MigrationVisitor {
   final ModuleMigrator migrator;
 
-  /// Constructs a new module migration visitor.
-  ///
-  /// 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, Uri url, {Map<Uri, String> migrated})
-      : super(url, true, migrated: migrated);
-
-  _ModuleMigrationVisitor newInstance(Uri url) =>
-      _ModuleMigrationVisitor(migrator, url, migrated: migrated);
-
   /// Namespaces of modules used in this stylesheet.
-  final _namespaces = <Uri, String>{};
+  Map<Uri, String> _namespaces = {};
 
   /// Set of additional use rules necessary for referencing members of
   /// implicit dependencies / built-in modules.
   ///
   /// This set contains the path provided in the use rule, not the canonical
   /// path (e.g. "a" rather than "dir/a.scss").
-  final _additionalUseRules = Set<String>();
+  Set<String> _additionalUseRules = Set();
 
-  /// Global variables declared with !default that could be configured.
-  final _configurableVariables = normalizedSet();
+  /// The URL of the current stylesheet.
+  Uri _currentUrl;
+
+  /// The URL of the last stylesheet that was completely migrated.
+  Uri _lastUrl;
 
   /// Local variables, mixins, and functions for this migration.
   ///
   /// When at the top level of the stylesheet, this will be null.
   LocalScope _localScope;
 
+  /// Constructs a new module migration visitor.
+  ///
+  /// 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);
+
   /// Returns the migrated contents of this stylesheet, based on [patches] and
   /// [_additionalUseRules], or null if the stylesheet does not change.
   @override
   String getMigratedContents() {
     var results = super.getMigratedContents();
     if (results == null) return null;
-    var semicolon = stylesheet.span.file.url.path.endsWith('.sass') ? "" : ";";
+    var semicolon = _currentUrl.path.endsWith('.sass') ? "" : ";";
     var uses = _additionalUseRules.map((use) => '@use "$use"$semicolon\n');
     return uses.join("") + results;
   }
 
+  /// Stores per-file state before visiting [node] and restores it afterwards.
+  @override
+  void visitStylesheet(Stylesheet node) {
+    var oldNamespaces = _namespaces;
+    var oldAdditionalUseRules = _additionalUseRules;
+    var oldUrl = _currentUrl;
+    _namespaces = {};
+    _additionalUseRules = Set();
+    _currentUrl = node.span.sourceUrl;
+    super.visitStylesheet(node);
+    _namespaces = oldNamespaces;
+    _additionalUseRules = oldAdditionalUseRules;
+    _lastUrl = _currentUrl;
+    _currentUrl = oldUrl;
+  }
+
   /// Visits the children of [node] with a local scope.
   ///
   /// Note: The children of a stylesheet are at the root, so we should not add
@@ -187,20 +205,34 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
       return;
     }
     // TODO(jathak): Confirm that this import appears before other rules
-    var url = resolveRelativeUrl(Uri.parse(import.url));
-    var importMigration = newInstance(url)..run();
-    _namespaces[importMigration.stylesheet.span.sourceUrl] =
-        namespaceForPath(import.url);
+
+    visitDependency(Uri.parse(import.url), _currentUrl);
+    _namespaces[_lastUrl] = namespaceForPath(import.url);
 
     var overrides = [];
-    for (var variable in importMigration._configurableVariables) {
-      if (migrator._variables.containsKey(variable)) {
-        var declaration = migrator._variables[variable];
-        if (namespaceForNode(declaration) == null) {
-          overrides.add("\$${declaration.name}: ${declaration.expression}");
+    for (var name in migrator._variables.keys) {
+      var declarations = migrator._variables[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}");
+          } else if (lastNonDefault.span.sourceUrl != _lastUrl) {
+            // A downstream stylesheet configures this variable, so forward it.
+            var semicolon = _currentUrl.path.endsWith('.sass') ? '' : ';';
+            addPatch(patchBefore(
+                node, '@forward ${import.span.text} show \$$name$semicolon\n'));
+            // Add a fake variable declaration so that downstream stylesheets
+            // configure this module instead of the one we forwarded.
+            declarations.insert(i + 1,
+                VariableDeclaration(name, null, node.span, guarded: true));
+          }
         }
-        // TODO(jathak): Remove this declaration from the current stylesheet if
-        //   it's not referenced before this point.
       }
     }
     var config = "";
@@ -245,7 +277,9 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
       return;
     }
     if (!migrator._variables.containsKey(node.name)) return;
-    var namespace = namespaceForNode(migrator._variables[node.name]);
+    var lastRealDeclaration = migrator._variables[node.name]
+        .lastWhere((node) => node.expression != null);
+    var namespace = namespaceForNode(lastRealDeclaration);
     if (namespace == null) return;
     addPatch(Patch(node.span, "\$$namespace.${node.name}"));
   }
@@ -261,14 +295,16 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
   /// it exists, or as a global variable otherwise.
   void declareVariable(VariableDeclaration node) {
     if (_localScope == null || node.isGlobal) {
-      if (node.isGuarded) {
-        _configurableVariables.add(node.name);
+      /*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);
     } else {
       _localScope.variables.add(node.name);
     }
@@ -297,11 +333,11 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
   /// Finds the namespace for the stylesheet containing [node], adding a new use
   /// rule if necessary.
   String namespaceForNode(SassNode node) {
-    if (node.span.sourceUrl == stylesheet.span.sourceUrl) return null;
+    if (node.span.sourceUrl == _currentUrl) return null;
     if (!_namespaces.containsKey(node.span.sourceUrl)) {
       /// Add new use rule for indirect dependency
       var relativePath = p.relative(node.span.sourceUrl.path,
-          from: p.dirname(stylesheet.span.sourceUrl.path));
+          from: p.dirname(_currentUrl.path));
       var basename = p.basenameWithoutExtension(relativePath);
       if (basename.startsWith('_')) basename = basename.substring(1);
       var simplePath = p.relative(p.join(p.dirname(relativePath), basename));

test/migrators/module/indirect_configuration.hrx

@@ -0,0 +1,22 @@
+<==> arguments
+--migrate-deps
+
+<==> input/entrypoint.scss
+$config: red;
+@import "direct";
+
+<==> input/_direct.scss
+@import "indirect";
+
+<==> input/_indirect.scss
+$config: green !default;
+
+<==> output/entrypoint.scss
+$config: red;
+@use "direct" with (
+  $config: red
+);
+
+<==> output/_direct.scss
+@forward "indirect" show $config;
+@use "indirect";

test/migrators/module/indirect_configuration_with_reference.hrx

@@ -0,0 +1,30 @@
+<==> arguments
+--migrate-deps
+
+<==> input/entrypoint.scss
+$config: red;
+@import "direct";
+
+<==> input/_direct.scss
+@import "indirect";
+
+a {
+  b: $config;
+}
+
+<==> input/_indirect.scss
+$config: green !default;
+
+<==> output/entrypoint.scss
+$config: red;
+@use "direct" with (
+  $config: red
+);
+
+<==> output/_direct.scss
+@forward "indirect" show $config;
+@use "indirect";
+
+a {
+  b: $indirect.config;
+}