danog/dart-sass
1
0
Fork 0
mirror of https://github.com/danog/dart-sass.git synced 2024-11-30 04:39:03 +01:00
Code Issues Packages Projects Releases Wiki Activity

Fix an import-resolution bug (#488)

Browse Source 
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.
This commit is contained in:
Natalie Weizenbaum 2018-10-11 15:06:26 -07:00 committed by GitHub
parent 123bc1ff68
commit 0595ac3e71
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
10 changed files with 190 additions and 65 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
CHANGELOG.md
View File

@ -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.

51
lib/src/async_import_cache.dart
View File

@ -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,7 +95,7 @@ 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);
}
@ -102,7 +103,7 @@ class AsyncImportCache {
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.

53
lib/src/import_cache.dart
View File

@ -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,7 +98,7 @@ 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);
}
@ -105,7 +106,7 @@ class ImportCache {
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.

4
lib/src/importer/async.dart
View File

@ -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,

7
lib/src/utils.dart
View File

@ -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);

24
lib/src/visitor/async_evaluate.dart
View File

@ -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);
}

26
lib/src/visitor/evaluate.dart
View File

@ -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);
}

2
pubspec.yaml
View File

@ -1,5 +1,5 @@
name: sass
version: 1.14.2-dev
version: 1.14.2
description: A Sass implementation in Dart.
author: Dart Team <misc@dartlang.org>
homepage: https://github.com/sass/dart-sass

25
test/cli/shared.dart
View File

@ -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", () {

54
test/dart_api/importer_test.dart
View File

@ -12,8 +12,9 @@ import 'package:sass/src/exception.dart';
main() {
test("uses an importer to resolve an @import", () {
var css = compileString('@import "orange";', importers: [
new _TestImporter((url) => url, (url) {
return new ImporterResult('.$url {color: $url}', indented: false);
new _TestImporter((url) => Uri.parse("u:$url"), (url) {
var color = url.path;
return new ImporterResult('.$color {color: $color}', indented: false);
})
]);
@ -22,8 +23,9 @@ main() {
test("passes the canonicalized URL to the importer", () {
var css = compileString('@import "orange";', importers: [
new _TestImporter((url) => new Uri(path: 'blue'), (url) {
return new ImporterResult('.$url {color: $url}', indented: false);
new _TestImporter((url) => Uri.parse('u:blue'), (url) {
var color = url.path;
return new ImporterResult('.$color {color: $color}', indented: false);
})
]);
@ -36,9 +38,11 @@ main() {
@import "orange";
""", importers: [
new _TestImporter(
(url) => new Uri(path: 'blue'),
(url) => Uri.parse('u:blue'),
expectAsync1((url) {
return new ImporterResult('.$url {color: $url}', indented: false);
var color = url.path;
return new ImporterResult('.$color {color: $color}',
indented: false);
}, count: 1))
]);
@ -54,26 +58,28 @@ main() {
test("resolves URLs relative to the pre-canonicalized URL", () {
var times = 0;
var css = compileString('@import "foo:bar/baz";', importers: [
new _TestImporter(
expectAsync1((url) {
times++;
if (times == 1) return new Uri(path: 'first');
var css = compileString('@import "foo:bar/baz";',
importers: [
new _TestImporter(
expectAsync1((url) {
times++;
if (times == 1) return new Uri(path: 'first');
expect(url, equals(Uri.parse('foo:bar/bang')));
return new Uri(path: 'second');
}, count: 2),
expectAsync1((url) {
return new ImporterResult(
times == 1
? '''
expect(url, equals(Uri.parse('foo:bar/bang')));
return new Uri(path: 'second');
}, count: 2),
expectAsync1((url) {
return new ImporterResult(
times == 1
? '''
.first {url: "$url"}
@import "bang";
'''
: '.second {url: "$url"}',
indented: false);
}, count: 2))
]);
: '.second {url: "$url"}',
indented: false);
}, count: 2))
],
logger: Logger.quiet);
expect(css, equalsIgnoringWhitespace('''
.first { url: "first"; }
@ -99,7 +105,7 @@ main() {
test("wraps an error in load()", () {
expect(() {
compileString('@import "orange";', importers: [
new _TestImporter((url) => url, (url) {
new _TestImporter((url) => Uri.parse("u:$url"), (url) {
throw "this import is bad actually";
})
]);
@ -114,7 +120,7 @@ main() {
test("prefers .message to .toString() for an importer error", () {
expect(() {
compileString('@import "orange";', importers: [
new _TestImporter((url) => url, (url) {
new _TestImporter((url) => Uri.parse("u:$url"), (url) {
throw new FormatException("bad format somehow");
})
]);