Don't cache canonicalize calls when containingUrl is available (#2215)

See #2208

Author: nex3

CHANGELOG.md

@@ -1,3 +1,13 @@
+## 1.75.0
+
+* Fix a bug in which stylesheet canonicalization could be cached incorrectly
+  when custom importers or the Node.js package importer made decisions based on
+  the URL of the containing stylesheet.
+
+### JS API
+
+* Allow `importer` to be passed without `url` in `StringOptionsWithImporter`.
+
 ## 1.74.1
 
 * No user-visible changes.

lib/src/async_import_cache.dart

@@ -154,64 +154,85 @@ final class AsyncImportCache {
     }
 
     if (baseImporter != null && url.scheme == '') {
-      var relativeResult = await putIfAbsentAsync(
-          _relativeCanonicalizeCache,
-          (
-            url,
-            forImport: forImport,
-            baseImporter: baseImporter,
-            baseUrl: baseUrl
-          ),
-          () => _canonicalize(baseImporter, baseUrl?.resolveUri(url) ?? url,
-              baseUrl, forImport));
+      var relativeResult = await putIfAbsentAsync(_relativeCanonicalizeCache, (
+        url,
+        forImport: forImport,
+        baseImporter: baseImporter,
+        baseUrl: baseUrl
+      ), () async {
+        var (result, cacheable) = await _canonicalize(
+            baseImporter, baseUrl?.resolveUri(url) ?? url, baseUrl, forImport);
+        assert(
+            cacheable,
+            "Relative loads should always be cacheable because they never "
+            "provide access to the containing URL.");
+        return result;
+      });
       if (relativeResult != null) return relativeResult;
     }
 
-    return await putIfAbsentAsync(
-        _canonicalizeCache, (url, forImport: forImport), () async {
-      for (var importer in _importers) {
-        if (await _canonicalize(importer, url, baseUrl, forImport)
-            case var result?) {
+    var key = (url, forImport: forImport);
+    if (_canonicalizeCache.containsKey(key)) return _canonicalizeCache[key];
+
+    // Each indivudal call to a `canonicalize()` override may not be cacheable
+    // (specifically, if it has access to `containingUrl` it's too
+    // context-sensitive to usefully cache). We want to cache a given URL across
+    // the _entire_ importer chain, so we use [cacheable] to track whether _all_
+    // `canonicalize()` calls we've attempted are cacheable. Only if they are do
+    // we store the result in the cache.
+    var cacheable = true;
+    for (var importer in _importers) {
+      switch (await _canonicalize(importer, url, baseUrl, forImport)) {
+        case (var result?, true) when cacheable:
+          _canonicalizeCache[key] = result;
+          return result;
+
+        case (var result?, _):
           return result;
-        }
+
+        case (_, false):
+          cacheable = false;
       }
+    }
 
-      return null;
-    });
+    if (cacheable) _canonicalizeCache[key] = null;
+    return null;
   }
 
   /// Calls [importer.canonicalize] and prints a deprecation warning if it
   /// returns a relative URL.
   ///
