Better handling of late imports (#280)

* Better handling of late imports

Fixes #278.

Also fixes a bug where a file that uses and imports the same dependency
would be migrated to duplicate `@use` rules.

* Code review

Author: jathak

CHANGELOG.md

@@ -1,3 +1,27 @@
+## 2.4.0
+
+### Module System Migration
+
+* Better handling of late `@import` rules. Previously, these were treated
+  identically to nested imports, but now they can be hoisted to the top of the
+  file where `@use` is allowed if they do not emit any CSS.
+
+  To allow the migrator to hoist even late imports that _do_ emit CSS, use the
+  `--unsafe-hoist` flag. `@import` rules that emit CSS will still be converted
+  to `meta.load-css()` even with this flag if no Sass members are referenced
+  from them.
+
+  If there are any plain CSS at-rules that are used by your post-processing
+  tools but never actually result in meaningful CSS, you can pass them
+  (without the `@`) to `--safe-at-rule` so that the migrator doesn't consider
+  them to emit CSS when handling late imports.
+
+  The migrator's behavior when encountering actual nested imports remains
+  unchanged.
+
+* Fix a bug that resulted in duplicate `@use` rules when migrating stylesheets
+  that contained both a `@use` and an `@import` of the same dependency.
+
 ## 2.3.3
 
 ### Module System Migration

lib/src/migrators/module.dart

@@ -35,6 +35,12 @@ class ModuleMigrator extends Migrator {
   final argParser = ArgParser()
     ..addFlag('built-in-only',
         help: 'Migrates global functions without migrating @import.')
+    ..addMultiOption('safe-at-rule',
+        help: 'CSS at-rules (without the @) used by postprocessing tools '
+            'that should not be considered to be emitting CSS.')
+    ..addFlag('unsafe-hoist',
+        help: 'Allow the migrator to hoist late imports to the top of the '
+            'file even when they emit CSS.')
     ..addMultiOption('remove-prefix',
         abbr: 'p',
         help: 'Removes PREFIX from all migrated member names.\n'
@@ -94,14 +100,16 @@ class ModuleMigrator extends Migrator {
           'which prefixed members to forward.');
     }
 
-    var references = References(importCache, stylesheet, importer);
+    var references = References(importCache, stylesheet, importer,
+        safeAtRules: argResults!['safe-at-rule'] as List<String>);
     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);
+        forwards: forwards,
+        unsafeHoist: argResults!['unsafe-hoist'] as bool);
     var migrated = visitor.run(stylesheet, importer);
     _filesWithRenamedDeclarations.addAll(
         {for (var member in visitor.renamedMembers.keys) member.sourceUrl});
@@ -156,6 +164,11 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
       assertInStylesheet(__builtInUseRules, '_builtInUseRules');
   Set<String>? __builtInUseRules;
 
+  /// Set of `@use` rules hoisted from `@import` rules later in the stylesheet.
+  Set<String> get _hoistedUseRules =>
+      assertInStylesheet(__hoistedUseRules, '_hoistedUseRules');
+  Set<String>? __hoistedUseRules;
+
   /// Set of additional `@use` rules for stylesheets at a load path.
   Set<String> get _additionalLoadPathUseRules => assertInStylesheet(
       __additionalLoadPathUseRules, '_additionalLoadPathUseRules');
@@ -184,6 +197,9 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
   /// from any member visible at the entrypoint.
   var _needsImportOnly = false;
 
+  /// The stylesheet currently being migrated.
+  late Stylesheet _currentStylesheet;
+
   /// Set of variables declared outside the current stylesheet that overrode
   /// `!default` variables within the current stylesheet.
   Set<MemberDeclaration<VariableDeclaration>> get _configuredVariables =>
@@ -211,6 +227,10 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
   /// Whether to migrate only global functions, leaving `@import` rules as-is.
   final bool builtInOnly;
 
+  /// Whether to allow hoisting imports to the top of the file even when they
+  /// emit CSS.
+  final bool unsafeHoist;
+
   /// Constructs a new module migration visitor.
   ///
   /// [importCache] must be the same one used by [references].
