Properly migrate orphan import-only files (#162)

Fixes #160.

Co-authored-by: Natalie Weizenbaum <[email protected]>

Author: jathak

.travis.yml

@@ -39,7 +39,7 @@ jobs:
 
   # Static checks
   - dart_task: {dartanalyzer: --fatal-warnings --fatal-hints ./}
-  - dart_task: dartfmt
+  - dart_task: {dartfmt: sdk}
 
   ## Deployment
 

CHANGELOG.md

@@ -1,3 +1,12 @@
+## 1.2.4
+
+### Module Migrator
+
+* The migrator no longer crashes when it encounters an import-only file without
+  a corresponding regular file.
+* If an import-only file does not forward its corresponding regular file, the
+  migrator no longer includes a `@use` rule for it.
+
 ## 1.2.3
 
 * Updates help text to use the correct binary name (`sass-migrator`).

lib/src/migrators/module.dart

@@ -447,12 +447,11 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
     // If there was already a blank line after the insertion point, or the
     // insertion point was at the end of the file, remove the additional line
     // break at the end of the extra rules.
-    if (insertionPoint == node.span.start) extras = '$extras\n';
+    extras = '$extras\n';
     var whitespace = extendThroughWhitespace(insertionPoint.pointSpan());
     if (whitespace.text.contains('\n\n') || whitespace.end == node.span.end) {
       extras = extras.substring(0, extras.length - 1);
     }
-    if (insertionPoint == _afterLastImport) extras = '\n$extras';
     addPatch(Patch.insert(insertionPoint, extras));
   }
 
@@ -746,7 +745,13 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
     var imports =
         partitionOnType<Import, StaticImport, DynamicImport>(node.imports);
     var staticImports = imports.item1;
-    var dynamicImports = imports.item2;
+    var dynamicImports = imports.item2.where((import) {
+      var url = importCache
+          .canonicalize(Uri.parse(import.url),
+              baseImporter: importer, forImport: true)
+          ?.item2;
+      return !references.orphanImportOnlyFiles.contains(url);
+    });
 
     var start = node.span.start;
     var first = true;
@@ -760,6 +765,10 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
       _migrateImport(import, start);
     }
 
+    var spanWithNewline = extendForward(
+            extendThroughWhitespace(node.span), '$_semicolonIfNotIndented\n') ??
+        node.span;
+
     if (staticImports.isNotEmpty) {
       if (dynamicImports.isNotEmpty) {
         addPatch(Patch.insert(start, "$_semicolonIfNotIndented\n"));
@@ -777,18 +786,13 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
             extendForward(extended, ",") ?? extendBackward(extended, ",")));
       }
     } else {
-      addPatch(patchDelete(node.span));
+      addPatch(
+          patchDelete(dynamicImports.isEmpty ? spanWithNewline : node.span));
     }
 
     if (_useAllowed) {
       _beforeFirstImport ??= node.span.start;
-      if (currentUrl.path.endsWith('.sass')) {
-        _afterLastImport = node.span.end;
-      } else {
-        _afterLastImport =
-            extendForward(extendThroughWhitespace(node.span), ';')?.end ??
-                node.span.end;
-      }
+      _afterLastImport = spanWithNewline.end;
     }
   }
 

lib/src/migrators/module/references.dart

@@ -90,6 +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;
+
   /// An iterable of all member declarations.
   Iterable<MemberDeclaration> get allDeclarations =>
       variables.values.followedBy(mixins.values).followedBy(functions.values);
@@ -149,7 +153,8 @@ class References {
           getFunctionReferences,
       Set<MemberDeclaration> globalDeclarations,
       Map<MemberDeclaration, Set<Uri>> libraries,
-      Map<SassNode, ReferenceSource> sources)
+      Map<SassNode, ReferenceSource> sources,
+      Set<Uri> orphanImportOnlyFiles)
       : variables = UnmodifiableBidirectionalMapView(variables),
         variableReassignments =
             UnmodifiableBidirectionalMapView(variableReassignments),
