Better handle filesystem importers when load paths aren't necessary (#…

…2203)

See #2199
See sass/sass#3815

CHANGELOG.md

@@ -3,8 +3,31 @@
 * Add support for nesting in plain CSS files. This is not processed by Sass at
   all; it's emitted exactly as-is in the CSS.
 
+* In certain circumstances, the current working directory was unintentionally
+  being made available as a load path. This is now deprecated. Anyone relying on
+  this should explicitly pass in `.` as a load path or `FilesystemImporter('.')`
+  as the current importer.
+
 * Add linux-riscv64 and windows-arm64 releases.
 
+### Command-Line Interface
+
+* Fix a bug where absolute `file:` URLs weren't loaded for files compiled via
+  the command line unless an unrelated load path was also passed.
+
+* Fix a bug where `--update` would always update files that were specified via
+  absolute path unless an unrelated load path was also passed.
+
+### Dart API
+
+* Add `FilesystemImporter.noLoadPath`, which is a `FilesystemImporter` that can
+  load absolute `file:` URLs and resolve URLs relative to the base file but
+  doesn't load relative URLs from a load path.
+
+* `FilesystemImporter.cwd` is now deprecated. Either use
+  `FilesystemImporter.noLoadPath` if you weren't intending to rely on the load
+  path, or `FilesystemImporter('.')` if you were.
+
 ## 1.72.0
 
 * Support adjacent `/`s without whitespace in between when parsing plain CSS

bin/sass.dart

@@ -13,6 +13,7 @@ import 'package:sass/src/executable/options.dart';
 import 'package:sass/src/executable/repl.dart';
 import 'package:sass/src/executable/watch.dart';
 import 'package:sass/src/import_cache.dart';
+import 'package:sass/src/importer/filesystem.dart';
 import 'package:sass/src/io.dart';
 import 'package:sass/src/logger/deprecation_handling.dart';
 import 'package:sass/src/stylesheet_graph.dart';
