Cache separate canonical URLs for @use and @import (#908)

Fixes #899.

The cache for canonical URLs is now keyed on *both* the rule URL and
whether that URL was canonicalized for an `@import` rule.

Author: jathak

CHANGELOG.md

@@ -1,3 +1,8 @@
+## 1.24.1
+
+* Fix a bug where the wrong file could be loaded when the same URL is used by
+  both a `@use` rule and an `@import` rule.
+
 ## 1.24.0
 
 * Add an optional `with` clause to the `@forward` rule. This works like the

lib/src/async_import_cache.dart

@@ -11,6 +11,7 @@ import 'package:tuple/tuple.dart';
 import 'ast/sass.dart';
 import 'importer.dart';
 import 'importer/result.dart';
+import 'importer/utils.dart';
 import 'io.dart';
 import 'logger.dart';
 import 'sync_package_resolver.dart';
@@ -26,11 +27,15 @@ class AsyncImportCache {
 
   /// The canonicalized URLs for each non-canonical URL.
   ///
+  /// The second item in each key's tuple is true when this canonicalization is
+  /// for an `@import` rule. Otherwise, it's for a `@use` or `@forward` rule.
+  ///
   /// This map's values are the same as the return value of [canonicalize].
   ///
   /// This cache isn't used for relative imports, because they're
   /// context-dependent.
-  final Map<Uri, Tuple3<AsyncImporter, Uri, Uri>> _canonicalizeCache;
+  final Map<Tuple2<Uri, bool>, Tuple3<AsyncImporter, Uri, Uri>>
+      _canonicalizeCache;
 
   /// The parsed stylesheets for each canonicalized import URL.
   final Map<Uri, Stylesheet> _importCache;
@@ -109,18 +114,20 @@ class AsyncImportCache {
   /// If any importers understand [url], returns that importer as well as the
   /// canonicalized URL. Otherwise, returns `null`.
   Future<Tuple3<AsyncImporter, Uri, Uri>> canonicalize(Uri url,
-      [AsyncImporter baseImporter, Uri baseUrl]) async {
+      {AsyncImporter baseImporter, Uri baseUrl, bool forImport = false}) async {
     if (baseImporter != null) {
       var resolvedUrl = baseUrl != null ? baseUrl.resolveUri(url) : url;
-      var canonicalUrl = await _canonicalize(baseImporter, resolvedUrl);
+      var canonicalUrl =
+          await _canonicalize(baseImporter, resolvedUrl, forImport);
       if (canonicalUrl != null) {
         return Tuple3(baseImporter, canonicalUrl, resolvedUrl);
       }
     }
 
-    return await putIfAbsentAsync(_canonicalizeCache, url, () async {
+    return await putIfAbsentAsync(_canonicalizeCache, Tuple2(url, forImport),
+        () async {
       for (var importer in _importers) {
-        var canonicalUrl = await _canonicalize(importer, url);
+        var canonicalUrl = await _canonicalize(importer, url, forImport);
         if (canonicalUrl != null) {
           return Tuple3(importer, canonicalUrl, url);
         }
@@ -132,8 +139,11 @@ class AsyncImportCache {
 
   /// Calls [importer.canonicalize] and prints a deprecation warning if it
   /// returns a relative URL.
-  Future<Uri> _canonicalize(AsyncImporter importer, Uri url) async {
-    var result = await importer.canonicalize(url);
+  Future<Uri> _canonicalize(
+      AsyncImporter importer, Uri url, bool forImport) async {
+    var result = await (forImport
+        ? inImportRule(() => importer.canonicalize(url))
+        : importer.canonicalize(url));
     if (result?.scheme == '') {
       _logger.warn("""
 Importer $importer canonicalized $url to $result.
@@ -153,8 +163,9 @@ Relative canonical URLs are deprecated and will eventually be disallowed.
   ///
   /// Caches the result of the import and uses cached results if possible.
   Future<Tuple2<AsyncImporter, Stylesheet>> import(Uri url,
-      [AsyncImporter baseImporter, Uri baseUrl]) async {
-    var tuple = await canonicalize(url, baseImporter, baseUrl);
+      {AsyncImporter baseImporter, Uri baseUrl, bool forImport = false}) async {
+    var tuple = await canonicalize(url,
+        baseImporter: baseImporter, baseUrl: baseUrl, forImport: forImport);
     if (tuple == null) return null;
     var stylesheet =
         await importCanonical(tuple.item1, tuple.item2, tuple.item3);
@@ -216,7 +227,8 @@ Relative canonical URLs are deprecated and will eventually be disallowed.
   ///
   /// Has no effect if the canonical version of [url] has not been cached.
   void clearCanonicalize(Uri url) {
-    _canonicalizeCache.remove(url);
+    _canonicalizeCache.remove(Tuple2(url, false));
+    _canonicalizeCache.remove(Tuple2(url, true));
   }
 
   /// Clears the cached parse tree for the stylesheet with the given

lib/src/executable/watch.dart

@@ -5,6 +5,7 @@
 import 'dart:async';
 import 'dart:collection';
 
+import 'package:meta/meta.dart';
 import 'package:path/path.dart' as p;
 import 'package:stack_trace/stack_trace.dart';
 import 'package:stream_transform/stream_transform.dart';
@@ -288,8 +289,9 @@ class _Watcher {
     var changed = <StylesheetNode>[];
     for (var node in _graph.nodes.values) {
       var importChanged = false;
-      for (var url in node.upstream.keys) {
-        if (_name(p.url.basename(url.path)) != name) continue;
+      void recanonicalize(Uri url, StylesheetNode upstream,
+          {@required bool forImport}) {
+        if (_name(p.url.basename(url.path)) != name) return;
         _graph.clearCanonicalize(url);
 
         // If the import produces a different canonicalized URL than it did
@@ -298,15 +300,25 @@ class _Watcher {
           Uri newCanonicalUrl;
           try {
             newCanonicalUrl = _graph.importCache
-                .canonicalize(url, node.importer, node.canonicalUrl)
+                .canonicalize(url,
+                    baseImporter: node.importer,
+                    baseUrl: node.canonicalUrl,
+                    forImport: forImport)
                 ?.item2;
           } catch (_) {
-            // If the call to canonicalize failed, do nothing. We'll surface the
-            // error more nicely when we try to recompile the file.
+            // If the call to canonicalize failed, do nothing. We'll surface
+            // the error more nicely when we try to recompile the file.
           }
-          importChanged = newCanonicalUrl != node.upstream[url]?.canonicalUrl;
+          importChanged = newCanonicalUrl != upstream?.canonicalUrl;
         }
       }
+
+      for (var entry in node.upstream.entries) {
+        recanonicalize(entry.key, entry.value, forImport: false);
+      }
+      for (var entry in node.upstreamImports.entries) {
+        recanonicalize(entry.key, entry.value, forImport: true);
+      }
       if (importChanged) changed.add(node);
     }
 

lib/src/import_cache.dart

@@ -5,7 +5,7 @@
 // DO NOT EDIT. This file was generated from async_import_cache.dart.
 // See tool/grind/synchronize.dart for details.
 //
-// Checksum: 8555cca43b8d54d392e81f33935fd379d1eb3c72
+// Checksum: 3ca2f221c3c1503c688be49d4f7501bd9be8031a
 //
 // ignore_for_file: unused_import
 
@@ -16,6 +16,7 @@ import 'package:tuple/tuple.dart';
 import 'ast/sass.dart';
 import 'importer.dart';
 import 'importer/result.dart';
+import 'importer/utils.dart';
 import 'io.dart';
 import 'logger.dart';
 import 'sync_package_resolver.dart';
@@ -31,11 +32,14 @@ class ImportCache {
 
   /// The canonicalized URLs for each non-canonical URL.
   ///
+  /// The second item in each key's tuple is true when this canonicalization is
+  /// for an `@import` rule. Otherwise, it's for a `@use` or `@forward` rule.
+  ///
   /// This map's values are the same as the return value of [canonicalize].
   ///
   /// This cache isn't used for relative imports, because they're
   /// context-dependent.
-  final Map<Uri, Tuple3<Importer, Uri, Uri>> _canonicalizeCache;
+  final Map<Tuple2<Uri, bool>, Tuple3<Importer, Uri, Uri>> _canonicalizeCache;
 
   /// The parsed stylesheets for each canonicalized import URL.
   final Map<Uri, Stylesheet> _importCache;
@@ -114,18 +118,18 @@ class ImportCache {
   /// If any importers understand [url], returns that importer as well as the
   /// canonicalized URL. Otherwise, returns `null`.
   Tuple3<Importer, Uri, Uri> canonicalize(Uri url,
-      [Importer baseImporter, Uri baseUrl]) {
+      {Importer baseImporter, Uri baseUrl, bool forImport = false}) {
     if (baseImporter != null) {
       var resolvedUrl = baseUrl != null ? baseUrl.resolveUri(url) : url;
-      var canonicalUrl = _canonicalize(baseImporter, resolvedUrl);
+      var canonicalUrl = _canonicalize(baseImporter, resolvedUrl, forImport);
       if (canonicalUrl != null) {
         return Tuple3(baseImporter, canonicalUrl, resolvedUrl);
       }
     }
 
-    return _canonicalizeCache.putIfAbsent(url, () {
+    return _canonicalizeCache.putIfAbsent(Tuple2(url, forImport), () {
       for (var importer in _importers) {
-        var canonicalUrl = _canonicalize(importer, url);
+        var canonicalUrl = _canonicalize(importer, url, forImport);
         if (canonicalUrl != null) {
           return Tuple3(importer, canonicalUrl, url);
         }
@@ -137,8 +141,10 @@ class ImportCache {
 
   /// Calls [importer.canonicalize] and prints a deprecation warning if it
   /// returns a relative URL.
-  Uri _canonicalize(Importer importer, Uri url) {
-    var result = importer.canonicalize(url);
+  Uri _canonicalize(Importer importer, Uri url, bool forImport) {
+    var result = (forImport
+        ? inImportRule(() => importer.canonicalize(url))
+        : importer.canonicalize(url));
     if (result?.scheme == '') {
       _logger.warn("""
 Importer $importer canonicalized $url to $result.
@@ -158,8 +164,9 @@ Relative canonical URLs are deprecated and will eventually be disallowed.
   ///
   /// Caches the result of the import and uses cached results if possible.
   Tuple2<Importer, Stylesheet> import(Uri url,
-      [Importer baseImporter, Uri baseUrl]) {
-    var tuple = canonicalize(url, baseImporter, baseUrl);
+      {Importer baseImporter, Uri baseUrl, bool forImport = false}) {
+    var tuple = canonicalize(url,
+        baseImporter: baseImporter, baseUrl: baseUrl, forImport: forImport);
     if (tuple == null) return null;
     var stylesheet = importCanonical(tuple.item1, tuple.item2, tuple.item3);
     return Tuple2(tuple.item1, stylesheet);
@@ -220,7 +227,8 @@ Relative canonical URLs are deprecated and will eventually be disallowed.
   ///
   /// Has no effect if the canonical version of [url] has not been cached.
   void clearCanonicalize(Uri url) {
-    _canonicalizeCache.remove(url);
+    _canonicalizeCache.remove(Tuple2(url, false));
+    _canonicalizeCache.remove(Tuple2(url, true));
   }
 
   /// Clears the cached parse tree for the stylesheet with the given

lib/src/importer/utils.dart

@@ -6,43 +6,42 @@ import 'package:path/path.dart' as p;
 
 import '../io.dart';
 
-/// Whether the Sass interpreter is currently evaluating a `@use` rule.
+/// Whether the Sass compiler is currently evaluating an `@import` rule.
 ///
-/// The `@use` rule has slightly different path-resolution behavior than
-/// `@import`: `@use` prioritizes a `.css` file with a given name at the same
-/// level as `.sass` and `.scss`, while `@import` prefers `.sass` and `.scss`
-/// over `.css`. It's admittedly hacky to set this globally, but `@import` will
-/// eventually be removed, at which point we can delete this and have one
-/// consistent behavior.
-bool _inUseRule = false;
+/// When evaluating `@import` rules, URLs should canonicalize to an import-only
+/// file if one exists for the URL being canonicalized. Otherwise,
+/// canonicalization should be identical for `@import` and `@use` rules. It's
+/// admittedly hacky to set this globally, but `@import` will eventually be
+/// removed, at which point we can delete this and have one consistent behavior.
+bool _inImportRule = false;
 
-/// Runs [callback] in a context where [resolveImportPath] uses `@use` semantics
-/// rather than `@import` semantics.
-T inUseRule<T>(T callback()) {
-  var wasInUseRule = _inUseRule;
-  _inUseRule = true;
+/// Runs [callback] in a context where [resolveImportPath] uses `@import`
+/// semantics rather than `@use` semantics.
+T inImportRule<T>(T callback()) {
+  var wasInImportRule = _inImportRule;
+  _inImportRule = true;
   try {
     return callback();
   } finally {
-    _inUseRule = wasInUseRule;
+    _inImportRule = wasInImportRule;
   }
 }
 
-/// Like [inUseRule], but asynchronous.
-Future<T> inUseRuleAsync<T>(Future<T> callback()) async {
-  var wasInUseRule = _inUseRule;
-  _inUseRule = true;
+/// Like [inImportRule], but asynchronous.
+Future<T> inImportRuleAsync<T>(Future<T> callback()) async {
+  var wasInImportRule = _inImportRule;
+  _inImportRule = true;
   try {
     return await callback();
   } finally {
-    _inUseRule = wasInUseRule;
+    _inImportRule = wasInImportRule;
   }
 }
 
 /// Resolves an imported path using the same logic as the filesystem importer.
 ///
-/// This tries to fill in extensions and partial prefixes and check if a directory default. If no file can be
-/// found, it returns `null`.
+/// This tries to fill in extensions and partial prefixes and check for a
+/// directory default. If no file can be found, it returns `null`.
 String resolveImportPath(String path) {
   var extension = p.extension(path);
   if (extension == '.sass' || extension == '.scss' || extension == '.css') {
@@ -96,7 +95,7 @@ String _exactlyOne(List<String> paths) {
       paths.map((path) => "  " + p.prettyUri(p.toUri(path))).join("\n");
 }
 
-/// If [_inUseRule] is `false`, invokes callback and returns the result.
+/// If [_inImportRule] is `true`, invokes callback and returns the result.
 ///
 /// Otherwise, returns `null`.
-T _ifInImport<T>(T callback()) => _inUseRule ? null : callback();
+T _ifInImport<T>(T callback()) => _inImportRule ? callback() : null;