Refactor how @import rules are migrated (#172)

Fixes #170.

This refactors the migration of imports so that the entire rule is
migrated in one patch, as opposed to patching each import within a rule
separately. Migration for nested imports is also now factored out into
its own helper (`_migrateImportToLoadCss`).

As part of this refactor, I also made three behavior changes:

- For import-only files without corresponding regular files, the regular
  file they actually forward will now be treated as a replacement,
  allowing path-changes from import-only files to be migrated in place
  (and avoiding the issue of adjacent imports being removed that caused
  #170)

- Midstream files that both forward upstream variables for downstream
  files to configure and configure upstream variables themselves would
  previously be migrated to a broken result. These cases should now be
  migrated to a `@forward ... with` rule, which didn't exist the last
  time I updated the configurable variable code.

- `@import` rules that are migrated to both a `@use` and `@forward`
  rule would previously only migrate one in-place (adding the extra at
  after all existing rules). Now, both should be migrated in place.

Author: jathak

CHANGELOG.md

@@ -5,6 +5,18 @@
 * Fix a bug where generated import-only files for index files would contain
   invalid forwards.
 
+* Better handling for import-only files without corresponding regular files,
+  including fixing a crash when `@import` rules for two files like this are
+  adjacent to each other.
+
+* Midstream files that both forward configurable variables and configure other
+  variables themselves should now be properly migrated.
+
+* When an `@import` rule is migrated to both a `@use` rule and a `@forward`
+  rule, both rules will now be migrated in-place (previously, the `@use` rule
+  would replace the `@import` rule and the `@forward` rule would be added after
+  all other dependencies).
+
 ## 1.2.5
 
 ### Module Migrator

lib/src/migrators/module.dart

@@ -90,9 +90,10 @@ class References {
   /// map to the [ReferenceSource] for the `sass:meta` module).
   final Map<SassNode, ReferenceSource> sources;
 
-  /// The set of import-only files that do not directly depend on their regular
-  /// counterparts.
-  final Set<Uri> orphanImportOnlyFiles;
+  /// Map of import-only files that do not directly depend on their regular
+  /// counterparts to the last forward appearing within it (or null, if no
+  /// regular file is forwarded by the import-only file).
+  final Map<Uri, ForwardRule> orphanImportOnlyFiles;
 
   /// An iterable of all member declarations.
   Iterable<MemberDeclaration> get allDeclarations =>
@@ -154,7 +155,7 @@ class References {
       Set<MemberDeclaration> globalDeclarations,
       Map<MemberDeclaration, Set<Uri>> libraries,
       Map<SassNode, ReferenceSource> sources,
-      Set<Uri> orphanImportOnlyFiles)
+      Map<Uri, ForwardRule> orphanImportOnlyFiles)
       : variables = UnmodifiableBidirectionalMapView(variables),
         variableReassignments =
             UnmodifiableBidirectionalMapView(variableReassignments),
@@ -170,7 +171,7 @@ class References {
             entry.key: UnmodifiableSetView(entry.value)
         }),
         sources = UnmodifiableMapView(sources),
-        orphanImportOnlyFiles = UnmodifiableSetView(orphanImportOnlyFiles);
+        orphanImportOnlyFiles = UnmodifiableMapView(orphanImportOnlyFiles);
 
   /// Constructs a new [References] object based on a [stylesheet] (imported by
   /// [importer]) and its dependencies.
@@ -196,7 +197,7 @@ class _ReferenceVisitor extends RecursiveAstVisitor {
   final _globalDeclarations = <MemberDeclaration>{};
   final _libraries = <MemberDeclaration, Set<Uri>>{};
   final _sources = <SassNode, ReferenceSource>{};
-  final _orphanImportOnlyFiles = <Uri>{};
+  final _orphanImportOnlyFiles = <Uri, ForwardRule>{};
 
   /// The current global scope.
   ///
@@ -261,6 +262,9 @@ class _ReferenceVisitor extends RecursiveAstVisitor {
   /// Cache used to load stylesheets.
   ImportCache importCache;
 
+  /// The last `@forward` rule to be visited that was not an import-only file.
+  ForwardRule _lastRegularForward;
+
   _ReferenceVisitor(this.importCache);
 
   /// Constructs a new References object based on a [stylesheet] (imported by
@@ -342,7 +346,12 @@ class _ReferenceVisitor extends RecursiveAstVisitor {
     _currentUrl = node.span.sourceUrl;
     _isOrphanImportOnly = isImportOnlyFile(_currentUrl);
     super.visitStylesheet(node);
-    if (_isOrphanImportOnly) _orphanImportOnlyFiles.add(_currentUrl);
+    if (_isOrphanImportOnly) {
+      _orphanImportOnlyFiles[_currentUrl] =
+          _lastRegularForward.span.sourceUrl == _currentUrl
+              ? _lastRegularForward
+              : null;
+    }
     _isOrphanImportOnly = oldOrphaned;
     _namespaces = oldNamespaces;
     _currentUrl = oldUrl;
@@ -459,6 +468,7 @@ class _ReferenceVisitor extends RecursiveAstVisitor {
     if (_isOrphanImportOnly && _currentUrl == getImportOnlyUrl(canonicalUrl)) {
       _isOrphanImportOnly = false;
     }
+    if (!isImportOnlyFile(canonicalUrl)) _lastRegularForward = node;
     var moduleScope = _moduleScopes[canonicalUrl];
     for (var declaration in moduleScope.variables.values) {
       if (declaration.member is! VariableDeclaration) {

lib/src/migrators/module/references.dart

@@ -76,6 +76,47 @@ FileSpan extendForward(FileSpan span, String text) {
   return span.file.span(span.start.offset, end + text.length);
 }
 
+/// Returns the next location after [import] that it would be safe to insert
+/// a `@use` or `@forward` rule.
+///
+/// This is generally the start of the next line, but may vary if there's any
+/// non-whitespace, non-comment code on the same line.
+FileLocation afterImport(ImportRule import, {bool shouldHaveSemicolon = true}) {
+  var loc = import.span.end;
+  var textAfter = loc.file.getText(loc.offset);
+  var inLineComment = false;
+  var inBlockComment = false;
+  var i = 0;
+  for (; i < textAfter.length; i++) {
+    var char = textAfter.codeUnitAt(i);
+    if (inBlockComment) {
+      if (char == $asterisk && textAfter.codeUnitAt(i + 1) == $slash) {
+        i++;
+        inBlockComment = false;
+      }
+    } else if (char == $lf && !shouldHaveSemicolon) {
+      i++;
+      break;
+    } else if (inLineComment) {
+      continue;
+    } else if (char == $slash) {
+      var next = textAfter.codeUnitAt(i + 1);
+      if (next == $slash) {
+        inLineComment = true;
+      } else if (next == $asterisk) {
+        inBlockComment = true;
+      } else {
+        break;
+      }
+    } else if (shouldHaveSemicolon && char == $semicolon) {
+      shouldHaveSemicolon = false;
+    } else if (!isWhitespace(char)) {
+      break;
+    }
+  }
+  return loc.file.location(loc.offset + i);
+}
+
 /// Extends [span] backward if it's preceded by exactly [text].
 ///
 /// If [span] is preceded by anything other than [text], returns `null`.

lib/src/utils.dart

@@ -1,5 +1,5 @@
 name: sass_migrator
-version: 1.2.6-dev
+version: 1.2.6
 description: A tool for running migrations on Sass files
 author: Jennifer Thakar <[email protected]>
 homepage: https://github.com/sass/migrator

pubspec.yaml

@@ -0,0 +1,20 @@
+<==> arguments
+--migrate-deps
+
+<==> input/entrypoint.scss
+$indirect: red;
+@import "midstream";
+
+<==> input/_midstream.scss
+$direct: blue;
+@import "upstream";
+
+<==> input/_upstream.scss
+$direct: green !default;
+$indirect: green !default;
+
+<==> output/entrypoint.scss
+@use "midstream" with ($indirect: red);
+
+<==> output/_midstream.scss
+@forward "upstream" show $indirect with ($direct: blue);

test/migrators/module/configuration/direct_and_indirect.hrx

@@ -16,4 +16,3 @@ $config: green !default;
 
 <==> output/_direct.scss
 @forward "indirect" show $config;
-@use "indirect";

test/migrators/module/configuration/indirect.hrx

@@ -0,0 +1,26 @@
+<==> arguments
+--migrate-deps
+
+<==> README
+This test makes sure that comments after an `@import` are not mangled when an
+additional `@use` rule is inserted.
+
+<==> input/entrypoint.scss
+@import "direct"; // comment for direct
+
+a {color: $variable;}
+
+<==> input/_direct.scss
+@import "indirect";
+
+<==> input/_indirect.scss
+$variable: blue;
+
+<==> output/entrypoint.scss
+@use "direct"; // comment for direct
+@use "indirect";
+
+a {color: indirect.$variable;}
+
+<==> output/_direct.scss
+@use "indirect";

test/migrators/module/dependency_order/comment_after_import.hrx

@@ -43,11 +43,11 @@ $load-path: yellow;
 @use "sass:color";
 @forward "forwarded";
 @use "library";
+@forward "library";
 @use "load-path";
 @use "relative";
 
 @forward "load-path";
-@forward "library";
 @forward "relative";
 
 a {

test/migrators/module/dependency_order/full_order.hrx

@@ -0,0 +1,24 @@
+<==> arguments
+--migrate-deps
+
+<==> README
+This test makes sure that inserting an additional `@use` rule still works if an
+`@import` is followed by additional code on the same line.
+
+<==> input/entrypoint.scss
+@import "direct";a {color: $variable;}
+
+<==> input/_direct.scss
+@import "indirect";
+
+<==> input/_indirect.scss
+$variable: blue;
+
+<==> output/entrypoint.scss
+@use "direct";
+@use "indirect";
+
+a {color: indirect.$variable;}
+
+<==> output/_direct.scss
+@use "indirect";