Skip to content

Commit

Permalink
Implement access tracking for containingUrl (#2220)
Browse files Browse the repository at this point in the history
Co-authored-by: Natalie Weizenbaum <nweiz@google.com>
  • Loading branch information
ntkme and nex3 authored Apr 17, 2024
1 parent 821b98e commit 2a9eaad
Show file tree
Hide file tree
Showing 15 changed files with 141 additions and 72 deletions.
18 changes: 9 additions & 9 deletions lib/src/async_import_cache.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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);

Expand Down
4 changes: 3 additions & 1 deletion lib/src/embedded/importer/file.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
4 changes: 3 additions & 1 deletion lib/src/embedded/importer/host.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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 =>
Expand Down
20 changes: 10 additions & 10 deletions lib/src/import_cache.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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';
Expand Down Expand Up @@ -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);

Expand Down
15 changes: 14 additions & 1 deletion lib/src/importer/async.dart
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import 'dart:async';

import 'package:meta/meta.dart';

import 'canonicalize_context.dart';
import 'result.dart';
import 'utils.dart' as utils;

Expand Down Expand Up @@ -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.
///
Expand Down
47 changes: 47 additions & 0 deletions lib/src/importer/canonicalize_context.dart
Original file line number Diff line number Diff line change
@@ -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<T>(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);
}
8 changes: 3 additions & 5 deletions lib/src/importer/js_to_dart/async.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand All @@ -38,11 +39,8 @@ final class JSToDartAsyncImporter extends AsyncImporter {
}

FutureOr<Uri?> 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;

Expand Down
10 changes: 3 additions & 7 deletions lib/src/importer/js_to_dart/async_file.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -28,11 +27,8 @@ final class JSToDartAsyncFileImporter extends AsyncImporter {
FutureOr<Uri?> 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)) {
Expand Down
10 changes: 3 additions & 7 deletions lib/src/importer/js_to_dart/file.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)) {
Expand Down
8 changes: 3 additions & 5 deletions lib/src/importer/js_to_dart/sync.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);

Expand Down
38 changes: 22 additions & 16 deletions lib/src/importer/utils.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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.
///
Expand All @@ -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>(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<T>(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<T>(CanonicalizeContext? context, T callback()) =>
runZoned(callback, zoneValues: {#_canonicalizeContext: context});

/// Resolves an imported path using the same logic as the filesystem importer.
///
Expand Down
2 changes: 2 additions & 0 deletions lib/src/js.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -64,6 +65,7 @@ void main() {
"dart2js\t${const String.fromEnvironment('dart-version')}\t"
"(Dart Compiler)\t[Dart]";

updateCanonicalizeContextPrototype();
updateSourceSpanPrototype();

// Legacy API
Expand Down
10 changes: 1 addition & 9 deletions lib/src/js/importer.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import 'package:js/js.dart';

import '../importer/canonicalize_context.dart';
import 'url.dart';

@JS()
Expand All @@ -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 {
Expand Down
16 changes: 16 additions & 0 deletions lib/src/js/importer/canonicalize_context.dart
Original file line number Diff line number Diff line change
@@ -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),
});
Loading

0 comments on commit 2a9eaad

Please sign in to comment.