Fix an import-resolution bug (#488)

When a stylesheet is imported, the parsed stylesheet object is cached based on its canonical URL. However, the stylesheet.span.sourceUrl was based on the text of the import that was used to load that stylesheet. The idea was to make the source URL in stack traces look nicer, but it meant that relative URLs could be resolved based on the old importer's URL before being sent to the new importer, which caused bugs. Now stylesheet.span.sourceUrl is always the canonical URL of the stylesheet, and thus safe to cache. We then use the import cache to convert the canonical URL to a human-friendly URL at the point at which we generate stack traces. This also deprecates support for relative canonical URLs. The semantics of these URLs were always unclear, and with the new change in import internals the old behavior doesn't make much sense. It's preserved for backwards-compatibility, but deprecated.
sass · Oct 11, 2018 · 0595ac3 · 0595ac3
1 parent 123bc1f
commit 0595ac3
Show file tree

Hide file tree

Showing 10 changed files with 191 additions and 66 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,11 +1,20 @@
 ## 1.14.2
 
+* Fix a bug where loading the same stylesheet from two different import paths
+  could cause its imports to fail to resolve.
+
 * Properly escape U+001F INFORMATION SEPARATOR ONE in unquoted strings.
 
 ### Command-Line Interface
 
 * Don't crash when using `@debug` in a stylesheet passed on standard input.
 
+### Dart API
+
+* `AsyncImporter.canonicalize()` and `Importer.canonicalize()` must now return
+  absolute URLs. Relative URLs are still supported, but are deprecated and will
+  be removed in a future release.
+
 ## 1.14.1
 
 * Canonicalize escaped digits at the beginning of identifiers as hex escapes.

diff --git a/lib/src/async_import_cache.dart b/lib/src/async_import_cache.dart
@@ -4,6 +4,7 @@
 
 import 'dart:async';
 
+import 'package:collection/collection.dart';
 import 'package:path/path.dart' as p;
 import 'package:tuple/tuple.dart';
 
@@ -94,15 +95,15 @@ class AsyncImportCache {
       [AsyncImporter baseImporter, Uri baseUrl]) async {
     if (baseImporter != null) {
       var resolvedUrl = baseUrl != null ? baseUrl.resolveUri(url) : url;
-      var canonicalUrl = await baseImporter.canonicalize(resolvedUrl);
+      var canonicalUrl = await _canonicalize(baseImporter, resolvedUrl);
       if (canonicalUrl != null) {
         return new Tuple3(baseImporter, canonicalUrl, resolvedUrl);
       }
     }
 
     return await putIfAbsentAsync(_canonicalizeCache, url, () async {
       for (var importer in _importers) {
-        var canonicalUrl = await importer.canonicalize(url);
+        var canonicalUrl = await _canonicalize(importer, url);
         if (canonicalUrl != null) {
           return new Tuple3(importer, canonicalUrl, url);
         }
@@ -112,6 +113,19 @@ 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);
+    if (result?.scheme == '') {
+      _logger.warn("""
+Importer $importer canonicalized $url to $result.
+Relative canonical URLs are deprecated and will eventually be disallowed.
+""", deprecation: true);
+    }
+    return result;
+  }
+
   /// Tries to import [url] using one of this cache's importers.
   ///
   /// If [baseImporter] is non-`null`, this first tries to use [baseImporter] to
@@ -145,18 +159,35 @@ class AsyncImportCache {
     return await putIfAbsentAsync(_importCache, canonicalUrl, () async {
       var result = await importer.load(canonicalUrl);
       if (result == null) return null;
-
-      // Use the canonicalized basename so that we display e.g.
-      // package:example/_example.scss rather than package:example/example in
-      // stack traces.
-      var displayUrl = originalUrl == null
-          ? canonicalUrl
-          : originalUrl.resolve(p.url.basename(canonicalUrl.path));
       return new Stylesheet.parse(result.contents, result.syntax,
-          url: displayUrl, logger: _logger);
+          // For backwards-compatibility, relative canonical URLs are resolved
+          // relative to [originalUrl].
+          url: originalUrl == null
+              ? canonicalUrl
+              : originalUrl.resolveUri(canonicalUrl),
+          logger: _logger);
     });
   }
 
+  /// Return a human-friendly URL for [canonicalUrl] to use in a stack trace.
+  ///
+  /// Throws a [StateError] if the stylesheet for [canonicalUrl] hasn't been
+  /// loaded by this cache.
+  Uri humanize(Uri canonicalUrl) {
+    // Display the URL with the shortest path length.
+    var url = minBy(
+        _canonicalizeCache.values
+            .where((tuple) => tuple?.item2 == canonicalUrl)
+            .map((tuple) => tuple.item3),
+        (url) => url.path.length);
+    if (url == null) return canonicalUrl;
+
+    // Use the canonicalized basename so that we display e.g.
+    // package:example/_example.scss rather than package:example/example in
+    // stack traces.
+    return url.resolve(p.url.basename(canonicalUrl.path));
+  }
+
   /// Clears the cached canonical version of the given [url].
   ///
   /// Has no effect if the canonical version of [url] has not been cached.

diff --git a/lib/src/import_cache.dart b/lib/src/import_cache.dart
@@ -5,8 +5,9 @@
 // DO NOT EDIT. This file was generated from async_import_cache.dart.
 // See tool/synchronize.dart for details.
 //
-// Checksum: 172219d313175ec88ce1ced2c37ca4b60348a4d2
+// Checksum: 57c42546fb8e0b68e29ea841ba106ee99127bede
 
+import 'package:collection/collection.dart';
 import 'package:path/path.dart' as p;
 import 'package:tuple/tuple.dart';
 
@@ -97,15 +98,15 @@ class ImportCache {
       [Importer baseImporter, Uri baseUrl]) {
     if (baseImporter != null) {
       var resolvedUrl = baseUrl != null ? baseUrl.resolveUri(url) : url;
-      var canonicalUrl = baseImporter.canonicalize(resolvedUrl);
+      var canonicalUrl = _canonicalize(baseImporter, resolvedUrl);
       if (canonicalUrl != null) {
         return new Tuple3(baseImporter, canonicalUrl, resolvedUrl);
       }
     }
 
     return _canonicalizeCache.putIfAbsent(url, () {
       for (var importer in _importers) {
-        var canonicalUrl = importer.canonicalize(url);
+        var canonicalUrl = _canonicalize(importer, url);
         if (canonicalUrl != null) {
           return new Tuple3(importer, canonicalUrl, url);
         }
@@ -115,6 +116,19 @@ 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);
+    if (result?.scheme == '') {
+      _logger.warn("""
+Importer $importer canonicalized $url to $result.
+Relative canonical URLs are deprecated and will eventually be disallowed.
+""", deprecation: true);
+    }
+    return result;
+  }
+
   /// Tries to import [url] using one of this cache's importers.
   ///
   /// If [baseImporter] is non-`null`, this first tries to use [baseImporter] to
@@ -147,18 +161,35 @@ class ImportCache {
     return _importCache.putIfAbsent(canonicalUrl, () {
       var result = importer.load(canonicalUrl);
       if (result == null) return null;
-
-      // Use the canonicalized basename so that we display e.g.
-      // package:example/_example.scss rather than package:example/example in
-      // stack traces.
-      var displayUrl = originalUrl == null
-          ? canonicalUrl
-          : originalUrl.resolve(p.url.basename(canonicalUrl.path));
       return new Stylesheet.parse(result.contents, result.syntax,
-          url: displayUrl, logger: _logger);
+          // For backwards-compatibility, relative canonical URLs are resolved
+          // relative to [originalUrl].
+          url: originalUrl == null
+              ? canonicalUrl
+              : originalUrl.resolveUri(canonicalUrl),
+          logger: _logger);
     });
   }
 
+  /// Return a human-friendly URL for [canonicalUrl] to use in a stack trace.
+  ///
+  /// Throws a [StateError] if the stylesheet for [canonicalUrl] hasn't been
+  /// loaded by this cache.
+  Uri humanize(Uri canonicalUrl) {
+    // Display the URL with the shortest path length.
+    var url = minBy(
+        _canonicalizeCache.values
+            .where((tuple) => tuple?.item2 == canonicalUrl)
+            .map((tuple) => tuple.item3),
+        (url) => url.path.length);
+    if (url == null) return canonicalUrl;
+
+    // Use the canonicalized basename so that we display e.g.
+    // package:example/_example.scss rather than package:example/example in
+    // stack traces.
+    return url.resolve(p.url.basename(canonicalUrl.path));
+  }
+
   /// Clears the cached canonical version of the given [url].
   ///
   /// Has no effect if the canonical version of [url] has not been cached.

diff --git a/lib/src/importer/async.dart b/lib/src/importer/async.dart
@@ -22,6 +22,10 @@ import 'result.dart';
 abstract class AsyncImporter {
   /// If [url] is recognized by this importer, returns its canonical format.
   ///
+  /// Note that canonical URLs *must* be absolute, including a scheme. Returning
+  /// `file:` URLs is encouraged if the imported stylesheet comes from a file on
+  /// disk.
+  ///
   /// If Sass has already loaded a stylesheet with the returned canonical URL,
   /// it re-uses the existing parse tree. This means that importers **must
   /// ensure** that the same canonical URL always refers to the same stylesheet,

diff --git a/lib/src/utils.dart b/lib/src/utils.dart
@@ -138,8 +138,11 @@ bool listEquals<T>(List<T> list1, List<T> list2) =>
 int listHash(List list) => const ListEquality().hash(list);
 
 /// Returns a stack frame for the given [span] with the given [member] name.
-Frame frameForSpan(SourceSpan span, String member) => new Frame(
-    span.sourceUrl ?? _noSourceUrl,
+///
+/// By default, the frame's URL is set to `span.sourceUrl`. However, if [url] is
+/// passed, it's used instead.
+Frame frameForSpan(SourceSpan span, String member, {Uri url}) => new Frame(
+    url ?? span.sourceUrl ?? _noSourceUrl,
     span.start.line + 1,
     span.start.column + 1,
     member);

diff --git a/lib/src/visitor/async_evaluate.dart b/lib/src/visitor/async_evaluate.dart
@@ -188,7 +188,10 @@ class _EvaluateVisitor
 
   /// The dynamic call stack representing function invocations, mixin
   /// invocations, and imports surrounding the current context.
-  final _stack = <Frame>[];
+  ///
+  /// Each member is a tuple of the span where the stack trace starts and the
+  /// name of the member being invoked.
+  final _stack = <Tuple2<String, FileSpan>>[];
 
   /// Whether we're running in Node Sass-compatibility mode.
   bool get _asNodeSass => _nodeImporter != null;
@@ -775,8 +778,7 @@ class _EvaluateVisitor
       }
     } on SassException catch (error) {
       var frames = error.trace.frames.toList()
-        ..add(_stackFrame(import.span))
-        ..addAll(_stack.toList());
+        ..addAll(_stackTrace(import.span).frames);
       throw new SassRuntimeException(
           error.message, error.span, new Trace(frames));
     } catch (error) {
@@ -1803,7 +1805,7 @@ class _EvaluateVisitor
   /// Runs [callback] with the new stack.
   Future<T> _withStackFrame<T>(
       String member, FileSpan span, Future<T> callback()) async {
-    _stack.add(_stackFrame(span));
+    _stack.add(new Tuple2(_member, span));
     var oldMember = _member;
     _member = member;
     var result = await callback();
@@ -1812,15 +1814,21 @@ class _EvaluateVisitor
     return result;
   }
 
-  /// Creates a new stack frame with location information from [span] and
-  /// [_member].
-  Frame _stackFrame(FileSpan span) => frameForSpan(span, _member);
+  /// Creates a new stack frame with location information from [member] and
+  /// [span].
+  Frame _stackFrame(String member, FileSpan span) => frameForSpan(span, member,
+      url: span.sourceUrl == null
+          ? null
+          : _importCache.humanize(span.sourceUrl));
 
   /// Returns a stack trace at the current point.
   ///
   /// [span] is the current location, used for the bottom-most stack frame.
   Trace _stackTrace(FileSpan span) {
-    var frames = _stack.toList()..add(_stackFrame(span));
+    var frames = _stack
+        .map((tuple) => _stackFrame(tuple.item1, tuple.item2))
+        .toList()
+          ..add(_stackFrame(_member, span));
     return new Trace(frames.reversed);
   }
 

diff --git a/lib/src/visitor/evaluate.dart b/lib/src/visitor/evaluate.dart
@@ -5,7 +5,7 @@
 // DO NOT EDIT. This file was generated from async_evaluate.dart.
 // See tool/synchronize.dart for details.
 //
-// Checksum: 11e77e1df658d69b4ecab6447225f79c358db535
+// Checksum: f20e0967bae462f7d1728053fa0a41c09bcc0e03
 
 import 'async_evaluate.dart' show EvaluateResult;
 export 'async_evaluate.dart' show EvaluateResult;
@@ -193,7 +193,10 @@ class _EvaluateVisitor
 
   /// The dynamic call stack representing function invocations, mixin
   /// invocations, and imports surrounding the current context.
-  final _stack = <Frame>[];
+  ///
+  /// Each member is a tuple of the span where the stack trace starts and the
+  /// name of the member being invoked.
+  final _stack = <Tuple2<String, FileSpan>>[];
 
   /// Whether we're running in Node Sass-compatibility mode.
   bool get _asNodeSass => _nodeImporter != null;
@@ -773,8 +776,7 @@ class _EvaluateVisitor
       }
     } on SassException catch (error) {
       var frames = error.trace.frames.toList()
-        ..add(_stackFrame(import.span))
-        ..addAll(_stack.toList());
+        ..addAll(_stackTrace(import.span).frames);
       throw new SassRuntimeException(
           error.message, error.span, new Trace(frames));
     } catch (error) {
@@ -1774,7 +1776,7 @@ class _EvaluateVisitor
   ///
   /// Runs [callback] with the new stack.
   T _withStackFrame<T>(String member, FileSpan span, T callback()) {
-    _stack.add(_stackFrame(span));
+    _stack.add(new Tuple2(_member, span));
     var oldMember = _member;
     _member = member;
     var result = callback();
@@ -1783,15 +1785,21 @@ class _EvaluateVisitor
     return result;
   }
 
-  /// Creates a new stack frame with location information from [span] and
-  /// [_member].
-  Frame _stackFrame(FileSpan span) => frameForSpan(span, _member);
+  /// Creates a new stack frame with location information from [member] and
+  /// [span].
+  Frame _stackFrame(String member, FileSpan span) => frameForSpan(span, member,
+      url: span.sourceUrl == null
+          ? null
+          : _importCache.humanize(span.sourceUrl));
 
   /// Returns a stack trace at the current point.
   ///
   /// [span] is the current location, used for the bottom-most stack frame.
   Trace _stackTrace(FileSpan span) {
-    var frames = _stack.toList()..add(_stackFrame(span));
+    var frames = _stack
+        .map((tuple) => _stackFrame(tuple.item1, tuple.item2))
+        .toList()
+          ..add(_stackFrame(_member, span));
     return new Trace(frames.reversed);
   }
 

diff --git a/pubspec.yaml b/pubspec.yaml
@@ -1,5 +1,5 @@
 name: sass
-version: 1.14.2-dev
+version: 1.14.2
 description: A Sass implementation in Dart.
 author: Dart Team <[email protected]>
 homepage: https://github.com/sass/dart-sass

diff --git a/test/cli/shared.dart b/test/cli/shared.dart
@@ -160,6 +160,31 @@ void sharedTests(Future<TestProcess> runSass(Iterable<String> arguments)) {
           ["--load-path", "dir2", "--load-path", "dir1", "test.scss"],
           equalsIgnoringWhitespace("x { y: z; }"));
     });
+
+    // Regression test for an internal Google issue.
+    test("multiple times from different load paths", () async {
+      await d.file("test.scss", """
+        @import 'parent/child/test2';
+        @import 'child/test2';
+      """).create();
+
+      await d.dir("grandparent", [
+        d.dir("parent", [
+          d.dir("child", [
+            d.file("test2.scss", "@import 'test3';"),
+            d.file("test3.scss", "a {b: c};")
+          ])
+        ])
+      ]).create();
+
+      await expectCompiles([
+        "--load-path",
+        "grandparent",
+        "--load-path",
+        "grandparent/parent",
+        "test.scss"
+      ], equalsIgnoringWhitespace("a { b: c; } a { b: c; }"));
+    });
   });
 
   group("with --stdin", () {