@@ -226,6 +246,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
       super.importCache, this.references, List<String> loadPaths,
       {required super.migrateDependencies,
       required this.builtInOnly,
+      required this.unsafeHoist,
       Iterable<String> prefixesToRemove = const [],
       this.forwards = const {}})
       : loadPaths = List.unmodifiable(
@@ -421,10 +442,12 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
   /// visiting it and restores the per-file state afterwards.
   @override
   void visitStylesheet(Stylesheet node) {
+    _currentStylesheet = node;
     var oldNamespaces = __namespaces;
     var oldForwardedUrls = __forwardedUrls;
     var oldUsedUrls = __usedUrls;
     var oldBuiltInUseRules = __builtInUseRules;
+    var oldHoistedUseRules = __hoistedUseRules;
     var oldLoadPathUseRules = __additionalLoadPathUseRules;
     var oldRelativeUseRules = __additionalRelativeUseRules;
     var oldBeforeFirstImport = _beforeFirstImport;
@@ -438,10 +461,11 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
                 ?.$2 ??
             rule.url: rule.namespace
     };
+    __usedUrls = _namespaces.keys.toSet();
     _determineNamespaces(node.span.sourceUrl!, _namespaces);
-    __usedUrls = {};
     __forwardedUrls = {};
     __builtInUseRules = {};
+    __hoistedUseRules = {};
     __additionalLoadPathUseRules = {};
     __additionalRelativeUseRules = {};
     _beforeFirstImport = null;
@@ -452,6 +476,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
     __forwardedUrls = oldForwardedUrls;
     __usedUrls = oldUsedUrls;
     __builtInUseRules = oldBuiltInUseRules;
+    __hoistedUseRules = oldHoistedUseRules;
     __additionalLoadPathUseRules = oldLoadPathUseRules;
     __additionalRelativeUseRules = oldRelativeUseRules;
     _beforeFirstImport = oldBeforeFirstImport;
@@ -474,7 +499,8 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
               useRulesToString(_builtInUseRules)),
           beforeExisting: true);
     }
-    var extras = useRulesToString(_additionalLoadPathUseRules) +
+    var extras = useRulesToString(_hoistedUseRules) +
+        useRulesToString(_additionalLoadPathUseRules) +
         useRulesToString(_additionalRelativeUseRules) +
         _getAdditionalForwardRules();
     if (extras == '') return;
@@ -795,7 +821,8 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
     }
     String rulesText;
 
-    var migratedRules = <String>[];
+    var inPlaceUseRules = <String>[];
+    var loadCssRules = <String>[];
 
     var indent = ' ' * node.span.start.column;
 
@@ -816,23 +843,35 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
         }
       }
       if (ruleUrl != null) {
+        var canonicalUrl = importCache
+            .canonicalize(ruleUrl, baseImporter: importer, baseUrl: currentUrl)!
+            .$2;
+        var isNested = !_currentStylesheet.children.contains(node);
         if (builtInOnly) {
           if (migrateDependencies) {
             _upstreamStylesheets.add(currentUrl);
             visitDependency(ruleUrl, import.span);
             _upstreamStylesheets.remove(currentUrl);
           }
         } else if (_useAllowed) {
-          migratedRules.addAll(_migrateImportToRules(ruleUrl, import.span));
+          inPlaceUseRules.addAll(_migrateImportToRules(ruleUrl, import.span));
+        } else if (!isNested &&
+            (!(references.fileEmitsCss[canonicalUrl] ?? true) ||
+                (unsafeHoist &&
+                    references.anyMemberReferenced(
+                        canonicalUrl, currentUrl)))) {
+          _hoistedUseRules.addAll(_migrateImportToRules(ruleUrl, import.span));
         } else {
-          migratedRules.add(_migrateImportToLoadCss(ruleUrl, import.span)
-              .replaceAll('\n', '\n$indent'));
+          loadCssRules.add(
+              _migrateImportToLoadCss(ruleUrl, import.span, isNested)
+                  .replaceAll('\n', '\n$indent'));
         }
       }
     }
     if (builtInOnly) return;
 
