From 588cd31b0ce1d64cacef5f7ec36c522e3a059d05 Mon Sep 17 00:00:00 2001 From: Jennifer Thakar Date: Thu, 26 Dec 2019 14:20:01 -0800 Subject: [PATCH 1/3] Cache separate canonical URLs for @use and @import 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. --- CHANGELOG.md | 3 ++ lib/src/async_import_cache.dart | 35 ++++++++++----- lib/src/executable/watch.dart | 10 +++-- lib/src/import_cache.dart | 33 +++++++++----- lib/src/importer/utils.dart | 45 +++++++++---------- lib/src/stylesheet_graph.dart | 34 ++++++++------ lib/src/visitor/async_evaluate.dart | 16 ++++--- lib/src/visitor/evaluate.dart | 17 +++---- ...nd_imports.dart => find_dependencies.dart} | 29 +++++++----- test/cli/shared.dart | 16 +++++++ 10 files changed, 151 insertions(+), 87 deletions(-) rename lib/src/visitor/{find_imports.dart => find_dependencies.dart} (54%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7093b4778..3599902ce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,9 @@ * Add the variables `$pi` and `$e` to the built-in "sass:math" module. +* 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 diff --git a/lib/src/async_import_cache.dart b/lib/src/async_import_cache.dart index 7385e84f1..cdf650350 100644 --- a/lib/src/async_import_cache.dart +++ b/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> _canonicalizeCache; + final Map, Tuple3> + _canonicalizeCache; /// The parsed stylesheets for each canonicalized import URL. final Map _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> 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,14 @@ class AsyncImportCache { /// Calls [importer.canonicalize] and prints a deprecation warning if it /// returns a relative URL. - Future _canonicalize(AsyncImporter importer, Uri url) async { - var result = await importer.canonicalize(url); + Future _canonicalize( + AsyncImporter importer, Uri url, bool forImport) async { + Uri result; + if (forImport) { + result = await inImportRule(() => importer.canonicalize(url)); + } else { + result = await importer.canonicalize(url); + } if (result?.scheme == '') { _logger.warn(""" Importer $importer canonicalized $url to $result. @@ -153,8 +166,9 @@ Relative canonical URLs are deprecated and will eventually be disallowed. /// /// Caches the result of the import and uses cached results if possible. Future> 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 +230,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 diff --git a/lib/src/executable/watch.dart b/lib/src/executable/watch.dart index 4df60d8c9..140145890 100644 --- a/lib/src/executable/watch.dart +++ b/lib/src/executable/watch.dart @@ -288,7 +288,8 @@ class _Watcher { var changed = []; for (var node in _graph.nodes.values) { var importChanged = false; - for (var url in node.upstream.keys) { + for (var tuple in node.upstream.keys) { + var url = tuple.item1; if (_name(p.url.basename(url.path)) != name) continue; _graph.clearCanonicalize(url); @@ -298,13 +299,16 @@ class _Watcher { Uri newCanonicalUrl; try { newCanonicalUrl = _graph.importCache - .canonicalize(url, node.importer, node.canonicalUrl) + .canonicalize(url, + baseImporter: node.importer, + baseUrl: node.canonicalUrl, + forImport: tuple.item2) ?.item2; } catch (_) { // 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 != node.upstream[tuple]?.canonicalUrl; } } if (importChanged) changed.add(node); diff --git a/lib/src/import_cache.dart b/lib/src/import_cache.dart index 9f99db637..0ed9bca89 100644 --- a/lib/src/import_cache.dart +++ b/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: e3bcbb88321ab00d5c7341fabd792633be874f07 // // 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> _canonicalizeCache; + final Map, Tuple3> _canonicalizeCache; /// The parsed stylesheets for each canonicalized import URL. final Map _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 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,13 @@ 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) { + Uri result; + if (forImport) { + result = inImportRule(() => importer.canonicalize(url)); + } else { + result = importer.canonicalize(url); + } if (result?.scheme == '') { _logger.warn(""" Importer $importer canonicalized $url to $result. @@ -158,8 +167,9 @@ Relative canonical URLs are deprecated and will eventually be disallowed. /// /// Caches the result of the import and uses cached results if possible. Tuple2 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 +230,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 diff --git a/lib/src/importer/utils.dart b/lib/src/importer/utils.dart index 7db6832b8..fa6569378 100644 --- a/lib/src/importer/utils.dart +++ b/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 callback()) { - var wasInUseRule = _inUseRule; - _inUseRule = true; +/// Runs [callback] in a context where [resolveImportPath] uses `@import` +/// semantics rather than `@use` semantics. +T inImportRule(T callback()) { + var wasInImportRule = _inImportRule; + _inImportRule = true; try { return callback(); } finally { - _inUseRule = wasInUseRule; + _inImportRule = wasInImportRule; } } -/// Like [inUseRule], but asynchronous. -Future inUseRuleAsync(Future callback()) async { - var wasInUseRule = _inUseRule; - _inUseRule = true; +/// Like [inImportRule], but asynchronous. +Future inImportRuleAsync(Future 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 if 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 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 callback()) => _inUseRule ? null : callback(); +T _ifInImport(T callback()) => _inImportRule ? callback() : null; diff --git a/lib/src/stylesheet_graph.dart b/lib/src/stylesheet_graph.dart index bfbd6e15f..989538776 100644 --- a/lib/src/stylesheet_graph.dart +++ b/lib/src/stylesheet_graph.dart @@ -5,11 +5,12 @@ import 'dart:collection'; import 'package:collection/collection.dart'; +import 'package:tuple/tuple.dart'; import 'ast/sass.dart'; import 'import_cache.dart'; import 'importer.dart'; -import 'visitor/find_imports.dart'; +import 'visitor/find_dependencies.dart'; /// A graph of the import relationships between stylesheets, available via /// [nodes]. @@ -64,8 +65,8 @@ class StylesheetGraph { /// /// Returns `null` if the import cache can't find a stylesheet at [url]. StylesheetNode _add(Uri url, [Importer baseImporter, Uri baseUrl]) { - var tuple = _ignoreErrors( - () => importCache.canonicalize(url, baseImporter, baseUrl)); + var tuple = _ignoreErrors(() => importCache.canonicalize(url, + baseImporter: baseImporter, baseUrl: baseUrl)); if (tuple == null) return null; return addCanonical(tuple.item1, tuple.item2, tuple.item3); } @@ -92,14 +93,17 @@ class StylesheetGraph { /// Returns a map from non-canonicalized imported URLs in [stylesheet], which /// appears within [baseUrl] imported by [baseImporter]. - Map _upstreamNodes( + Map, StylesheetNode> _upstreamNodes( Stylesheet stylesheet, Importer baseImporter, Uri baseUrl) { var active = {baseUrl}; - var upstreamUrls = - findImports(stylesheet).map((import) => Uri.parse(import.url)); + var tuple = findDependencies(stylesheet); + var importUrls = tuple.item2.map((import) => Uri.parse(import.url)); return { - for (var url in upstreamUrls) - url: _nodeFor(url, baseImporter, baseUrl, active) + for (var url in tuple.item1) + Tuple2(url, false): _nodeFor(url, baseImporter, baseUrl, active), + for (var url in importUrls) + Tuple2(url, true): + _nodeFor(url, baseImporter, baseUrl, active, forImport: true) }; } @@ -160,9 +164,10 @@ class StylesheetGraph { /// The [active] set should contain the canonical URLs that are currently /// being imported. It's used to detect circular imports. StylesheetNode _nodeFor( - Uri url, Importer baseImporter, Uri baseUrl, Set active) { - var tuple = _ignoreErrors( - () => importCache.canonicalize(url, baseImporter, baseUrl)); + Uri url, Importer baseImporter, Uri baseUrl, Set active, + {bool forImport = false}) { + var tuple = _ignoreErrors(() => importCache.canonicalize(url, + baseImporter: baseImporter, baseUrl: baseUrl, forImport: forImport)); // If an import fails, let the evaluator surface that error rather than // surfacing it here. @@ -237,8 +242,9 @@ class StylesheetNode { /// stylesheets those imports refer to. /// /// This may have `null` values, which indicate failed imports. - Map get upstream => UnmodifiableMapView(_upstream); - Map _upstream; + Map, StylesheetNode> get upstream => + UnmodifiableMapView(_upstream); + Map, StylesheetNode> _upstream; /// The stylesheets that import [stylesheet]. Set get downstream => UnmodifiableSetView(_downstream); @@ -253,7 +259,7 @@ class StylesheetNode { /// Sets [newUpstream] as the new value of [upstream] and adjusts upstream /// nodes' [downstream] fields accordingly. - void _replaceUpstream(Map newUpstream) { + void _replaceUpstream(Map, StylesheetNode> newUpstream) { var oldUpstream = Set.of(upstream.values)..remove(null); var newUpstreamSet = Set.of(newUpstream.values)..remove(null); diff --git a/lib/src/visitor/async_evaluate.dart b/lib/src/visitor/async_evaluate.dart index c465e5510..f3dd4d7d7 100644 --- a/lib/src/visitor/async_evaluate.dart +++ b/lib/src/visitor/async_evaluate.dart @@ -31,7 +31,6 @@ import '../functions.dart'; import '../functions/meta.dart' as meta; import '../importer.dart'; import '../importer/node.dart'; -import '../importer/utils.dart'; import '../io.dart'; import '../logger.dart'; import '../module.dart'; @@ -540,8 +539,8 @@ class _EvaluateVisitor } await _withStackFrame(stackFrame, nodeForSpan, () async { - var result = await inUseRuleAsync(() => - _loadStylesheet(url.toString(), nodeForSpan.span, baseUrl: baseUrl)); + var result = await _loadStylesheet(url.toString(), nodeForSpan.span, + baseUrl: baseUrl); var importer = result.item1; var stylesheet = result.item2; @@ -1310,7 +1309,8 @@ class _EvaluateVisitor /// Adds the stylesheet imported by [import] to the current document. Future _visitDynamicImport(DynamicImport import) { return _withStackFrame("@import", import, () async { - var result = await _loadStylesheet(import.url, import.span); + var result = + await _loadStylesheet(import.url, import.span, forImport: true); var importer = result.item1; var stylesheet = result.item2; @@ -1404,7 +1404,7 @@ class _EvaluateVisitor /// `_stylesheet.span.sourceUrl`. Future> _loadStylesheet( String url, FileSpan span, - {Uri baseUrl}) async { + {Uri baseUrl, bool forImport = false}) async { try { assert(_importSpan == null); _importSpan = span; @@ -1413,8 +1413,10 @@ class _EvaluateVisitor var stylesheet = await _importLikeNode(url); if (stylesheet != null) return Tuple2(null, stylesheet); } else { - var tuple = await _importCache.import( - Uri.parse(url), _importer, baseUrl ?? _stylesheet?.span?.sourceUrl); + var tuple = await _importCache.import(Uri.parse(url), + baseImporter: _importer, + baseUrl: baseUrl ?? _stylesheet?.span?.sourceUrl, + forImport: forImport); if (tuple != null) return tuple; } diff --git a/lib/src/visitor/evaluate.dart b/lib/src/visitor/evaluate.dart index 4b65bc7ba..de8110b76 100644 --- 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/grind/synchronize.dart for details. // -// Checksum: 3884b2f9f8775f0c3e49725bb2f647372219a149 +// Checksum: e3d7bea705bed417db9925546c076caed4465b2e // // ignore_for_file: unused_import @@ -40,7 +40,6 @@ import '../functions.dart'; import '../functions/meta.dart' as meta; import '../importer.dart'; import '../importer/node.dart'; -import '../importer/utils.dart'; import '../io.dart'; import '../logger.dart'; import '../module.dart'; @@ -544,8 +543,8 @@ class _EvaluateVisitor } _withStackFrame(stackFrame, nodeForSpan, () { - var result = inUseRule(() => - _loadStylesheet(url.toString(), nodeForSpan.span, baseUrl: baseUrl)); + var result = + _loadStylesheet(url.toString(), nodeForSpan.span, baseUrl: baseUrl); var importer = result.item1; var stylesheet = result.item2; @@ -1309,7 +1308,7 @@ class _EvaluateVisitor /// Adds the stylesheet imported by [import] to the current document. void _visitDynamicImport(DynamicImport import) { return _withStackFrame("@import", import, () { - var result = _loadStylesheet(import.url, import.span); + var result = _loadStylesheet(import.url, import.span, forImport: true); var importer = result.item1; var stylesheet = result.item2; @@ -1402,7 +1401,7 @@ class _EvaluateVisitor /// This first tries loading [url] relative to [baseUrl], which defaults to /// `_stylesheet.span.sourceUrl`. Tuple2 _loadStylesheet(String url, FileSpan span, - {Uri baseUrl}) { + {Uri baseUrl, bool forImport = false}) { try { assert(_importSpan == null); _importSpan = span; @@ -1411,8 +1410,10 @@ class _EvaluateVisitor var stylesheet = _importLikeNode(url); if (stylesheet != null) return Tuple2(null, stylesheet); } else { - var tuple = _importCache.import( - Uri.parse(url), _importer, baseUrl ?? _stylesheet?.span?.sourceUrl); + var tuple = _importCache.import(Uri.parse(url), + baseImporter: _importer, + baseUrl: baseUrl ?? _stylesheet?.span?.sourceUrl, + forImport: forImport); if (tuple != null) return tuple; } diff --git a/lib/src/visitor/find_imports.dart b/lib/src/visitor/find_dependencies.dart similarity index 54% rename from lib/src/visitor/find_imports.dart rename to lib/src/visitor/find_dependencies.dart index 2a8c8d8be..9e077c38b 100644 --- a/lib/src/visitor/find_imports.dart +++ b/lib/src/visitor/find_dependencies.dart @@ -2,21 +2,28 @@ // MIT-style license that can be found in the LICENSE file or at // https://opensource.org/licenses/MIT. +import 'package:tuple/tuple.dart'; + import '../ast/sass.dart'; import 'recursive_statement.dart'; -/// Returns a list of all [DynamicImport]s in [stylesheet]. -List findImports(Stylesheet stylesheet) => - _FindImportsVisitor().run(stylesheet); - -/// A visitor that traverses a stylesheet and records all the [DynamicImport]s -/// it contains. -class _FindImportsVisitor extends RecursiveStatementVisitor { +/// Returns two lists of dependencies for [stylesheet]. +/// +/// The first is a list of URLs from all `@use` and `@forward` rules in +/// [stylesheet]. The second is a list of all imports in [stylesheet]. +Tuple2, List> findDependencies( + Stylesheet stylesheet) => + _FindDependenciesVisitor().run(stylesheet); + +/// A visitor that traverses a stylesheet and records, all `@import`, `@use`, +/// and `@forward` rules it contains. +class _FindDependenciesVisitor extends RecursiveStatementVisitor { + final _usesAndForwards = []; final _imports = []; - List run(Stylesheet stylesheet) { + Tuple2, List> run(Stylesheet stylesheet) { visitStylesheet(stylesheet); - return _imports; + return Tuple2(_usesAndForwards, _imports); } // These can never contain imports. @@ -29,11 +36,11 @@ class _FindImportsVisitor extends RecursiveStatementVisitor { void visitSupportsCondition(SupportsCondition condition) {} void visitUseRule(UseRule node) { - _imports.add(DynamicImport(node.url.toString(), node.span)); + _usesAndForwards.add(node.url); } void visitForwardRule(ForwardRule node) { - _imports.add(DynamicImport(node.url.toString(), node.span)); + _usesAndForwards.add(node.url); } void visitImportRule(ImportRule node) { diff --git a/test/cli/shared.dart b/test/cli/shared.dart index 0106c8d78..dd3f500a0 100644 --- a/test/cli/shared.dart +++ b/test/cli/shared.dart @@ -228,6 +228,22 @@ void sharedTests( "test.scss" ], equalsIgnoringWhitespace("a { b: c; } a { b: c; }")); }); + + // Regression test for sass/dart-sass#899 + test("with both @use and @import", () async { + await d.file("test.scss", """ + @use 'library'; + @import 'library'; + """).create(); + + await d.dir("load-path", [ + d.file("_library.scss", "a { b: regular }"), + d.file("_library.import.scss", "a { b: import-only }") + ]).create(); + + await expectCompiles(["--load-path", "load-path", "test.scss"], + equalsIgnoringWhitespace("a { b: regular; } a { b: import-only; }")); + }); }); group("with --stdin", () { From 295a8f98c82c4c39f17c9767402a8d79c5bc7ab4 Mon Sep 17 00:00:00 2001 From: Jennifer Thakar Date: Fri, 27 Dec 2019 15:30:18 -0800 Subject: [PATCH 2/3] Split StylesheetNode.upstream into two maps --- lib/src/async_import_cache.dart | 9 ++--- lib/src/executable/watch.dart | 18 +++++---- lib/src/import_cache.dart | 11 ++---- lib/src/importer/utils.dart | 2 +- lib/src/stylesheet_graph.dart | 67 ++++++++++++++++++++++----------- 5 files changed, 65 insertions(+), 42 deletions(-) diff --git a/lib/src/async_import_cache.dart b/lib/src/async_import_cache.dart index cdf650350..94bc45ac3 100644 --- a/lib/src/async_import_cache.dart +++ b/lib/src/async_import_cache.dart @@ -141,12 +141,9 @@ class AsyncImportCache { /// returns a relative URL. Future _canonicalize( AsyncImporter importer, Uri url, bool forImport) async { - Uri result; - if (forImport) { - result = await inImportRule(() => importer.canonicalize(url)); - } else { - result = await importer.canonicalize(url); - } + var result = await (forImport + ? inImportRule(() => importer.canonicalize(url)) + : importer.canonicalize(url)); if (result?.scheme == '') { _logger.warn(""" Importer $importer canonicalized $url to $result. diff --git a/lib/src/executable/watch.dart b/lib/src/executable/watch.dart index 140145890..f596c0d5e 100644 --- a/lib/src/executable/watch.dart +++ b/lib/src/executable/watch.dart @@ -288,9 +288,9 @@ class _Watcher { var changed = []; for (var node in _graph.nodes.values) { var importChanged = false; - for (var tuple in node.upstream.keys) { - var url = tuple.item1; - if (_name(p.url.basename(url.path)) != name) continue; + var forImport = false; + void recanonicalize(Uri url, StylesheetNode node) { + if (_name(p.url.basename(url.path)) != name) return; _graph.clearCanonicalize(url); // If the import produces a different canonicalized URL than it did @@ -302,15 +302,19 @@ class _Watcher { .canonicalize(url, baseImporter: node.importer, baseUrl: node.canonicalUrl, - forImport: tuple.item2) + 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[tuple]?.canonicalUrl; + importChanged = newCanonicalUrl != node?.canonicalUrl; } } + + node.upstream.forEach(recanonicalize); + forImport = true; + node.upstreamImports.forEach(recanonicalize); if (importChanged) changed.add(node); } diff --git a/lib/src/import_cache.dart b/lib/src/import_cache.dart index 0ed9bca89..9613dafbd 100644 --- a/lib/src/import_cache.dart +++ b/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: e3bcbb88321ab00d5c7341fabd792633be874f07 +// Checksum: 3ca2f221c3c1503c688be49d4f7501bd9be8031a // // ignore_for_file: unused_import @@ -142,12 +142,9 @@ class ImportCache { /// Calls [importer.canonicalize] and prints a deprecation warning if it /// returns a relative URL. Uri _canonicalize(Importer importer, Uri url, bool forImport) { - Uri result; - if (forImport) { - result = inImportRule(() => importer.canonicalize(url)); - } else { - result = importer.canonicalize(url); - } + var result = (forImport + ? inImportRule(() => importer.canonicalize(url)) + : importer.canonicalize(url)); if (result?.scheme == '') { _logger.warn(""" Importer $importer canonicalized $url to $result. diff --git a/lib/src/importer/utils.dart b/lib/src/importer/utils.dart index fa6569378..8bf19a736 100644 --- a/lib/src/importer/utils.dart +++ b/lib/src/importer/utils.dart @@ -40,7 +40,7 @@ Future inImportRuleAsync(Future callback()) async { /// 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 +/// 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); diff --git a/lib/src/stylesheet_graph.dart b/lib/src/stylesheet_graph.dart index 989538776..f4b9a6190 100644 --- a/lib/src/stylesheet_graph.dart +++ b/lib/src/stylesheet_graph.dart @@ -40,7 +40,10 @@ class StylesheetGraph { DateTime transitiveModificationTime(StylesheetNode node) { return _transitiveModificationTimes.putIfAbsent(node.canonicalUrl, () { var latest = node.importer.modificationTime(node.canonicalUrl); - for (var upstream in node.upstream.values) { + for (var upstream in { + ...node.upstream.values, + ...node.upstreamImports.values + }) { // If an import is missing, always recompile so we show the user the // error. var upstreamTime = upstream == null @@ -93,18 +96,18 @@ class StylesheetGraph { /// Returns a map from non-canonicalized imported URLs in [stylesheet], which /// appears within [baseUrl] imported by [baseImporter]. - Map, StylesheetNode> _upstreamNodes( + Tuple2, Map> _upstreamNodes( Stylesheet stylesheet, Importer baseImporter, Uri baseUrl) { var active = {baseUrl}; var tuple = findDependencies(stylesheet); var importUrls = tuple.item2.map((import) => Uri.parse(import.url)); - return { + return Tuple2({ for (var url in tuple.item1) - Tuple2(url, false): _nodeFor(url, baseImporter, baseUrl, active), + url: _nodeFor(url, baseImporter, baseUrl, active) + }, { for (var url in importUrls) - Tuple2(url, true): - _nodeFor(url, baseImporter, baseUrl, active, forImport: true) - }; + url: _nodeFor(url, baseImporter, baseUrl, active, forImport: true) + }); } /// Re-parses the stylesheet at [canonicalUrl] and updates the dependency graph @@ -238,30 +241,45 @@ class StylesheetNode { /// The canonical URL of [stylesheet]. final Uri canonicalUrl; - /// A map from non-canonicalized import URLs in [stylesheet] to the + /// A map from non-canonicalized `@use` and `@forward` URLs in [stylesheet] to + /// the stylesheets those rules refer to. + /// + /// This may have `null` values, which indicate failed imports. + Map get upstream => UnmodifiableMapView(_upstream); + Map _upstream; + + /// A map from non-canonicalized `@import` URLs in [stylesheet] to the /// stylesheets those imports refer to. /// /// This may have `null` values, which indicate failed imports. - Map, StylesheetNode> get upstream => - UnmodifiableMapView(_upstream); - Map, StylesheetNode> _upstream; + Map get upstreamImports => + UnmodifiableMapView(_upstreamImports); + Map _upstreamImports; /// The stylesheets that import [stylesheet]. Set get downstream => UnmodifiableSetView(_downstream); final _downstream = {}; - StylesheetNode._( - this._stylesheet, this.importer, this.canonicalUrl, this._upstream) { - for (var node in upstream.values) { + StylesheetNode._(this._stylesheet, this.importer, this.canonicalUrl, + Tuple2, Map> allUpstream) + : _upstream = allUpstream.item1, + _upstreamImports = allUpstream.item2 { + for (var node in {...upstream.values, ...upstreamImports.values}) { if (node != null) node._downstream.add(this); } } - /// Sets [newUpstream] as the new value of [upstream] and adjusts upstream - /// nodes' [downstream] fields accordingly. - void _replaceUpstream(Map, StylesheetNode> newUpstream) { - var oldUpstream = Set.of(upstream.values)..remove(null); - var newUpstreamSet = Set.of(newUpstream.values)..remove(null); + /// Sets [newUpstream] as the new value of [upstream] and [newUpstreamImports] + /// as the new value of [upstreamImports], and adjusts upstream nodes' + /// [downstream] fields accordingly. + void _replaceUpstream( + Tuple2, Map> allUpstream) { + var oldUpstream = {...upstream.values, ...upstreamImports.values} + ..remove(null); + var newUpstreamSet = { + ...allUpstream.item1.values, + ...allUpstream.item2.values + }..remove(null); for (var removed in oldUpstream.difference(newUpstreamSet)) { var wasRemoved = removed._downstream.remove(this); @@ -273,13 +291,14 @@ class StylesheetNode { assert(wasAdded); } - _upstream = newUpstream; + _upstream = allUpstream.item1; + _upstreamImports = allUpstream.item2; } /// Removes [this] as an upstream and downstream node from all the nodes that /// import it and that it imports. void _remove() { - for (var node in upstream.values) { + for (var node in {...upstream.values, ...upstreamImports.values}) { if (node == null) continue; var wasRemoved = node._downstream.remove(this); assert(wasRemoved); @@ -292,6 +311,12 @@ class StylesheetNode { break; } } + for (var url in node._upstreamImports.keys.toList()) { + if (node._upstreamImports[url] == this) { + node._upstreamImports[url] = null; + break; + } + } } } } From c6403885b0f58fefd8bfb72e56a640747ddb4b85 Mon Sep 17 00:00:00 2001 From: Jennifer Thakar Date: Fri, 27 Dec 2019 16:58:13 -0800 Subject: [PATCH 3/3] Code review --- lib/src/executable/watch.dart | 16 +++++++++------ lib/src/stylesheet_graph.dart | 28 ++++++++++++++------------ lib/src/visitor/find_dependencies.dart | 9 ++++----- 3 files changed, 29 insertions(+), 24 deletions(-) diff --git a/lib/src/executable/watch.dart b/lib/src/executable/watch.dart index f596c0d5e..013a5e24f 100644 --- a/lib/src/executable/watch.dart +++ b/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,8 @@ class _Watcher { var changed = []; for (var node in _graph.nodes.values) { var importChanged = false; - var forImport = false; - void recanonicalize(Uri url, StylesheetNode node) { + void recanonicalize(Uri url, StylesheetNode upstream, + {@required bool forImport}) { if (_name(p.url.basename(url.path)) != name) return; _graph.clearCanonicalize(url); @@ -308,13 +309,16 @@ class _Watcher { // 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?.canonicalUrl; + importChanged = newCanonicalUrl != upstream?.canonicalUrl; } } - node.upstream.forEach(recanonicalize); - forImport = true; - node.upstreamImports.forEach(recanonicalize); + 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); } diff --git a/lib/src/stylesheet_graph.dart b/lib/src/stylesheet_graph.dart index f4b9a6190..556f8f8e2 100644 --- a/lib/src/stylesheet_graph.dart +++ b/lib/src/stylesheet_graph.dart @@ -40,10 +40,8 @@ class StylesheetGraph { DateTime transitiveModificationTime(StylesheetNode node) { return _transitiveModificationTimes.putIfAbsent(node.canonicalUrl, () { var latest = node.importer.modificationTime(node.canonicalUrl); - for (var upstream in { - ...node.upstream.values, - ...node.upstreamImports.values - }) { + for (var upstream + in node.upstream.values.followedBy(node.upstreamImports.values)) { // If an import is missing, always recompile so we show the user the // error. var upstreamTime = upstream == null @@ -94,18 +92,20 @@ class StylesheetGraph { _upstreamNodes(stylesheet, importer, canonicalUrl))); } - /// Returns a map from non-canonicalized imported URLs in [stylesheet], which - /// appears within [baseUrl] imported by [baseImporter]. + /// Returns two maps from non-canonicalized imported URLs in [stylesheet] to + /// nodes, which appears within [baseUrl] imported by [baseImporter]. + /// + /// The first map contains stylesheets depended on via `@use` and `@forward` + /// while the second map contains those depended on via `@import`. Tuple2, Map> _upstreamNodes( Stylesheet stylesheet, Importer baseImporter, Uri baseUrl) { var active = {baseUrl}; var tuple = findDependencies(stylesheet); - var importUrls = tuple.item2.map((import) => Uri.parse(import.url)); return Tuple2({ for (var url in tuple.item1) url: _nodeFor(url, baseImporter, baseUrl, active) }, { - for (var url in importUrls) + for (var url in tuple.item2) url: _nodeFor(url, baseImporter, baseUrl, active, forImport: true) }); } @@ -244,7 +244,7 @@ class StylesheetNode { /// A map from non-canonicalized `@use` and `@forward` URLs in [stylesheet] to /// the stylesheets those rules refer to. /// - /// This may have `null` values, which indicate failed imports. + /// This may have `null` values, which indicate failed loads. Map get upstream => UnmodifiableMapView(_upstream); Map _upstream; @@ -264,14 +264,16 @@ class StylesheetNode { Tuple2, Map> allUpstream) : _upstream = allUpstream.item1, _upstreamImports = allUpstream.item2 { - for (var node in {...upstream.values, ...upstreamImports.values}) { + for (var node in upstream.values.followedBy(upstreamImports.values)) { if (node != null) node._downstream.add(this); } } - /// Sets [newUpstream] as the new value of [upstream] and [newUpstreamImports] - /// as the new value of [upstreamImports], and adjusts upstream nodes' - /// [downstream] fields accordingly. + /// Updates [upstream] and [upstreamImports] from [allUpstream] and adjusts + /// upstream nodes' [downstream] fields accordingly. + /// + /// The first item in [allUpstream] replaces [upstream] while the second item + /// replaces [upstreamImports]. void _replaceUpstream( Tuple2, Map> allUpstream) { var oldUpstream = {...upstream.values, ...upstreamImports.values} diff --git a/lib/src/visitor/find_dependencies.dart b/lib/src/visitor/find_dependencies.dart index 9e077c38b..d2963ec50 100644 --- a/lib/src/visitor/find_dependencies.dart +++ b/lib/src/visitor/find_dependencies.dart @@ -11,17 +11,16 @@ import 'recursive_statement.dart'; /// /// The first is a list of URLs from all `@use` and `@forward` rules in /// [stylesheet]. The second is a list of all imports in [stylesheet]. -Tuple2, List> findDependencies( - Stylesheet stylesheet) => +Tuple2, List> findDependencies(Stylesheet stylesheet) => _FindDependenciesVisitor().run(stylesheet); /// A visitor that traverses a stylesheet and records, all `@import`, `@use`, /// and `@forward` rules it contains. class _FindDependenciesVisitor extends RecursiveStatementVisitor { final _usesAndForwards = []; - final _imports = []; + final _imports = []; - Tuple2, List> run(Stylesheet stylesheet) { + Tuple2, List> run(Stylesheet stylesheet) { visitStylesheet(stylesheet); return Tuple2(_usesAndForwards, _imports); } @@ -45,7 +44,7 @@ class _FindDependenciesVisitor extends RecursiveStatementVisitor { void visitImportRule(ImportRule node) { for (var import in node.imports) { - if (import is DynamicImport) _imports.add(import); + if (import is DynamicImport) _imports.add(Uri.parse(import.url)); } } }