Improved late import hoisting (#282)

* Improved late import hoisting

While 2.4.0 allowed the migrator to hoist late imports that did not emit
CSS themselves, this additionally allows it to hoist them above safe at
rules in the same file, even if the imports themselves do actually emit
CSS.

* Code review

Author: jathak

CHANGELOG.md

@@ -1,3 +1,15 @@
+## 2.4.2
+
+### Module Migration
+
+* Late `@import` rules can now be hoisted above safe at-rules in the same file.
+
+* Fix a typo in the error message for late `@import` rules that could not be
+  migrated.
+
+* Fix a bug where hoisted `@use` rules with configuration would have incorrect
+  syntax.
+
 ## 2.4.1
 
 ### Module Migrator

lib/src/migrators/module.dart

@@ -25,6 +25,7 @@ import 'module/reference_source.dart';
 import 'module/references.dart';
 import 'module/unreferencable_members.dart';
 import 'module/unreferencable_type.dart';
+import 'module/use_allowed.dart';
 
 /// Migrates stylesheets to the new module system.
 class ModuleMigrator extends Migrator {
@@ -99,17 +100,19 @@ class ModuleMigrator extends Migrator {
           'You must provide --remove-prefix with --forward=prefixed so we know '
           'which prefixed members to forward.');
     }
+    var safeAtRules = (argResults!['safe-at-rule'] as List<String>).toSet();
 
-    var references = References(importCache, stylesheet, importer,
-        safeAtRules: argResults!['safe-at-rule'] as List<String>);
+    var references =
+        References(importCache, stylesheet, importer, safeAtRules: safeAtRules);
     var visitor = _ModuleMigrationVisitor(
         importCache, references, globalResults!['load-path'] as List<String>,
         migrateDependencies: migrateDependencies,
         builtInOnly: builtInOnly,
         prefixesToRemove: (argResults!['remove-prefix'] as List<String>)
             .map((prefix) => prefix.replaceAll('_', '-')),
         forwards: forwards,
-        unsafeHoist: argResults!['unsafe-hoist'] as bool);
+        unsafeHoist: argResults!['unsafe-hoist'] as bool,
+        safeAtRules: safeAtRules);
     var migrated = visitor.run(stylesheet, importer);
     _filesWithRenamedDeclarations.addAll(
         {for (var member in visitor.renamedMembers.keys) member.sourceUrl});
@@ -188,8 +191,9 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
   /// or `@forward` rule, or null if none has been visited yet.
   FileLocation? _afterLastImport;
 
-  /// Whether @use and @forward are allowed in the current context.
-  var _useAllowed = true;
+  /// The state of whether `@use` and `@forward` are allowed at the current
+  /// point in the file.
+  var _useAllowed = UseAllowed.allowed;
 
   /// Whether an import-only stylesheet should be generated.
   ///
@@ -231,6 +235,10 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
   /// emit CSS.
   final bool unsafeHoist;
 
+  /// CSS at rules that should be considered to not emit CSS for the purpose
+  /// of hoisting late `@import` rules.
+  final Set<String> safeAtRules;
+
   /// Constructs a new module migration visitor.
   ///
   /// [importCache] must be the same one used by [references].
@@ -248,7 +256,8 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
       required this.builtInOnly,
       required this.unsafeHoist,
       Iterable<String> prefixesToRemove = const [],
-      this.forwards = const {}})
+      this.forwards = const {},
+      this.safeAtRules = const {}})
       : loadPaths = List.unmodifiable(
             loadPaths.map((path) => p.toUri(p.absolute(path)).path)),
         prefixesToRemove = UnmodifiableSetView(prefixesToRemove.toSet());
@@ -470,7 +479,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
     __additionalRelativeUseRules = {};
     _beforeFirstImport = null;
     _afterLastImport = null;
-    _useAllowed = true;
+    _useAllowed = UseAllowed.allowed;
     super.visitStylesheet(node);
     __namespaces = oldNamespaces;
     __forwardedUrls = oldForwardedUrls;