-    rulesText = migratedRules.join('$semicolon\n$indent');
+    rulesText =
+        [...inPlaceUseRules, ...loadCssRules].join('$semicolon\n$indent');
     if (rulesText.isEmpty) {
       var span = node.span.extendIfMatches(RegExp(' *$semicolon\n?'));
       addPatch(patchDelete(span));
@@ -894,8 +933,9 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
     if ((rules.isEmpty && normalForwardRules == null) ||
         withClause.isNotEmpty ||
         references.anyMemberReferenced(canonicalUrl, currentUrl)) {
-      _usedUrls.add(canonicalUrl);
-      rules.add('@use $quotedUrl$asClause$withClause');
+      if (_usedUrls.add(canonicalUrl)) {
+        rules.add('@use $quotedUrl$asClause$withClause');
+      }
     }
     if (normalForwardRules != null) rules.addAll(normalForwardRules);
     return rules;
@@ -906,7 +946,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
   ///
   /// This is used for migrating `@import` rules that are nested or appear after
   /// some other rules.
-  String _migrateImportToLoadCss(Uri ruleUrl, FileSpan context) {
+  String _migrateImportToLoadCss(Uri ruleUrl, FileSpan context, bool isNested) {
     var oldUnreferencable = _unreferencable;
     _unreferencable = UnreferencableMembers(_unreferencable);
     for (var declaration in references.allDeclarations) {
@@ -927,7 +967,11 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
     _unreferencable = oldUnreferencable;
     for (var declaration in references.allDeclarations) {
       if (declaration.sourceUrl != canonicalUrl) continue;
-      _unreferencable.add(declaration, UnreferencableType.fromNestedImport);
+      _unreferencable.add(
+          declaration,
+          isNested
+              ? UnreferencableType.fromNestedImport
+              : UnreferencableType.fromLateImport);
     }
 
     var meta = _findOrAddBuiltInNamespace('meta');

lib/src/migrators/module/references.dart

@@ -85,6 +85,13 @@ class References {
   /// regular file is forwarded by the import-only file).
   final Map<Uri, ForwardRule?> orphanImportOnlyFiles;
 
+  /// A map from stylesheet URLs to whether or not they possibly emit CSS,
+  ///
+  /// A file is considered to emit CSS if it contains style rules, contains
+  /// any top-level `@include` rules or plain CSS at-rules, or depends on any
+  /// file that itself emits CSS.
+  final Map<Uri, bool> fileEmitsCss;
+
   /// An iterable of all member declarations.
   Iterable<MemberDeclaration> get allDeclarations =>
       variables.values.followedBy(mixins.values).followedBy(functions.values);
@@ -142,7 +149,8 @@ class References {
       Set<MemberDeclaration> globalDeclarations,
       Map<MemberDeclaration, Set<Uri>> libraries,
       Map<SassReference, ReferenceSource> sources,
-      Map<Uri, ForwardRule?> orphanImportOnlyFiles)
+      Map<Uri, ForwardRule?> orphanImportOnlyFiles,
+      Map<Uri, bool> fileEmitsCss)
       : variables = UnmodifiableBidirectionalMapView(variables),
         variableReassignments =
             UnmodifiableBidirectionalMapView(variableReassignments),
@@ -158,13 +166,16 @@ class References {
             entry.key: UnmodifiableSetView(entry.value)
         }),
         sources = UnmodifiableMapView(sources),
-        orphanImportOnlyFiles = UnmodifiableMapView(orphanImportOnlyFiles);
+        orphanImportOnlyFiles = UnmodifiableMapView(orphanImportOnlyFiles),
+        fileEmitsCss = UnmodifiableMapView(fileEmitsCss);
 
   /// Constructs a new [References] object based on a [stylesheet] (imported by
   /// [importer]) and its dependencies.
   factory References(
-          ImportCache importCache, Stylesheet stylesheet, Importer importer) =>
-      _ReferenceVisitor(importCache).build(stylesheet, importer);
+          ImportCache importCache, Stylesheet stylesheet, Importer importer,
+          {Iterable<String> safeAtRules = const []}) =>
+      _ReferenceVisitor(importCache, safeAtRules.toSet())
+          .build(stylesheet, importer);
 }
 
 /// A visitor that builds a References object.
@@ -183,6 +194,7 @@ class _ReferenceVisitor extends ScopedAstVisitor {
   final _libraries = <MemberDeclaration, Set<Uri>>{};
   final _sources = <SassReference, ReferenceSource>{};
   final _orphanImportOnlyFiles = <Uri, ForwardRule?>{};
+  final _fileEmitsCss = <Uri, bool>{};
 
   /// Mapping from canonical stylesheet URLs to the global scope of the module
   /// contained within it.
@@ -229,6 +241,10 @@ class _ReferenceVisitor extends ScopedAstVisitor {
   /// stylesheet.
   late Importer _importer;
 
+  /// Whether any dependency of the current file emits CSS (which then means
+  /// that this file emits CSS even if it doesn't directly).
+  late bool _dependencyEmitsCss;
+
   /// If the current stylesheet is an import-only file, this starts as true and
   /// is changed to false if it forwards its regular counterpart.
   ///
@@ -241,7 +257,10 @@ class _ReferenceVisitor extends ScopedAstVisitor {
   /// The last `@forward` rule to be visited that was not an import-only file.
   ForwardRule? _lastRegularForward;
 
-  _ReferenceVisitor(this.importCache);
+  /// CSS at-rules that should be considered to emit CSS.
+  final Set<String> safeAtRules;
+
+  _ReferenceVisitor(this.importCache, this.safeAtRules);
 
   /// Constructs a new References object based on a [stylesheet] (imported by
   /// [importer]) and its dependencies.
@@ -253,6 +272,7 @@ class _ReferenceVisitor extends ScopedAstVisitor {
     _moduleScopes[_currentUrl] = currentScope;
     _declarationSources = {};
     _moduleSources[_currentUrl] = _declarationSources;
+    _dependencyEmitsCss = false;
     visitStylesheet(stylesheet);
 
     for (var variable in currentScope.variables.values) {
@@ -274,7 +294,8 @@ class _ReferenceVisitor extends ScopedAstVisitor {
         _globalDeclarations,
         _libraries,
         _sources,
-        _orphanImportOnlyFiles);
+        _orphanImportOnlyFiles,
+        _fileEmitsCss);
   }
 
   /// Checks any remaining [_unresolvedReferences] to see if they match a
@@ -318,6 +339,8 @@ class _ReferenceVisitor extends ScopedAstVisitor {
     var oldNamespaces = _namespaces;
     var oldUrl = _currentUrl;
     var oldOrphaned = _isOrphanImportOnly;
+    var oldDependencyEmitsCss = _dependencyEmitsCss;
+    _dependencyEmitsCss = false;
     _namespaces = {};
     _currentUrl = node.span.sourceUrl!;
     _isOrphanImportOnly = isImportOnlyFile(_currentUrl);
@@ -328,11 +351,30 @@ class _ReferenceVisitor extends ScopedAstVisitor {
               ? _lastRegularForward
               : null;
     }
+    var emitsCss = _dependencyEmitsCss || node.children.any(_statementEmitsCss);
+    _fileEmitsCss[_currentUrl] = emitsCss;
     _isOrphanImportOnly = oldOrphaned;
     _namespaces = oldNamespaces;
     _currentUrl = oldUrl;
+    _dependencyEmitsCss = oldDependencyEmitsCss || emitsCss;
   }
 
+  /// Returns true if [statement] emits any CSS.
+  bool _statementEmitsCss(Statement statement) => switch (statement) {
+        StyleRule() || IncludeRule() || MediaRule() || SupportsRule() => true,
+        AtRule(:var name) => !safeAtRules.contains(name.asPlain),
+        EachRule(:var children) ||
+        ForRule(:var children) ||
+        MediaRule(:var children) ||
+        WhileRule(:var children) =>
+          children.any(_statementEmitsCss),
+        IfRule(:var clauses) =>
+          clauses.any((clause) => clause.children.any(_statementEmitsCss)),
+        // All other statement types either don't emit CSS or are always
+        // nested inside of a CSS-emitting statement.
+        _ => false,
+      };
+
   /// Visits the stylesheet this `@import` rule points to using the existing
   /// global scope.
   @override