@@ -45,7 +46,7 @@ Future<void> main(List<String> args) async {
     }
 
     var graph = StylesheetGraph(ImportCache(
-        importers: options.pkgImporters,
+        importers: [...options.pkgImporters, FilesystemImporter.noLoadPath],
         loadPaths: options.loadPaths,
         // This logger is only used for handling fatal/future deprecations
         // during parsing, and is re-used across parses, so we don't want to

lib/src/deprecation.dart

@@ -69,6 +69,11 @@ enum Deprecation {
       deprecatedIn: '1.62.3',
       description: 'Passing null as alpha in the ${isJS ? 'JS' : 'Dart'} API.'),
 
+  fsImporterCwd('fs-importer-cwd',
+      deprecatedIn: '1.73.0',
+      description:
+          'Using the current working directory as an implicit load path.'),
+
   @Deprecated('This deprecation name was never actually used.')
   calcInterp('calc-interp', deprecatedIn: null),
 

lib/src/evaluation_context.dart

@@ -7,6 +7,7 @@ import 'dart:async';
 import 'package:source_span/source_span.dart';
 
 import 'deprecation.dart';
+import 'logger.dart';
 
 /// An interface that exposes information about the current Sass evaluation.
 ///
@@ -17,13 +18,16 @@ abstract interface class EvaluationContext {
   ///
   /// Throws [StateError] if there isn't a Sass stylesheet currently being
   /// evaluated.
-  static EvaluationContext get current {
-    if (Zone.current[#_evaluationContext] case EvaluationContext context) {
-      return context;
-    } else {
-      throw StateError("No Sass stylesheet is currently being evaluated.");
-    }
-  }
+  static EvaluationContext get current =>
+      currentOrNull ??
+      (throw StateError("No Sass stylesheet is currently being evaluated."));
+
+  /// The current evaluation context, or `null` if none exists.
+  static EvaluationContext? get currentOrNull =>
+      switch (Zone.current[#_evaluationContext]) {
+        EvaluationContext context => context,
+        _ => null
+      };
 
   /// Returns the span for the currently executing callable.
   ///
@@ -50,13 +54,20 @@ abstract interface class EvaluationContext {
 /// This may only be called within a custom function or importer callback.
 /// {@category Compile}
 void warn(String message, {bool deprecation = false}) =>
-    EvaluationContext.current
-        .warn(message, deprecation ? Deprecation.userAuthored : null);
+    switch (EvaluationContext.currentOrNull) {
+      var context? =>
+        context.warn(message, deprecation ? Deprecation.userAuthored : null),
+      _ when deprecation => (const Logger.stderr())
+          .warnForDeprecation(Deprecation.userAuthored, message),
+      _ => (const Logger.stderr()).warn(message)
+    };
 
 /// Prints a deprecation warning with [message] of type [deprecation].
-void warnForDeprecation(String message, Deprecation deprecation) {
-  EvaluationContext.current.warn(message, deprecation);
-}
+void warnForDeprecation(String message, Deprecation deprecation) =>
+    switch (EvaluationContext.currentOrNull) {
+      var context? => context.warn(message, deprecation),
+      _ => (const Logger.stderr()).warnForDeprecation(deprecation, message)
+    };
 
 /// Runs [callback] with [context] as [EvaluationContext.current].
 ///

lib/src/executable/compile_stylesheet.dart

@@ -73,8 +73,8 @@ Future<void> _compileStylesheetWithoutErrorHandling(ExecutableOptions options,
     try {
       if (source != null &&
           destination != null &&
-          !graph.modifiedSince(
-              p.toUri(source), modificationTime(destination), importer)) {
+          !graph.modifiedSince(p.toUri(p.absolute(source)),
+              modificationTime(destination), importer)) {
         return;
       }
     } on FileSystemException catch (_) {

lib/src/importer/filesystem.dart

@@ -5,30 +5,86 @@
 import 'package:meta/meta.dart';
 import 'package:path/path.dart' as p;
 
+import '../deprecation.dart';
+import '../evaluation_context.dart';
 import '../importer.dart';
 import '../io.dart' as io;
 import '../syntax.dart';
 import '../util/nullable.dart';
 import 'utils.dart';
 
-/// An importer that loads files from a load path on the filesystem.
+/// An importer that loads files from a load path on the filesystem, either
+/// relative to the path passed to [FilesystemImporter.new] or absolute `file:`
+/// URLs.
+///
+/// Use [FilesystemImporter.noLoadPath] to _only_ load absolute `file:` URLs and
+/// URLs relative to the current file.
 ///
 /// {@category Importer}
 @sealed
 class FilesystemImporter extends Importer {
   /// The path relative to which this importer looks for files.
-  final String _loadPath;
+  ///
+  /// If this is `null`, this importer will _only_ load absolute `file:` URLs
+  /// and URLs relative to the current file.
+  final String? _loadPath;
+
+  /// Whether loading from files from this importer's [_loadPath] is deprecated.
+  final bool _loadPathDeprecated;
 
   /// Creates an importer that loads files relative to [loadPath].
-  FilesystemImporter(String loadPath) : _loadPath = p.absolute(loadPath);
+  FilesystemImporter(String loadPath)
+      : _loadPath = p.absolute(loadPath),
+        _loadPathDeprecated = false;
+
+  FilesystemImporter._deprecated(String loadPath)
+      : _loadPath = p.absolute(loadPath),
+        _loadPathDeprecated = true;
+
+  /// Creates an importer that _only_ loads absolute `file:` URLs and URLs
+  /// relative to the current file.
+  FilesystemImporter._noLoadPath()
+      : _loadPath = null,
+        _loadPathDeprecated = false;
 
-  /// Creates an importer relative to the current working directory.
-  static final cwd = FilesystemImporter('.');
+  /// A [FilesystemImporter] that loads files relative to the current working
+  /// directory.
+  ///
+  /// Historically, this was the best default for supporting `file:` URL loads
+  /// when the load path didn't matter. However, adding the current working
+  /// directory to the load path wasn't always desirable, so it's no longer
+  /// recommended. Instead, either use [FilesystemImporter.noLoadPath] if the
+  /// load path doesn't matter, or `FilesystemImporter('.')` to explicitly
+  /// preserve the existing behavior.
+  @Deprecated(
+      "Use FilesystemImporter.noLoadPath or FilesystemImporter('.') instead.")
+  static final cwd = FilesystemImporter._deprecated('.');
+
+  /// Creates an importer that _only_ loads absolute `file:` URLsand URLs
+  /// relative to the current file.
+  static final noLoadPath = FilesystemImporter._noLoadPath();
 
   Uri? canonicalize(Uri url) {
-    if (url.scheme != 'file' && url.scheme != '') return null;
-    return resolveImportPath(p.join(_loadPath, p.fromUri(url)))
-        .andThen((resolved) => p.toUri(io.canonicalize(resolved)));
+    String? resolved;
+    if (url.scheme == 'file') {
+      resolved = resolveImportPath(p.fromUri(url));
+    } else if (url.scheme != '') {
+      return null;
+    } else if (_loadPath case var loadPath?) {
+      resolved = resolveImportPath(p.join(loadPath, p.fromUri(url)));
+
+      if (resolved != null && _loadPathDeprecated) {
+        warnForDeprecation(
+            "Using the current working directory as an implicit load path is "
+            "deprecated. Either add it as an explicit load path or importer, or "
+            "load this stylesheet from a different URL.",
+            Deprecation.fsImporterCwd);
+      }
+    } else {
+      return null;
+    }
+
+    return resolved.andThen((resolved) => p.toUri(io.canonicalize(resolved)));
   }
 
   ImporterResult? load(Uri url) {
@@ -53,5 +109,5 @@ class FilesystemImporter extends Importer {
         basename == p.url.withoutExtension(canonicalBasename);
   }
 
-  String toString() => _loadPath;
+  String toString() => _loadPath ?? '<absolute file importer>';
 }

test/cli/shared/update.dart

@@ -2,6 +2,7 @@
 // MIT-style license that can be found in the LICENSE file or at
 // https://opensource.org/licenses/MIT.
 
+import 'package:path/path.dart' as p;
 import 'package:test/test.dart';
 import 'package:test_descriptor/test_descriptor.dart' as d;
 import 'package:test_process/test_process.dart';
@@ -148,6 +149,18 @@ void sharedTests(Future<TestProcess> runSass(Iterable<String> arguments)) {
       await d.file("out.css", "x {y: z}").validate();
     });
 
+    // Regression test for #2203
+    test("whose sources weren't modified with an absolute path", () async {
+      await d.file("test.scss", "a {b: c}").create();
+      await d.file("out.css", "x {y: z}").create();
+
+      var sass = await update(["${p.absolute(d.path('test.scss'))}:out.css"]);
+      expect(sass.stdout, emitsDone);
+      await sass.shouldExit(0);
+
+      await d.file("out.css", "x {y: z}").validate();
+    });
+
     test("whose sibling was modified", () async {
       await d.file("test1.scss", "a {b: c}").create();
       await d.file("out1.css", "x {y: z}").create();

test/dart_api/logger_test.dart

@@ -227,10 +227,6 @@ void main() {
         mustBeCalled();
       }));
     });
-
-    test("throws an error outside a callback", () {
-      expect(() => warn("heck"), throwsStateError);
-    });
   });
 }