Make several changes to improve the reliability of --watch mode (#2444)

1. The import cache now tracks the most recent time it actually loaded
   a stylesheet, so that `--watch` mode can invalidate cached data
   if it's older than the mtime on disk. This should help catch some
   cases where the watcher information doesn't match the filesystem.

2. Rather than eagerly recompiling as soon as it knows it needs to,
   the stylesheet graph now only starts recompiling once it's finished
   processing a batch of events. This ensures that any cache
   invalidation is finished before the recompilation happens.

3. The stylesheet graph and import cache now eagerly invalidate all
   canonicalize results that could be changed by an added or removed
   file, rather than only those that are implicated by the in-memory
   ASTs. This avoids issues when the in-memory AST is stale.

Closes #2440

Author: nex3

CHANGELOG.md

@@ -1,4 +1,9 @@
-## 1.82.0-dev
+## 1.82.0
+
+### Command-Line Interface
+
+* Improve `--watch` mode reliability when making multiple changes at once, such
+  as checking out a different Git branch.
 
 * Parse the `calc-size()` function as a calculation now that it's supported in
   some browsers.

lib/src/async_import_cache.dart

@@ -69,6 +69,10 @@ final class AsyncImportCache {
   /// The import results for each canonicalized import URL.
   final _resultsCache = <Uri, ImporterResult>{};
 
+  /// A map from canonical URLs to the most recent time at which those URLs were
+  /// loaded from their importers.
+  final _loadTimes = <Uri, DateTime>{};
+
   /// Creates an import cache that resolves imports using [importers].
   ///
   /// Imports are resolved by trying, in order:
@@ -282,9 +286,11 @@ final class AsyncImportCache {
   Future<Stylesheet?> importCanonical(AsyncImporter importer, Uri canonicalUrl,
       {Uri? originalUrl}) async {
     return await putIfAbsentAsync(_importCache, canonicalUrl, () async {
+      var loadTime = DateTime.now();
       var result = await importer.load(canonicalUrl);
       if (result == null) return null;
 
+      _loadTimes[canonicalUrl] = loadTime;
       _resultsCache[canonicalUrl] = result;
       return Stylesheet.parse(result.contents, result.syntax,
           // For backwards-compatibility, relative canonical URLs are resolved
@@ -320,17 +326,31 @@ final class AsyncImportCache {
   Uri sourceMapUrl(Uri canonicalUrl) =>
       _resultsCache[canonicalUrl]?.sourceMapUrl ?? canonicalUrl;
 
-  /// Clears the cached canonical version of the given non-canonical [url].
-  ///
-  /// Has no effect if the canonical version of [url] has not been cached.
+  /// Returns the most recent time the stylesheet at [canonicalUrl] was loaded
+  /// from its importer, or `null` if it has never been loaded.
+  @internal
+  DateTime? loadTime(Uri canonicalUrl) => _loadTimes[canonicalUrl];
+
+  /// Clears all cached canonicalizations that could potentially produce
+  /// [canonicalUrl].
   ///
   /// @nodoc
   @internal
-  void clearCanonicalize(Uri url) {
-    _canonicalizeCache.remove((url, forImport: false));
-    _canonicalizeCache.remove((url, forImport: true));
-    _perImporterCanonicalizeCache.removeWhere(
-        (key, _) => key.$2 == url || _nonCanonicalRelativeUrls[key] == url);
+  Future<void> clearCanonicalize(Uri canonicalUrl) async {
+    for (var key in [..._canonicalizeCache.keys]) {
+      for (var importer in _importers) {
+        if (await importer.couldCanonicalize(key.$1, canonicalUrl)) {
+          _canonicalizeCache.remove(key);
+          break;
+        }
+      }
+    }
+
+    for (var key in [..._perImporterCanonicalizeCache.keys]) {
+      if (await key.$1.couldCanonicalize(key.$2, canonicalUrl)) {
+        _perImporterCanonicalizeCache.remove(key);
+      }
+    }
   }
 
   /// Clears the cached parse tree for the stylesheet with the given

lib/src/executable/compile_stylesheet.dart

@@ -124,6 +124,10 @@ Future<void> _compileStylesheetWithoutErrorHandling(ExecutableOptions options,
               fatalDeprecations: options.fatalDeprecations,
               futureDeprecations: options.futureDeprecations);
     } else {
+      // Double-check that all modified files (according to mtime) are actually
+      // reloaded in the graph so we don't end up with stale ASTs.
+      graph.reloadAllModified();
+
       result = source == null
           ? compileString(await readStdin(),
               syntax: syntax,

lib/src/executable/watch.dart

@@ -62,6 +62,10 @@ final class _Watcher {
   /// The graph of stylesheets being compiled.
   final StylesheetGraph _graph;
 
+  /// A map from source paths to destinations that need to be recompiled once
+  /// the current batch of events has been processed.
+  final Map<String, String> _toRecompile = {};
+
   _Watcher(this._options, this._graph);
 
   /// Deletes the file at [path] and prints a message about it.
@@ -82,70 +86,75 @@ final class _Watcher {
   ///
   /// Returns a future that will only complete if an unexpected error occurs.
   Future<void> watch(MultiDirWatcher watcher) async {
-    await for (var event in _debounceEvents(watcher.events)) {
-      var extension = p.extension(event.path);
-      if (extension != '.sass' && extension != '.scss' && extension != '.css') {
-        continue;
+    await for (var batch in _debounceEvents(watcher.events)) {
+      for (var event in batch) {
+        var extension = p.extension(event.path);
+        if (extension != '.sass' &&
+            extension != '.scss' &&
+            extension != '.css') {
+          continue;
+        }
+
+        switch (event.type) {
+          case ChangeType.MODIFY:
+            _handleModify(event.path);
+
+          case ChangeType.ADD:
+            _handleAdd(event.path);
+
+          case ChangeType.REMOVE:
+            _handleRemove(event.path);
+        }
       }
 
-      switch (event.type) {
-        case ChangeType.MODIFY:
-          var success = await _handleModify(event.path);
-          if (!success && _options.stopOnError) return;
-
-        case ChangeType.ADD:
-          var success = await _handleAdd(event.path);
-          if (!success && _options.stopOnError) return;
-
-        case ChangeType.REMOVE:
-          var success = await _handleRemove(event.path);
-          if (!success && _options.stopOnError) return;
-      }
+      var toRecompile = {..._toRecompile};
+      _toRecompile.clear();
+      var success = await compileStylesheets(_options, _graph, toRecompile,
+          ifModified: true);
+      if (!success && _options.stopOnError) return;
     }
   }
 
   /// Handles a modify event for the stylesheet at [path].
   ///
   /// Returns whether all necessary recompilations succeeded.
-  Future<bool> _handleModify(String path) async {
+  void _handleModify(String path) {
     var url = _canonicalize(path);
 
     // It's important to access the node ahead-of-time because it's possible
     // that `_graph.reload()` notices the file has been deleted and removes it
     // from the graph.
     if (_graph.nodes[url] case var node?) {
       _graph.reload(url);
-      return await _recompileDownstream([node]);
+      _recompileDownstream([node]);
     } else {
-      return _handleAdd(path);
+      _handleAdd(path);
     }
   }
 
   /// Handles an add event for the stylesheet at [url].
   ///
   /// Returns whether all necessary recompilations succeeded.
-  Future<bool> _handleAdd(String path) async {
+  void _handleAdd(String path) {
     var destination = _destinationFor(path);
-    var success = destination == null ||
-        await compileStylesheets(_options, _graph, {path: destination},
-            ifModified: true);
+    if (destination != null) _toRecompile[path] = destination;
     var downstream = _graph.addCanonical(
         FilesystemImporter.cwd, _canonicalize(path), p.toUri(path));
-    return await _recompileDownstream(downstream) && success;
+    _recompileDownstream(downstream);
   }
 
   /// Handles a remove event for the stylesheet at [url].
   ///
   /// Returns whether all necessary recompilations succeeded.
-  Future<bool> _handleRemove(String path) async {
+  void _handleRemove(String path) async {
     var url = _canonicalize(path);
 
     if (_graph.nodes.containsKey(url)) {
       if (_destinationFor(path) case var destination?) _delete(destination);
     }
 
     var downstream = _graph.remove(FilesystemImporter.cwd, url);
-    return await _recompileDownstream(downstream);
+    _recompileDownstream(downstream);
   }
 
   /// Returns the canonical URL for the stylesheet path [path].
@@ -154,9 +163,10 @@ final class _Watcher {
   /// Combine [WatchEvent]s that happen in quick succession.
   ///
   /// Otherwise, if a file is erased and then rewritten, we can end up reading
-  /// the intermediate erased version.
-  Stream<WatchEvent> _debounceEvents(Stream<WatchEvent> events) {
-    return events.debounceBuffer(Duration(milliseconds: 25)).expand((buffer) {
+  /// the intermediate erased version. This returns a stream of batches of
+  /// events that all happened in succession.
+  Stream<List<WatchEvent>> _debounceEvents(Stream<WatchEvent> events) {
+    return events.debounceBuffer(Duration(milliseconds: 25)).map((buffer) {
       var typeForPath = p.PathMap<ChangeType>();
       for (var event in buffer) {
         var oldType = typeForPath[event.path];
@@ -175,32 +185,20 @@ final class _Watcher {
     });
   }
 
-  /// Recompiles [nodes] and everything that transitively imports them, if
-  /// necessary.
-  ///
-  /// Returns whether all recompilations succeeded.
-  Future<bool> _recompileDownstream(Iterable<StylesheetNode> nodes) async {
+  /// Marks [nodes] and everything that transitively imports them for
+  /// recompilation, if necessary.
+  void _recompileDownstream(Iterable<StylesheetNode> nodes) {
     var seen = <StylesheetNode>{};
-    var allSucceeded = true;
     while (nodes.isNotEmpty) {
       nodes = [
         for (var node in nodes)
           if (seen.add(node)) node
       ];
 
-      var sourcesToDestinations = _sourceEntrypointsToDestinations(nodes);
-      if (sourcesToDestinations.isNotEmpty) {
-        var success = await compileStylesheets(
-            _options, _graph, sourcesToDestinations,
-            ifModified: true);
-        if (!success && _options.stopOnError) return false;
-
-        allSucceeded = allSucceeded && success;
-      }
+      _toRecompile.addAll(_sourceEntrypointsToDestinations(nodes));
 
       nodes = [for (var node in nodes) ...node.downstream];
     }
-    return allSucceeded;
   }
 
   /// Returns a sourcesToDestinations mapping for nodes that are entrypoints.

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: 4d09da97db4e59518d193f58963897d36ef4db00
+// Checksum: 65a7c538299527be3240f0625a7c1cd4f8cd6824
 //
 // ignore_for_file: unused_import
 
@@ -70,6 +70,10 @@ final class ImportCache {
   /// The import results for each canonicalized import URL.
   final _resultsCache = <Uri, ImporterResult>{};
 
+  /// A map from canonical URLs to the most recent time at which those URLs were
+  /// loaded from their importers.
+  final _loadTimes = <Uri, DateTime>{};
+
   /// Creates an import cache that resolves imports using [importers].
   ///
   /// Imports are resolved by trying, in order:
@@ -276,9 +280,11 @@ final class ImportCache {
   Stylesheet? importCanonical(Importer importer, Uri canonicalUrl,
       {Uri? originalUrl}) {
     return _importCache.putIfAbsent(canonicalUrl, () {
+      var loadTime = DateTime.now();
       var result = importer.load(canonicalUrl);
       if (result == null) return null;
 
+      _loadTimes[canonicalUrl] = loadTime;
       _resultsCache[canonicalUrl] = result;
       return Stylesheet.parse(result.contents, result.syntax,
           // For backwards-compatibility, relative canonical URLs are resolved
@@ -314,17 +320,31 @@ final class ImportCache {
   Uri sourceMapUrl(Uri canonicalUrl) =>
       _resultsCache[canonicalUrl]?.sourceMapUrl ?? canonicalUrl;
 
-  /// Clears the cached canonical version of the given non-canonical [url].
-  ///
-  /// Has no effect if the canonical version of [url] has not been cached.
+  /// Returns the most recent time the stylesheet at [canonicalUrl] was loaded
+  /// from its importer, or `null` if it has never been loaded.
+  @internal
+  DateTime? loadTime(Uri canonicalUrl) => _loadTimes[canonicalUrl];
+
+  /// Clears all cached canonicalizations that could potentially produce
+  /// [canonicalUrl].
   ///
   /// @nodoc
   @internal
-  void clearCanonicalize(Uri url) {
-    _canonicalizeCache.remove((url, forImport: false));
-    _canonicalizeCache.remove((url, forImport: true));
-    _perImporterCanonicalizeCache.removeWhere(
-        (key, _) => key.$2 == url || _nonCanonicalRelativeUrls[key] == url);
+  void clearCanonicalize(Uri canonicalUrl) {
+    for (var key in [..._canonicalizeCache.keys]) {
+      for (var importer in _importers) {
+        if (importer.couldCanonicalize(key.$1, canonicalUrl)) {
+          _canonicalizeCache.remove(key);
+          break;
+        }
+      }
+    }
+
+    for (var key in [..._perImporterCanonicalizeCache.keys]) {
+      if (key.$1.couldCanonicalize(key.$2, canonicalUrl)) {
+        _perImporterCanonicalizeCache.remove(key);
+      }
+    }
   }
 
   /// Clears the cached parse tree for the stylesheet with the given

lib/src/stylesheet_graph.dart

@@ -8,6 +8,7 @@ import 'package:path/path.dart' as p;
 import 'ast/sass.dart';
 import 'import_cache.dart';
 import 'importer.dart';
+import 'io.dart';
 import 'util/map.dart';
 import 'util/nullable.dart';
 import 'visitor/find_dependencies.dart';
@@ -169,6 +170,33 @@ class StylesheetGraph {
     return true;
   }
 
+  /// Re-parses all stylesheets in the graph that have been modified on disk
+  /// since their last known in-memory modification.
+  ///
+  /// This guards against situations where a recompilation is triggered before
+  /// the graph is manually informed of all changes, such as when `--poll` runs
+  /// slowly or native file system notifications aren't comprehensive.
+  void reloadAllModified() {
+    // Copy to a list because [reload] can modify [_nodes].
+    for (var node in [..._nodes.values]) {
+      var modified = false;
+      try {
+        var loadTime = importCache.loadTime(node.canonicalUrl);
+        modified = loadTime != null &&
+            node.importer.modificationTime(node.canonicalUrl).isAfter(loadTime);
+      } on FileSystemException catch (_) {
+        // If the file no longer exists, treat that as a modification.
+        modified = true;
+      }
+
+      if (modified) {
+        if (!reload(node.canonicalUrl)) {
+          remove(node.importer, node.canonicalUrl);
+        }
+      }
+    }
+  }
+
   /// Removes the stylesheet at [canonicalUrl] (loaded by [importer]) from the
   /// stylesheet graph.
   ///
@@ -204,6 +232,7 @@ class StylesheetGraph {
   /// Returns all nodes whose imports were changed.
   Set<StylesheetNode> _recanonicalizeImports(
       Importer importer, Uri canonicalUrl) {
+    importCache.clearCanonicalize(canonicalUrl);
     var changed = <StylesheetNode>{};
     for (var node in nodes.values) {
       var newUpstream = _recanonicalizeImportsForNode(
@@ -242,7 +271,6 @@ class StylesheetGraph {
     var newMap = <Uri, StylesheetNode?>{};
     for (var (url, upstream) in map.pairs) {
       if (!importer.couldCanonicalize(url, canonicalUrl)) continue;
-      importCache.clearCanonicalize(url);
 
       // If the import produces a different canonicalized URL than it did
       // before, it changed and the stylesheet needs to be recompiled.

pkg/sass-parser/CHANGELOG.md

@@ -1,4 +1,4 @@
-## 0.4.7-dev
+## 0.4.7
 
 * No user-visible changes.
 

pkg/sass-parser/package.json

@@ -1,6 +1,6 @@
 {
   "name": "sass-parser",
-  "version": "0.4.7-dev",
+  "version": "0.4.7",
   "description": "A PostCSS-compatible wrapper of the official Sass parser",
   "repository": "sass/sass",
   "author": "Google Inc.",

pkg/sass_api/CHANGELOG.md

@@ -1,4 +1,4 @@
-## 14.4.0-dev
+## 14.4.0
 
 * No user-visible changes.