@@ -162,7 +167,8 @@ class References {
         globalDeclarations = UnmodifiableSetView(globalDeclarations),
         libraries = UnmodifiableMapView(
             mapMap(libraries, value: (_, urls) => UnmodifiableSetView(urls))),
-        sources = UnmodifiableMapView(sources);
+        sources = UnmodifiableMapView(sources),
+        orphanImportOnlyFiles = UnmodifiableSetView(orphanImportOnlyFiles);
 
   /// Constructs a new [References] object based on a [stylesheet] (imported by
   /// [importer]) and its dependencies.
@@ -188,6 +194,7 @@ class _ReferenceVisitor extends RecursiveAstVisitor {
   final _globalDeclarations = <MemberDeclaration>{};
   final _libraries = <MemberDeclaration, Set<Uri>>{};
   final _sources = <SassNode, ReferenceSource>{};
+  final _orphanImportOnlyFiles = <Uri>{};
 
   /// The current global scope.
   ///
@@ -243,6 +250,12 @@ class _ReferenceVisitor extends RecursiveAstVisitor {
   /// stylesheet.
   Importer _importer;
 
+  /// If the current stylesheet is an import-only file, this starts as true and
+  /// is changed to false if it forwards its regular counterpart.
+  ///
+  /// This is always false for regular files.
+  bool _isOrphanImportOnly;
+
   /// Cache used to load stylesheets.
   ImportCache importCache;
 
@@ -276,7 +289,8 @@ class _ReferenceVisitor extends RecursiveAstVisitor {
         _getFunctionReferences,
         _globalDeclarations,
         _libraries,
-        _sources);
+        _sources,
+        _orphanImportOnlyFiles);
   }
 
   /// Checks any remaining [_unresolvedReferences] to see if they match a
@@ -321,9 +335,13 @@ class _ReferenceVisitor extends RecursiveAstVisitor {
   void visitStylesheet(Stylesheet node) {
     var oldNamespaces = _namespaces;
     var oldUrl = _currentUrl;
+    var oldOrphaned = _isOrphanImportOnly;
     _namespaces = {};
     _currentUrl = node.span.sourceUrl;
+    _isOrphanImportOnly = isImportOnlyFile(_currentUrl);
     super.visitStylesheet(node);
+    if (_isOrphanImportOnly) _orphanImportOnlyFiles.add(_currentUrl);
+    _isOrphanImportOnly = oldOrphaned;
     _namespaces = oldNamespaces;
     _currentUrl = oldUrl;
   }
@@ -436,6 +454,9 @@ class _ReferenceVisitor extends RecursiveAstVisitor {
   void visitForwardRule(ForwardRule node) {
     super.visitForwardRule(node);
     var canonicalUrl = _loadUseOrForward(node.url, node);
+    if (_isOrphanImportOnly && _currentUrl == getImportOnlyUrl(canonicalUrl)) {
+      _isOrphanImportOnly = false;
+    }
     var moduleScope = _moduleScopes[canonicalUrl];
     for (var declaration in moduleScope.variables.values) {
       if (declaration.member is! VariableDeclaration) {

lib/src/migrators/module/unreferencable_type.dart

@@ -33,7 +33,9 @@ class UnreferencableType {
   MigrationException toException(SassNode reference, Uri source) {
     var type = reference is IncludeRule
         ? 'mixin'
-        : reference is FunctionExpression ? 'function' : 'variable';
+        : reference is FunctionExpression
+            ? 'function'
+            : 'variable';
     var url = p.prettyUri(source);
     switch (this) {
       case fromImporter:

pubspec.yaml

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

test/migrators/module/partial_migration/changed_path.hrx

@@ -0,0 +1,24 @@
+<==> arguments
+--migrate-deps
+
+<==> input/entrypoint.scss
+@import "old";
+a {
+  b: $lib-variable;
+}
+
+<==> input/_old.scss
+@error "Use 'new'";
+
+<==> input/_old.import.scss
+@forward "new" as lib-*;
+
+<==> input/_new.scss
+$variable: green;
+
+<==> output/entrypoint.scss
+@use "new";
+
+a {
+  b: new.$variable;
+}

test/migrators/module/partial_migration/changed_path_no_regular.hrx

@@ -0,0 +1,21 @@
+<==> arguments
+--migrate-deps
+
+<==> input/entrypoint.scss
+@import "old";
+a {
+  b: $lib-variable;
+}
+
+<==> input/_old.import.scss
+@forward "new" as lib-*;
+
+<==> input/_new.scss
+$variable: green;
+
+<==> output/entrypoint.scss
+@use "new";
+
+a {
+  b: new.$variable;
+}

test/migrators/module/partial_migration/changed_path_split.hrx

@@ -0,0 +1,28 @@
+<==> arguments
+--migrate-deps
+
+<==> input/entrypoint.scss
+@import "old";
+a {
+  b: $lib-variable1;
+  c: $lib-variable2;
+}
+
+<==> input/_old.import.scss
+@forward "a" as lib-*;
+@forward "b" as lib-*;
+
+<==> input/_a.scss
+$variable1: green;
+
+<==> input/_b.scss
+$variable2: blue;
+
+<==> output/entrypoint.scss
+@use "a";
+@use "b";
+
+a {
+  b: a.$variable1;
+  c: b.$variable2;
+}

test/utils.dart

@@ -57,7 +57,8 @@ Future<void> _testHrx(File hrxFile, String migrator) async {
     migrator,
     '--no-unicode',
     ...files.arguments,
-    for (var path in files.input.keys) if (path.startsWith("entrypoint")) path
+    for (var path in files.input.keys)
+      if (path.startsWith("entrypoint")) path
   ]);
 
   if (files.expectedLog != null) {