From 2a9eaadefa5583f728aa46c7db1cbd06623f0061 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E3=81=AA=E3=81=A4=E3=81=8D?= Date: Wed, 17 Apr 2024 14:33:10 -0700 Subject: [PATCH] Implement access tracking for containingUrl (#2220) Co-authored-by: Natalie Weizenbaum --- lib/src/async_import_cache.dart | 18 +++---- lib/src/embedded/importer/file.dart | 4 +- lib/src/embedded/importer/host.dart | 4 +- lib/src/import_cache.dart | 20 ++++---- lib/src/importer/async.dart | 15 +++++- lib/src/importer/canonicalize_context.dart | 47 +++++++++++++++++++ lib/src/importer/js_to_dart/async.dart | 8 ++-- lib/src/importer/js_to_dart/async_file.dart | 10 ++-- lib/src/importer/js_to_dart/file.dart | 10 ++-- lib/src/importer/js_to_dart/sync.dart | 8 ++-- lib/src/importer/utils.dart | 38 ++++++++------- lib/src/js.dart | 2 + lib/src/js/importer.dart | 10 +--- lib/src/js/importer/canonicalize_context.dart | 16 +++++++ lib/src/js/value/mixin.dart | 3 +- 15 files changed, 141 insertions(+), 72 deletions(-) create mode 100644 lib/src/importer/canonicalize_context.dart create mode 100644 lib/src/js/importer/canonicalize_context.dart diff --git a/lib/src/async_import_cache.dart b/lib/src/async_import_cache.dart index 576094cb4..8ddc66f3d 100644 --- a/lib/src/async_import_cache.dart +++ b/lib/src/async_import_cache.dart @@ -11,6 +11,7 @@ import 'package:path/path.dart' as p; import 'ast/sass.dart'; import 'deprecation.dart'; import 'importer.dart'; +import 'importer/canonicalize_context.dart'; import 'importer/no_op.dart'; import 'importer/utils.dart'; import 'io.dart'; @@ -206,18 +207,17 @@ final class AsyncImportCache { /// 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(url)) - : () => importer.canonicalize(url); - var passContainingUrl = baseUrl != null && (url.scheme == '' || await importer.isNonCanonicalScheme(url.scheme)); - var result = await withContainingUrl( - passContainingUrl ? baseUrl : null, canonicalize); - // TODO(sass/dart-sass#2208): Determine whether the containing URL was - // _actually_ accessed rather than assuming it was. - var cacheable = !passContainingUrl || importer is FilesystemImporter; + var canonicalizeContext = + CanonicalizeContext(passContainingUrl ? baseUrl : null, forImport); + + var result = await withCanonicalizeContext( + canonicalizeContext, () => importer.canonicalize(url)); + + var cacheable = + !passContainingUrl || !canonicalizeContext.wasContainingUrlAccessed; if (result == null) return (null, cacheable); diff --git a/lib/src/embedded/importer/file.dart b/lib/src/embedded/importer/file.dart index 57d97ddf9..7c4d9975c 100644 --- a/lib/src/embedded/importer/file.dart +++ b/lib/src/embedded/importer/file.dart @@ -21,10 +21,12 @@ final class FileImporter extends ImporterBase { ..importerId = _importerId ..url = url.toString() ..fromImport = fromImport; - if (containingUrl case var containingUrl?) { + if (canonicalizeContext.containingUrlWithoutMarking + case var containingUrl?) { request.containingUrl = containingUrl.toString(); } var response = dispatcher.sendFileImportRequest(request); + if (!response.containingUrlUnused) canonicalizeContext.containingUrl; switch (response.whichResult()) { case InboundMessage_FileImportResponse_Result.fileUrl: diff --git a/lib/src/embedded/importer/host.dart b/lib/src/embedded/importer/host.dart index 25245721b..e5342dc31 100644 --- a/lib/src/embedded/importer/host.dart +++ b/lib/src/embedded/importer/host.dart @@ -35,10 +35,12 @@ final class HostImporter extends ImporterBase { ..importerId = _importerId ..url = url.toString() ..fromImport = fromImport; - if (containingUrl case var containingUrl?) { + if (canonicalizeContext.containingUrlWithoutMarking + case var containingUrl?) { request.containingUrl = containingUrl.toString(); } var response = dispatcher.sendCanonicalizeRequest(request); + if (!response.containingUrlUnused) canonicalizeContext.containingUrl; return switch (response.whichResult()) { InboundMessage_CanonicalizeResponse_Result.url => diff --git a/lib/src/import_cache.dart b/lib/src/import_cache.dart index 6204971e4..7e5c941a0 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: 37dd173d676ec6cf201a25b3cca9ac81d92b1433 +// Checksum: 36bc42050cf2eb3a43f36376c4f06c1708eee777 // // ignore_for_file: unused_import @@ -18,6 +18,7 @@ import 'package:path/path.dart' as p; import 'ast/sass.dart'; import 'deprecation.dart'; import 'importer.dart'; +import 'importer/canonicalize_context.dart'; import 'importer/no_op.dart'; import 'importer/utils.dart'; import 'io.dart'; @@ -206,18 +207,17 @@ final class ImportCache { /// that result is cacheable at all. (CanonicalizeResult?, bool cacheable) _canonicalize( Importer importer, Uri url, Uri? baseUrl, bool forImport) { - var canonicalize = forImport - ? () => inImportRule(() => importer.canonicalize(url)) - : () => importer.canonicalize(url); - var passContainingUrl = baseUrl != null && (url.scheme == '' || importer.isNonCanonicalScheme(url.scheme)); - var result = - withContainingUrl(passContainingUrl ? baseUrl : null, canonicalize); - // TODO(sass/dart-sass#2208): Determine whether the containing URL was - // _actually_ accessed rather than assuming it was. - var cacheable = !passContainingUrl || importer is FilesystemImporter; + var canonicalizeContext = + CanonicalizeContext(passContainingUrl ? baseUrl : null, forImport); + + var result = withCanonicalizeContext( + canonicalizeContext, () => importer.canonicalize(url)); + + var cacheable = + !passContainingUrl || !canonicalizeContext.wasContainingUrlAccessed; if (result == null) return (null, cacheable); diff --git a/lib/src/importer/async.dart b/lib/src/importer/async.dart index d7d6951fe..d777d84f5 100644 --- a/lib/src/importer/async.dart +++ b/lib/src/importer/async.dart @@ -6,6 +6,7 @@ import 'dart:async'; import 'package:meta/meta.dart'; +import 'canonicalize_context.dart'; import 'result.dart'; import 'utils.dart' as utils; @@ -54,7 +55,19 @@ abstract class AsyncImporter { /// Outside of that context, its value is undefined and subject to change. @protected @nonVirtual - Uri? get containingUrl => utils.containingUrl; + Uri? get containingUrl => utils.canonicalizeContext.containingUrl; + + /// The canonicalize context of the stylesheet that caused the current + /// [canonicalize] invocation. + /// + /// Subclasses should only access this from within calls to [canonicalize]. + /// Outside of that context, its value is undefined and subject to change. + /// + /// @nodoc + @internal + @protected + @nonVirtual + CanonicalizeContext get canonicalizeContext => utils.canonicalizeContext; /// If [url] is recognized by this importer, returns its canonical format. /// diff --git a/lib/src/importer/canonicalize_context.dart b/lib/src/importer/canonicalize_context.dart new file mode 100644 index 000000000..e28e69e8d --- /dev/null +++ b/lib/src/importer/canonicalize_context.dart @@ -0,0 +1,47 @@ +// Copyright 2024 Google Inc. Use of this source code is governed by an +// MIT-style license that can be found in the LICENSE file or at +// https://opensource.org/licenses/MIT. + +import 'dart:async'; + +import 'package:meta/meta.dart'; + +/// Contextual information used by importers' `canonicalize` method. +@internal +final class CanonicalizeContext { + /// Whether the Sass compiler is currently evaluating an `@import` rule. + bool get fromImport => _fromImport; + bool _fromImport; + + /// The URL of the stylesheet that contains the current load. + Uri? get containingUrl { + _wasContainingUrlAccessed = true; + return _containingUrl; + } + + final Uri? _containingUrl; + + /// Returns the same value as [containingUrl], but doesn't mark it accessed. + Uri? get containingUrlWithoutMarking => _containingUrl; + + /// Whether [containingUrl] has been accessed. + /// + /// This is used to determine whether canonicalize result is cacheable. + bool get wasContainingUrlAccessed => _wasContainingUrlAccessed; + var _wasContainingUrlAccessed = false; + + /// Runs [callback] in a context with specificed [fromImport]. + T withFromImport(bool fromImport, T callback()) { + assert(Zone.current[#_canonicalizeContext] == this); + + var oldFromImport = _fromImport; + _fromImport = fromImport; + try { + return callback(); + } finally { + _fromImport = oldFromImport; + } + } + + CanonicalizeContext(this._containingUrl, this._fromImport); +} diff --git a/lib/src/importer/js_to_dart/async.dart b/lib/src/importer/js_to_dart/async.dart index 11ffbd735..c6d26cbe2 100644 --- a/lib/src/importer/js_to_dart/async.dart +++ b/lib/src/importer/js_to_dart/async.dart @@ -13,6 +13,7 @@ import '../../js/url.dart'; import '../../js/utils.dart'; import '../../util/nullable.dart'; import '../async.dart'; +import '../canonicalize_context.dart'; import '../result.dart'; import 'utils.dart'; @@ -38,11 +39,8 @@ final class JSToDartAsyncImporter extends AsyncImporter { } FutureOr canonicalize(Uri url) async { - var result = wrapJSExceptions(() => _canonicalize( - url.toString(), - CanonicalizeContext( - fromImport: fromImport, - containingUrl: containingUrl.andThen(dartToJSUrl)))); + var result = wrapJSExceptions( + () => _canonicalize(url.toString(), canonicalizeContext)); if (isPromise(result)) result = await promiseToFuture(result as Promise); if (result == null) return null; diff --git a/lib/src/importer/js_to_dart/async_file.dart b/lib/src/importer/js_to_dart/async_file.dart index 7be4b9461..95b2af908 100644 --- a/lib/src/importer/js_to_dart/async_file.dart +++ b/lib/src/importer/js_to_dart/async_file.dart @@ -8,11 +8,10 @@ import 'package:cli_pkg/js.dart'; import 'package:node_interop/js.dart'; import 'package:node_interop/util.dart'; -import '../../js/importer.dart'; import '../../js/url.dart'; import '../../js/utils.dart'; -import '../../util/nullable.dart'; import '../async.dart'; +import '../canonicalize_context.dart'; import '../filesystem.dart'; import '../result.dart'; import '../utils.dart'; @@ -28,11 +27,8 @@ final class JSToDartAsyncFileImporter extends AsyncImporter { FutureOr canonicalize(Uri url) async { if (url.scheme == 'file') return FilesystemImporter.cwd.canonicalize(url); - var result = wrapJSExceptions(() => _findFileUrl( - url.toString(), - CanonicalizeContext( - fromImport: fromImport, - containingUrl: containingUrl.andThen(dartToJSUrl)))); + var result = wrapJSExceptions( + () => _findFileUrl(url.toString(), canonicalizeContext)); if (isPromise(result)) result = await promiseToFuture(result as Promise); if (result == null) return null; if (!isJSUrl(result)) { diff --git a/lib/src/importer/js_to_dart/file.dart b/lib/src/importer/js_to_dart/file.dart index e3302f881..555c9df16 100644 --- a/lib/src/importer/js_to_dart/file.dart +++ b/lib/src/importer/js_to_dart/file.dart @@ -6,10 +6,9 @@ import 'package:cli_pkg/js.dart'; import 'package:node_interop/js.dart'; import '../../importer.dart'; -import '../../js/importer.dart'; import '../../js/url.dart'; import '../../js/utils.dart'; -import '../../util/nullable.dart'; +import '../canonicalize_context.dart'; import '../utils.dart'; /// A wrapper for a potentially-asynchronous JS API file importer that exposes @@ -23,11 +22,8 @@ final class JSToDartFileImporter extends Importer { Uri? canonicalize(Uri url) { if (url.scheme == 'file') return FilesystemImporter.cwd.canonicalize(url); - var result = wrapJSExceptions(() => _findFileUrl( - url.toString(), - CanonicalizeContext( - fromImport: fromImport, - containingUrl: containingUrl.andThen(dartToJSUrl)))); + var result = wrapJSExceptions( + () => _findFileUrl(url.toString(), canonicalizeContext)); if (result == null) return null; if (isPromise(result)) { diff --git a/lib/src/importer/js_to_dart/sync.dart b/lib/src/importer/js_to_dart/sync.dart index f69dafb35..06b91310a 100644 --- a/lib/src/importer/js_to_dart/sync.dart +++ b/lib/src/importer/js_to_dart/sync.dart @@ -10,6 +10,7 @@ import '../../js/importer.dart'; import '../../js/url.dart'; import '../../js/utils.dart'; import '../../util/nullable.dart'; +import '../canonicalize_context.dart'; import 'utils.dart'; /// A wrapper for a synchronous JS API importer that exposes it as a Dart @@ -34,11 +35,8 @@ final class JSToDartImporter extends Importer { } Uri? canonicalize(Uri url) { - var result = wrapJSExceptions(() => _canonicalize( - url.toString(), - CanonicalizeContext( - fromImport: fromImport, - containingUrl: containingUrl.andThen(dartToJSUrl)))); + var result = wrapJSExceptions( + () => _canonicalize(url.toString(), canonicalizeContext)); if (result == null) return null; if (isJSUrl(result)) return jsToDartUrl(result as JSUrl); diff --git a/lib/src/importer/utils.dart b/lib/src/importer/utils.dart index a68ae6f5e..4c5c8106c 100644 --- a/lib/src/importer/utils.dart +++ b/lib/src/importer/utils.dart @@ -7,6 +7,7 @@ import 'dart:async'; import 'package:path/path.dart' as p; import '../io.dart'; +import './canonicalize_context.dart'; /// Whether the Sass compiler is currently evaluating an `@import` rule. /// @@ -15,30 +16,35 @@ import '../io.dart'; /// 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 get fromImport => Zone.current[#_inImportRule] as bool? ?? false; - -/// The URL of the stylesheet that contains the current load. -Uri? get containingUrl => switch (Zone.current[#_containingUrl]) { +bool get fromImport => + ((Zone.current[#_canonicalizeContext] as CanonicalizeContext?) + ?.fromImport ?? + false); + +/// The CanonicalizeContext of the current load. +CanonicalizeContext get canonicalizeContext => + switch (Zone.current[#_canonicalizeContext]) { null => throw StateError( - "containingUrl may only be accessed within a call to canonicalize()."), - #_none => null, - Uri url => url, + "canonicalizeContext may only be accessed within a call to canonicalize()."), + CanonicalizeContext context => context, var value => throw StateError( - "Unexpected Zone.current[#_containingUrl] value $value.") + "Unexpected Zone.current[#_canonicalizeContext] value $value.") }; /// Runs [callback] in a context where [fromImport] returns `true` and /// [resolveImportPath] uses `@import` semantics rather than `@use` semantics. T inImportRule(T callback()) => - runZoned(callback, zoneValues: {#_inImportRule: true}); + switch (Zone.current[#_canonicalizeContext]) { + null => runZoned(callback, + zoneValues: {#_canonicalizeContext: CanonicalizeContext(null, true)}), + CanonicalizeContext context => context.withFromImport(true, callback), + var value => throw StateError( + "Unexpected Zone.current[#_canonicalizeContext] value $value.") + }; -/// Runs [callback] in a context where [containingUrl] returns [url]. -/// -/// If [when] is `false`, runs [callback] without setting [containingUrl]. -T withContainingUrl(Uri? url, T callback()) => - // Use #_none as a sentinel value so we can distinguish a containing URL - // that's set to null from one that's unset at all. - runZoned(callback, zoneValues: {#_containingUrl: url ?? #_none}); +/// Runs [callback] in the given context. +T withCanonicalizeContext(CanonicalizeContext? context, T callback()) => + runZoned(callback, zoneValues: {#_canonicalizeContext: context}); /// Resolves an imported path using the same logic as the filesystem importer. /// diff --git a/lib/src/js.dart b/lib/src/js.dart index 79bf90180..dc0384bc4 100644 --- a/lib/src/js.dart +++ b/lib/src/js.dart @@ -7,6 +7,7 @@ import 'package:js/js_util.dart'; import 'js/exception.dart'; import 'js/deprecations.dart'; import 'js/exports.dart'; +import 'js/importer/canonicalize_context.dart'; import 'js/compile.dart'; import 'js/compiler.dart'; import 'js/legacy.dart'; @@ -64,6 +65,7 @@ void main() { "dart2js\t${const String.fromEnvironment('dart-version')}\t" "(Dart Compiler)\t[Dart]"; + updateCanonicalizeContextPrototype(); updateSourceSpanPrototype(); // Legacy API diff --git a/lib/src/js/importer.dart b/lib/src/js/importer.dart index 0469737ee..09ffcd665 100644 --- a/lib/src/js/importer.dart +++ b/lib/src/js/importer.dart @@ -4,6 +4,7 @@ import 'package:js/js.dart'; +import '../importer/canonicalize_context.dart'; import 'url.dart'; @JS() @@ -15,15 +16,6 @@ class JSImporter { external Object? get nonCanonicalScheme; } -@JS() -@anonymous -class CanonicalizeContext { - external bool get fromImport; - external JSUrl? get containingUrl; - - external factory CanonicalizeContext({bool fromImport, JSUrl? containingUrl}); -} - @JS() @anonymous class JSImporterResult { diff --git a/lib/src/js/importer/canonicalize_context.dart b/lib/src/js/importer/canonicalize_context.dart new file mode 100644 index 000000000..412f21ce8 --- /dev/null +++ b/lib/src/js/importer/canonicalize_context.dart @@ -0,0 +1,16 @@ +// Copyright 2014 Google Inc. Use of this source code is governed by an +// MIT-style license that can be found in the LICENSE file or at +// https://opensource.org/licenses/MIT. + +import '../../importer/canonicalize_context.dart'; +import '../../util/nullable.dart'; +import '../reflection.dart'; +import '../utils.dart'; + +/// Adds JS members to Dart's `CanonicalizeContext` class. +void updateCanonicalizeContextPrototype() => + getJSClass(CanonicalizeContext(null, false)).defineGetters({ + 'fromImport': (CanonicalizeContext self) => self.fromImport, + 'containingUrl': (CanonicalizeContext self) => + self.containingUrl.andThen(dartToJSUrl), + }); diff --git a/lib/src/js/value/mixin.dart b/lib/src/js/value/mixin.dart index a41b394d2..cc55f3eb4 100644 --- a/lib/src/js/value/mixin.dart +++ b/lib/src/js/value/mixin.dart @@ -13,7 +13,8 @@ import '../utils.dart'; final JSClass mixinClass = () { var jsClass = createJSClass('sass.SassMixin', (Object self) { jsThrow(JsError( - 'It is not possible to construct a SassMixin through the JavaScript API')); + 'It is not possible to construct a SassMixin through the JavaScript ' + 'API')); }); getJSClass(SassMixin(Callable('f', '', (_) => sassNull)))