-  /// If [resolveUrl] is `true`, this resolves [url] relative to [baseUrl]
-  /// before passing it to [importer].
-  Future<AsyncCanonicalizeResult?> _canonicalize(
-      AsyncImporter importer, Uri url, Uri? baseUrl, bool forImport,
-      {bool resolveUrl = false}) async {
-    var resolved =
-        resolveUrl && baseUrl != null ? baseUrl.resolveUri(url) : url;
+  /// This returns both the result of the call to `canonicalize()` and whether
+  /// that result is cacheable at all.
+  Future<(AsyncCanonicalizeResult?, bool cacheable)> _canonicalize(
+      AsyncImporter importer, Uri url, Uri? baseUrl, bool forImport) async {
     var canonicalize = forImport
-        ? () => inImportRule(() => importer.canonicalize(resolved))
-        : () => importer.canonicalize(resolved);
+        ? () => inImportRule(() => importer.canonicalize(url))
+        : () => importer.canonicalize(url);
 
     var passContainingUrl = baseUrl != null &&
         (url.scheme == '' || await importer.isNonCanonicalScheme(url.scheme));
     var result = await withContainingUrl(
         passContainingUrl ? baseUrl : null, canonicalize);
-    if (result == null) return null;
+
+    // TODO(sass/dart-sass#2208): Determine whether the containing URL was
+    // _actually_ accessed rather than assuming it was.
+    var cacheable = !passContainingUrl || importer is FilesystemImporter;
+
+    if (result == null) return (null, cacheable);
 
     if (result.scheme == '') {
       _logger.warnForDeprecation(
           Deprecation.relativeCanonical,
-          "Importer $importer canonicalized $resolved to $result.\n"
+          "Importer $importer canonicalized $url to $result.\n"
           "Relative canonical URLs are deprecated and will eventually be "
           "disallowed.");
     } else if (await importer.isNonCanonicalScheme(result.scheme)) {
-      throw "Importer $importer canonicalized $resolved to $result, which "
-          "uses a scheme declared as non-canonical.";
+      throw "Importer $importer canonicalized $url to $result, which uses a "
+          "scheme declared as non-canonical.";
     }
 
-    return (importer, result, originalUrl: resolved);
+    return ((importer, result, originalUrl: url), cacheable);
   }
 
   /// Tries to import [url] using one of this cache's importers.

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: d157b83599dbc07a80ac6cb5ffdf5dde03b60376
+// Checksum: 37dd173d676ec6cf201a25b3cca9ac81d92b1433
 //
 // ignore_for_file: unused_import
 
@@ -154,61 +154,85 @@ final class ImportCache {
     }
 
     if (baseImporter != null && url.scheme == '') {
-      var relativeResult = _relativeCanonicalizeCache.putIfAbsent(
-          (
-            url,
-            forImport: forImport,
-            baseImporter: baseImporter,
-            baseUrl: baseUrl
-          ),
-          () => _canonicalize(baseImporter, baseUrl?.resolveUri(url) ?? url,
-              baseUrl, forImport));
+      var relativeResult = _relativeCanonicalizeCache.putIfAbsent((
+        url,
+        forImport: forImport,
+        baseImporter: baseImporter,
+        baseUrl: baseUrl
+      ), () {
+        var (result, cacheable) = _canonicalize(
+            baseImporter, baseUrl?.resolveUri(url) ?? url, baseUrl, forImport);
+        assert(
+            cacheable,
+            "Relative loads should always be cacheable because they never "
+            "provide access to the containing URL.");
+        return result;
+      });
       if (relativeResult != null) return relativeResult;
     }
 
-    return _canonicalizeCache.putIfAbsent((url, forImport: forImport), () {
-      for (var importer in _importers) {
-        if (_canonicalize(importer, url, baseUrl, forImport) case var result?) {
+    var key = (url, forImport: forImport);
+    if (_canonicalizeCache.containsKey(key)) return _canonicalizeCache[key];
+
+    // Each indivudal call to a `canonicalize()` override may not be cacheable
+    // (specifically, if it has access to `containingUrl` it's too
+    // context-sensitive to usefully cache). We want to cache a given URL across
+    // the _entire_ importer chain, so we use [cacheable] to track whether _all_
+    // `canonicalize()` calls we've attempted are cacheable. Only if they are do
+    // we store the result in the cache.
+    var cacheable = true;
+    for (var importer in _importers) {
+      switch (_canonicalize(importer, url, baseUrl, forImport)) {
+        case (var result?, true) when cacheable:
+          _canonicalizeCache[key] = result;
+          return result;
+
+        case (var result?, _):
           return result;
-        }
+
+        case (_, false):
+          cacheable = false;
       }
+    }
 
-      return null;
-    });
+    if (cacheable) _canonicalizeCache[key] = null;
+    return null;
   }
 
   /// Calls [importer.canonicalize] and prints a deprecation warning if it
   /// returns a relative URL.
   ///
-  /// If [resolveUrl] is `true`, this resolves [url] relative to [baseUrl]
-  /// before passing it to [importer].
-  CanonicalizeResult? _canonicalize(
-      Importer importer, Uri url, Uri? baseUrl, bool forImport,
-      {bool resolveUrl = false}) {
-    var resolved =
-        resolveUrl && baseUrl != null ? baseUrl.resolveUri(url) : url;
+  /// This returns both the result of the call to `canonicalize()` and whether
+  /// that result is cacheable at all.
+  (CanonicalizeResult?, bool cacheable) _canonicalize(
+      Importer importer, Uri url, Uri? baseUrl, bool forImport) {
     var canonicalize = forImport
-        ? () => inImportRule(() => importer.canonicalize(resolved))
-        : () => importer.canonicalize(resolved);
+        ? () => inImportRule(() => importer.canonicalize(url))
+        : () => importer.canonicalize(url);
 
     var passContainingUrl = baseUrl != null &&
         (url.scheme == '' || importer.isNonCanonicalScheme(url.scheme));
     var result =
         withContainingUrl(passContainingUrl ? baseUrl : null, canonicalize);
-    if (result == null) return null;
+
+    // TODO(sass/dart-sass#2208): Determine whether the containing URL was
+    // _actually_ accessed rather than assuming it was.
+    var cacheable = !passContainingUrl || importer is FilesystemImporter;
+
+    if (result == null) return (null, cacheable);
 
     if (result.scheme == '') {
       _logger.warnForDeprecation(
           Deprecation.relativeCanonical,
-          "Importer $importer canonicalized $resolved to $result.\n"
+          "Importer $importer canonicalized $url to $result.\n"
           "Relative canonical URLs are deprecated and will eventually be "
           "disallowed.");
     } else if (importer.isNonCanonicalScheme(result.scheme)) {
-      throw "Importer $importer canonicalized $resolved to $result, which "
-          "uses a scheme declared as non-canonical.";
+      throw "Importer $importer canonicalized $url to $result, which uses a "
+          "scheme declared as non-canonical.";
     }
 
-    return (importer, result, originalUrl: resolved);
+    return ((importer, result, originalUrl: url), cacheable);
   }
 
   /// Tries to import [url] using one of this cache's importers.

pkg/sass_api/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 10.2.0
+
+* No user-visible changes.
+
 ## 10.1.1
 
 * No user-visible changes.

pkg/sass_api/pubspec.yaml

@@ -2,15 +2,15 @@ name: sass_api
 # Note: Every time we add a new Sass AST node, we need to bump the *major*
 # version because it's a breaking change for anyone who's implementing the
 # visitor interface(s).
-version: 10.1.1
+version: 10.2.0
 description: Additional APIs for Dart Sass.
 homepage: https://github.com/sass/dart-sass
 
 environment:
   sdk: ">=3.0.0 <4.0.0"
 
 dependencies:
-  sass: 1.74.1
+  sass: 1.75.0
 
 dev_dependencies:
   dartdoc: ^6.0.0

pubspec.yaml

@@ -1,5 +1,5 @@
 name: sass
-version: 1.74.1
+version: 1.75.0
 description: A Sass implementation in Dart.
 homepage: https://github.com/sass/dart-sass