@@ -804,7 +813,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
   /// Visits a `@function` rule, renaming if necessary.
   @override
   void visitFunctionRule(FunctionRule node) {
-    _useAllowed = false;
+    _useAllowed = UseAllowed.notAllowed;
     _renameReference(nameSpan(node), MemberDeclaration(node));
     super.visitFunctionRule(node);
   }
@@ -816,7 +825,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
     var (staticImports, dynamicImports) =
         partitionOnType<Import, StaticImport, DynamicImport>(node.imports);
     if (dynamicImports.isEmpty) {
-      _useAllowed = false;
+      _useAllowed = _useAllowed.lowerToRequiresHoist();
       return;
     }
     String rulesText;
@@ -853,10 +862,11 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
             visitDependency(ruleUrl, import.span);
             _upstreamStylesheets.remove(currentUrl);
           }
-        } else if (_useAllowed) {
+        } else if (_useAllowed.canMigrateInPlace) {
           inPlaceUseRules.addAll(_migrateImportToRules(ruleUrl, import.span));
         } else if (!isNested &&
-            (!(references.fileEmitsCss[canonicalUrl] ?? true) ||
+            (_useAllowed.canAlwaysSafelyHoist ||
+                !(references.fileEmitsCss[canonicalUrl] ?? true) ||
                 (unsafeHoist &&
                     references.anyMemberReferenced(
                         canonicalUrl, currentUrl)))) {
@@ -878,14 +888,14 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
     } else {
       addPatch(Patch(node.span, rulesText));
     }
-    if (_useAllowed) {
+    if (_useAllowed.canMigrateInPlace) {
       _beforeFirstImport ??= node.span.start;
       _afterLastImport = afterImport(node,
           shouldHaveSemicolon: !currentUrl.path.endsWith('.sass'));
     }
 
     if (staticImports.isNotEmpty) {
-      _useAllowed = false;
+      _useAllowed = _useAllowed.lowerToRequiresHoist();
       addPatch(Patch.insert(
           _afterLastImport ?? node.span.file.location(0),
           '$indent@import ' +
@@ -906,7 +916,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
   /// redirect to a different path to be migrated in-place.
   List<String> _migrateImportToRules(Uri ruleUrl, FileSpan context) {
     var (canonicalUrl, config, forwardForConfig) =
-        _migrateImportCommon(ruleUrl, context);
+        _migrateImportCommon(ruleUrl, context, toLoadCss: false);
 
     var asClause = '';
     var defaultNamespace = namespaceForPath(ruleUrl.path);
@@ -955,7 +965,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
     }
 
     var (canonicalUrl, config, forwardForConfig) =
-        _migrateImportCommon(ruleUrl, context);
+        _migrateImportCommon(ruleUrl, context, toLoadCss: true);
     if (forwardForConfig != null) {
       throw MigrationSourceSpanException(
           "This declaration attempts to override a default value in an "
@@ -990,7 +1000,12 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
   /// `meta.load-css`. The third is a string of variables that should be added
   /// to a `show` clause of a `@forward` rule so that they can be configured by
   /// an upstream file.
-  (Uri, String?, String?) _migrateImportCommon(Uri ruleUrl, FileSpan context) {
+  ///
+  /// When [toLoadCss] is false, this uses the variable syntax for configuration
+  /// used by `@use`. When it is true, it instead uses the map syntax used by
+  /// `meta.load-css()`.
+  (Uri, String?, String?) _migrateImportCommon(Uri ruleUrl, FileSpan context,
+      {required bool toLoadCss}) {
     var oldConfiguredVariables = __configuredVariables;
     __configuredVariables = {};
     _upstreamStylesheets.add(currentUrl);
@@ -1057,7 +1072,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
         var end = start + semicolon.length;
         if (span.file.span(end, end + 1).text == '\n') end++;
         addPatch(patchDelete(span.file.span(start, end)));
-        var nameFormat = _useAllowed ? '\$$name' : '"$name"';
+        var nameFormat = toLoadCss ? '"$name"' : '\$$name';
         configured.add("$nameFormat: ${variable.member.expression}");
       }
     });
@@ -1157,7 +1172,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
   /// Adds a namespace to any mixin include that requires it.
   @override
   void visitIncludeRule(IncludeRule node) {
-    _useAllowed = false;
+    _useAllowed = UseAllowed.notAllowed;
     super.visitIncludeRule(node);
     if (node.namespace != null) return;
     if (builtInOnly && references.sources[node] is! BuiltInSource) return;
@@ -1178,7 +1193,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
   /// Visits a `@mixin` rule, renaming it if necessary.
   @override
   void visitMixinRule(MixinRule node) {
-    _useAllowed = false;
+    _useAllowed = UseAllowed.notAllowed;
     if (!builtInOnly) _renameReference(nameSpan(node), MemberDeclaration(node));
     super.visitMixinRule(node);
   }
@@ -1421,84 +1436,88 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
   /// Disallows `@use` after `@at-root` rules.
   @override
   void visitAtRootRule(AtRootRule node) {
-    _useAllowed = false;
+    _useAllowed = UseAllowed.notAllowed;
     super.visitAtRootRule(node);
   }
 
-  /// Disallows `@use` after at-rules.
+  /// Disallows `@use` after at-rules depending on [safeAtRules].
   @override
   void visitAtRule(AtRule node) {
-    _useAllowed = false;
+    var oldUseAllowed = _useAllowed;
+    _useAllowed = UseAllowed.notAllowed;
     super.visitAtRule(node);
+    if (safeAtRules.contains(node.name.asPlain)) {
+      _useAllowed = oldUseAllowed.lowerToRequiresHoist();
+    }
   }
 
-  /// Disallows `@use` after `@debug` rules.
+  /// Requires hoisting `@use` after `@debug` rules.
   @override
   void visitDebugRule(DebugRule node) {
-    _useAllowed = false;
+    _useAllowed = _useAllowed.lowerToRequiresHoist();
     super.visitDebugRule(node);
   }
 
   /// Disallows `@use` after `@each` rules.
   @override
   void visitEachRule(EachRule node) {
-    _useAllowed = false;
+    _useAllowed = UseAllowed.notAllowed;
     super.visitEachRule(node);
   }
 
   /// Disallows `@use` after `@error` rules.
   @override
   void visitErrorRule(ErrorRule node) {
-    _useAllowed = false;
+    _useAllowed = UseAllowed.notAllowed;
     super.visitErrorRule(node);
   }
 
   /// Disallows `@use` after `@for` rules.
   @override
   void visitForRule(ForRule node) {
-    _useAllowed = false;
+    _useAllowed = UseAllowed.notAllowed;
     super.visitForRule(node);
   }
 
   /// Disallows `@use` after `@if` rules.
   @override
   void visitIfRule(IfRule node) {
-    _useAllowed = false;
+    _useAllowed = UseAllowed.notAllowed;
     super.visitIfRule(node);
   }
 
   /// Disallows `@use` after `@media` rules.
   @override
   void visitMediaRule(MediaRule node) {
-    _useAllowed = false;
+    _useAllowed = UseAllowed.notAllowed;
     super.visitMediaRule(node);
   }
 
   /// Disallows `@use` after style rules.
   @override
   void visitStyleRule(StyleRule node) {
-    _useAllowed = false;
+    _useAllowed = UseAllowed.notAllowed;
     super.visitStyleRule(node);
   }
 
   /// Disallows `@use` after `@supports` rules.
   @override
   void visitSupportsRule(SupportsRule node) {
-    _useAllowed = false;
+    _useAllowed = UseAllowed.notAllowed;
     super.visitSupportsRule(node);
   }
 
-  /// Disallows `@use` after `@warn` rules.
+  /// Requires hoisting `@use` after `@warn` rules.
   @override
   void visitWarnRule(WarnRule node) {
-    _useAllowed = false;
+    _useAllowed = _useAllowed.lowerToRequiresHoist();
     super.visitWarnRule(node);
   }
 
   /// Disallows `@use` after `@while` rules.
   @override
   void visitWhileRule(WhileRule node) {
-    _useAllowed = false;
+    _useAllowed = UseAllowed.notAllowed;
     super.visitWhileRule(node);
   }
 }

lib/src/migrators/module/references.dart

@@ -257,7 +257,8 @@ class _ReferenceVisitor extends ScopedAstVisitor {
   /// The last `@forward` rule to be visited that was not an import-only file.
   ForwardRule? _lastRegularForward;
 
-  /// CSS at-rules that should be considered to emit CSS.
+  /// CSS at rules that should be considered to not emit CSS for the purpose
+  /// of hoisting late `@import` rules.
   final Set<String> safeAtRules;
 
   _ReferenceVisitor(this.importCache, this.safeAtRules);

lib/src/migrators/module/unreferencable_type.dart

@@ -59,7 +59,7 @@ class UnreferencableType {
           "containing ${type}s\nand one emitting CSS will allow the migrator "
           "to safely migrate it by hoisting\nthe one containing members to "
           "the top of the file and converting the one\nemitting CSS to "
-          "meta.load-css().\n\nAlternatively, pass --force-hoist to force the "
+          "meta.load-css().\n\nAlternatively, pass --unsafe-hoist to force the "
           "migrator to hoist all\nlate imports containing Sass members, "
           "which may reorder your CSS.",
           reference.span,

lib/src/migrators/module/use_allowed.dart

@@ -0,0 +1,44 @@
+// Copyright 2025 Google LLC
+//
+// Use of this source code is governed by an MIT-style
+// license that can be found in the LICENSE file or at
+// https://opensource.org/licenses/MIT.
+
+import 'package:meta/meta.dart';
+
+/// An enum of potential states for whether unnested `@import`s at the current
+/// point in the file can be migrated.
+@sealed
+class UseAllowed {
+  /// Status when `@use` and `@forward` are allowed at this point in the file.
+  static const allowed = UseAllowed._('allowed');
+
+  /// Status when `@use` and `@forward` are not allowed at this point in the
+  /// file, but they can be safely hoisted to the top of the file because the
+  /// only rules encountered so far are dependency rules (including plain CSS
+  /// `@import` rules) and designated safe at rules.
+  static const requiresHoist = UseAllowed._('requiresHoist');
+
+  /// Status when `@use` and `@forward` are not allowed at this point in the
+  /// file and cannot necessarily be safely hoisted to the top of the file.
+  ///
+  /// Migrated `@import`s may still be hoisted at this point if they do not
+  /// emit any CSS themselves or if `--unsafe-hoist` is passed.
+  static const notAllowed = UseAllowed._('notAllowed');
+
+  /// Identifier for this status
+  final String id;
+
+  const UseAllowed._(this.id);
+
+  /// Returns [requiresHoist] unless [this] is already [notAllowed], in which
+  /// case it should remain [notAllowed].
+  UseAllowed lowerToRequiresHoist() =>
+      switch (this) { notAllowed => notAllowed, _ => requiresHoist };
+
+  /// Returns true when `@import` rules can be migrated in place in this state.
+  bool get canMigrateInPlace => this == allowed;
+
+  /// Returns true when all `@import` rules can be safely hoisted in this state.
+  bool get canAlwaysSafelyHoist => this != notAllowed;
+}

pubspec.yaml

@@ -1,5 +1,5 @@
 name: sass_migrator
-version: 2.4.1
+version: 2.4.2
 description: A tool for running migrations on Sass files
 homepage: https://github.com/sass/migrator
 

test/migrators/module/configuration/hoisted.hrx

@@ -0,0 +1,18 @@
+<==> arguments
+--migrate-deps
+
+<==> input/entrypoint.scss
+a { b: c; }
+$config: red;
+$no-config: blue;
+@import "library";
+
+<==> input/_library.scss
+$config: green !default;
+$no-config: yellow;
+
+<==> output/entrypoint.scss
+@use "library" with ($config: red);
+
+a { b: c; }
+$no-config: blue;

test/migrators/module/late_imports/css_and_references.hrx

@@ -26,7 +26,7 @@ and one emitting CSS will allow the migrator to safely migrate it by hoisting
 the one containing members to the top of the file and converting the one
 emitting CSS to meta.load-css().
 
-Alternatively, pass --force-hoist to force the migrator to hoist all
+Alternatively, pass --unsafe-hoist to force the migrator to hoist all
 late imports containing Sass members, which may reorder your CSS.
   ,
 8 |   e: $var;