Merge pull request #962 from sass/watch-bug

Fix a --watch bug

CHANGELOG.md

@@ -1,3 +1,11 @@
+## 1.26.1
+
+### Command Line Interface
+
+* Fix a longstanding bug where `--watch` mode could enter into a state where
+  recompilation would not occur after a syntax error was introduced into a
+  dependency and then fixed.
+
 ## 1.26.0
 
 * **Potentially breaking bug fix:** `@use` rules whose URLs' basenames begin

lib/src/async_import_cache.dart

@@ -112,7 +112,8 @@ class AsyncImportCache {
   /// canonicalize [url] (resolved relative to [baseUrl] if it's passed).
   ///
   /// If any importers understand [url], returns that importer as well as the
-  /// canonicalized URL. Otherwise, returns `null`.
+  /// canonicalized URL and the original URL resolved relative to [baseUrl] if
+  /// applicable. Otherwise, returns `null`.
   Future<Tuple3<AsyncImporter, Uri, Uri>> canonicalize(Uri url,
       {AsyncImporter baseImporter, Uri baseUrl, bool forImport = false}) async {
     if (baseImporter != null) {

lib/src/executable/watch.dart

@@ -5,7 +5,6 @@
 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';
@@ -44,7 +43,8 @@ Future<void> watch(ExecutableOptions options, StylesheetGraph graph) async {
   for (var source in options.sourcesToDestinations.keys) {
     var destination = options.sourcesToDestinations[source];
     graph.addCanonical(FilesystemImporter('.'), p.toUri(p.canonicalize(source)),
-        p.toUri(source));
+        p.toUri(source),
+        recanonicalize: false);
     var success = await watcher.compile(source, destination, ifModified: true);
     if (!success && options.stopOnError) {
       dirWatcher.events.listen(null).cancel();
@@ -165,16 +165,12 @@ class _Watcher {
   ///
   /// Returns whether all necessary recompilations succeeded.
   Future<bool> _handleAdd(String path) async {
-    var success = await _retryPotentialImports(path);
-    if (!success && _options.stopOnError) return false;
-
     var destination = _destinationFor(path);
-    if (destination == null) return true;
 
-    _graph.addCanonical(
+    var success = destination == null || await compile(path, destination);
+    var downstream = _graph.addCanonical(
         FilesystemImporter('.'), _canonicalize(path), p.toUri(path));
-
-    return await compile(path, destination);
+    return await _recompileDownstream(downstream) && success;
   }
 
   /// Handles a remove event for the stylesheet at [url].
@@ -182,15 +178,13 @@ class _Watcher {
   /// Returns whether all necessary recompilations succeeded.
   Future<bool> _handleRemove(String path) async {
     var url = _canonicalize(path);
-    var success = await _retryPotentialImports(path);
-    if (!success && _options.stopOnError) return false;
-    if (!_graph.nodes.containsKey(url)) return true;
 
-    var destination = _destinationFor(path);
-    if (destination != null) _delete(destination);
+    if (_graph.nodes.containsKey(url)) {
+      var destination = _destinationFor(path);
+      if (destination != null) _delete(destination);
+    }
 
-    var downstream = _graph.nodes[url].downstream;
-    _graph.remove(url);
+    var downstream = _graph.remove(FilesystemImporter('.'), url);
     return await _recompileDownstream(downstream);
   }
 
@@ -278,56 +272,4 @@ class _Watcher {
 
     return null;
   }
-
-  /// Re-runs all imports in [_graph] that might refer to [path], and recompiles
-  /// the files that contain those imports if they end up importing new
-  /// stylesheets.
-  ///
-  /// Returns whether all recompilations succeeded.
-  Future<bool> _retryPotentialImports(String path) async {
-    var name = _name(p.basename(path));
-    var changed = <StylesheetNode>[];
-    for (var node in _graph.nodes.values) {
-      var importChanged = false;
-      void recanonicalize(Uri url, StylesheetNode upstream,
-          {@required bool forImport}) {
-        if (_name(p.url.basename(url.path)) != name) return;
-        _graph.clearCanonicalize(url);
-
-        // If the import produces a different canonicalized URL than it did
-        // before, it changed and the stylesheet needs to be recompiled.
-        if (!importChanged) {
-          Uri newCanonicalUrl;
-          try {
-            newCanonicalUrl = _graph.importCache
-                .canonicalize(url,
-                    baseImporter: node.importer,
-                    baseUrl: node.canonicalUrl,
-                    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.
-          }
-          importChanged = newCanonicalUrl != upstream?.canonicalUrl;
-        }
-      }
-
-      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);
-    }
-
-    return await _recompileDownstream(changed);
-  }
-
-  /// Removes an extension from [extension], and a leading underscore if it has one.
-  String _name(String basename) {
-    basename = p.withoutExtension(basename);
-    return basename.startsWith("_") ? basename.substring(1) : basename;
-  }
 }

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: 3ca2f221c3c1503c688be49d4f7501bd9be8031a
+// Checksum: c1dd0f2a41b03bd1227a2358688bf44499b9d90c
 //
 // ignore_for_file: unused_import
 
@@ -116,7 +116,8 @@ class ImportCache {
   /// canonicalize [url] (resolved relative to [baseUrl] if it's passed).
   ///
   /// If any importers understand [url], returns that importer as well as the
-  /// canonicalized URL. Otherwise, returns `null`.
+  /// canonicalized URL and the original URL resolved relative to [baseUrl] if
+  /// applicable. Otherwise, returns `null`.
   Tuple3<Importer, Uri, Uri> canonicalize(Uri url,
       {Importer baseImporter, Uri baseUrl, bool forImport = false}) {
     if (baseImporter != null) {

lib/src/importer.dart

@@ -36,4 +36,6 @@ abstract class Importer extends AsyncImporter {
   ImporterResult load(Uri url);
 
   DateTime modificationTime(Uri url) => DateTime.now();
+
+  bool couldCanonicalize(Uri url, Uri canonicalUrl) => true;
 }

lib/src/importer/async.dart

@@ -107,4 +107,13 @@ abstract class AsyncImporter {
   /// If this throws an exception, the exception is ignored and the current time
   /// is used as the modification time.
   FutureOr<DateTime> modificationTime(Uri url) => DateTime.now();
+
+  /// Without accessing the filesystem, returns whether or not passing [url] to
+  /// [canonicalize] could possibly return [canonicalUrl].
+  ///
+  /// This is expected to be very efficient, and subclasses are allowed to
+  /// return false positives if it would be inefficient to determine whether
+  /// [url] would actually resolve to [canonicalUrl]. Subclasses are not allowed
+  /// to return false negatives.
+  FutureOr<bool> couldCanonicalize(Uri url, Uri canonicalUrl) => true;
 }

lib/src/importer/filesystem.dart

@@ -39,5 +39,19 @@ class FilesystemImporter extends Importer {
 
   DateTime modificationTime(Uri url) => io.modificationTime(p.fromUri(url));
 
+  bool couldCanonicalize(Uri url, Uri canonicalUrl) {
+    if (url.scheme != 'file' && url.scheme != '') return false;
+    if (canonicalUrl.scheme != 'file') return false;
+
+    var basename = p.url.basename(url.path);
+    var canonicalBasename = p.url.basename(canonicalUrl.path);
+    if (!basename.startsWith("_") && canonicalBasename.startsWith("_")) {
+      canonicalBasename = canonicalBasename.substring(1);
+    }
+
+    return basename == canonicalBasename ||
+        basename == p.url.withoutExtension(canonicalBasename);
+  }
+
   String toString() => _loadPath;
 }

lib/src/importer/no_op.dart

@@ -12,6 +12,7 @@ import 'result.dart';
 class NoOpImporter extends Importer {
   Uri canonicalize(Uri url) => null;
   ImporterResult load(Uri url) => null;
+  bool couldCanonicalize(Uri url, Uri canonicalUrl) => false;
 
   String toString() => "(unknown)";
 }

lib/src/importer/package.dart

@@ -47,5 +47,9 @@ class PackageImporter extends Importer {
   DateTime modificationTime(Uri url) =>
       _filesystemImporter.modificationTime(url);
 
+  bool couldCanonicalize(Uri url, Uri canonicalUrl) =>
+      (url.scheme == 'file' || url.scheme == 'package' || url.scheme == '') &&
+      _filesystemImporter.couldCanonicalize(Uri(path: url.path), canonicalUrl);
+
   String toString() => "package:...";
 }

lib/src/stylesheet_graph.dart

@@ -5,6 +5,8 @@
 import 'dart:collection';
 
 import 'package:collection/collection.dart';
+import 'package:meta/meta.dart';
+import 'package:path/path.dart' as p;
 import 'package:tuple/tuple.dart';
 
 import 'ast/sass.dart';
@@ -69,27 +71,43 @@ class StylesheetGraph {
     var tuple = _ignoreErrors(() => importCache.canonicalize(url,
         baseImporter: baseImporter, baseUrl: baseUrl));
     if (tuple == null) return null;
-    return addCanonical(tuple.item1, tuple.item2, tuple.item3);
+
+    addCanonical(tuple.item1, tuple.item2, tuple.item3);
+    return nodes[tuple.item2];
   }
 
   /// Adds the stylesheet at the canonicalized [canonicalUrl] and all the
   /// stylesheets it imports to this graph and returns its node.
   ///
-  /// Returns `null` if [importer] can't import [canonicalUrl].
-  ///
   /// If passed, the [originalUrl] represents the URL that was canonicalized
   /// into [canonicalUrl]. It's used as the URL for the parsed stylesheet, which
   /// is in turn used in error reporting.
-  StylesheetNode addCanonical(Importer importer, Uri canonicalUrl,
-      [Uri originalUrl]) {
+  ///
+  /// Returns the set of nodes that need to be recompiled because their imports
+  /// changed as a result of this stylesheet being added. This does not include
+  /// the new stylesheet, which can be accessed via `nodes[canonicalUrl]`.
+  ///
+  /// If [recanonicalize] is `false`, this instead avoids checking downstream
+  /// nodes' imports and always returns an empty set. It should only be set to
+  /// `false` when initially adding stylesheets, not when handling future
+  /// updates.
+  Set<StylesheetNode> addCanonical(
+      Importer importer, Uri canonicalUrl, Uri originalUrl,
+      {bool recanonicalize = true}) {
+    var node = _nodes[canonicalUrl];
+    if (node != null) return const {};
+
     var stylesheet = _ignoreErrors(
         () => importCache.importCanonical(importer, canonicalUrl, originalUrl));
-    if (stylesheet == null) return null;
+    if (stylesheet == null) return const {};
 
-    return _nodes.putIfAbsent(
-        canonicalUrl,
-        () => StylesheetNode._(stylesheet, importer, canonicalUrl,
-            _upstreamNodes(stylesheet, importer, canonicalUrl)));
+    node = StylesheetNode._(stylesheet, importer, canonicalUrl,
+        _upstreamNodes(stylesheet, importer, canonicalUrl));
+    _nodes[canonicalUrl] = node;
+
+    return recanonicalize
+        ? _recanonicalizeImports(importer, canonicalUrl)
+        : const {};
   }
 
   /// Returns two maps from non-canonicalized imported URLs in [stylesheet] to
@@ -115,9 +133,9 @@ class StylesheetGraph {
   ///
   /// Throws a [StateError] if [canonicalUrl] isn't already in the dependency graph.
   ///
-  /// Removes the stylesheet from the graph entirely and returns `null` if the
-  /// stylesheet's importer can no longer import it.
-  StylesheetNode reload(Uri canonicalUrl) {
+  /// Returns `false` if the stylesheet's importer can no longer import it. The
+  /// caller is responsible for then calling [remove].
+  bool reload(Uri canonicalUrl) {
     var node = _nodes[canonicalUrl];
     if (node == null) {
       throw StateError("$canonicalUrl is not in the dependency graph.");
@@ -131,34 +149,109 @@ class StylesheetGraph {
     importCache.clearImport(canonicalUrl);
     var stylesheet = _ignoreErrors(
         () => importCache.importCanonical(node.importer, canonicalUrl));
-    if (stylesheet == null) {
-      remove(canonicalUrl);
-      return null;
-    }
+    if (stylesheet == null) return false;
     node._stylesheet = stylesheet;
 
-    node._stylesheet = stylesheet;
-    node._replaceUpstream(
-        _upstreamNodes(stylesheet, node.importer, canonicalUrl));
-    return node;
+    var upstream = _upstreamNodes(stylesheet, node.importer, canonicalUrl);
+    node._replaceUpstream(upstream.item1, upstream.item2);
+    return true;
   }
 
-  /// Removes the stylesheet at [canonicalUrl] from the stylesheet graph.
+  /// Removes the stylesheet at [canonicalUrl] (loaded by [importer]) from the
+  /// stylesheet graph.
   ///
-  /// Throws a [StateError] if [canonicalUrl] isn't already in the dependency graph.
-  void remove(Uri canonicalUrl) {
+  /// Note that [canonicalUrl] doesn't necessarily need to be in the stylesheet
+  /// graph itself. It may still be relevant to know that it's been removed,
+  /// because it could resolve import conflicts in stylesheets that *are* in the
+  /// graph.
+  ///
+  /// Returns the set of nodes that need to be recompiled because this node was
+  /// removed.
+  Set<StylesheetNode> remove(Importer importer, Uri canonicalUrl) {
     var node = _nodes.remove(canonicalUrl);
-    if (node == null) {
-      throw StateError("$canonicalUrl is not in the dependency graph.");
+    if (node != null) {
+      // Rather than spending time computing exactly which modification times
+      // should be updated, just clear the cache and let it be computed again
+      // later.
+      _transitiveModificationTimes.clear();
+      importCache.clearImport(canonicalUrl);
+      node._remove();
+    }
+
+    // We can't just recanonicalize [node.downstream] here, because it's
+    // possible that removing [node] fixed an import conflict, in which case the
+    // stylesheet with the import conflict should now be recompiled.
+    var toRecompile = _recanonicalizeImports(importer, canonicalUrl);
+    if (node != null) toRecompile.addAll(node._downstream);
+    return toRecompile;
+  }
+
+  /// Re-runs canonicalization for all URLs in the graph that could possibly
+  /// refer to [canonicalUrl] which was loaded via [Importer].
+  ///
+  /// Returns all nodes whose imports were changed.
+  Set<StylesheetNode> _recanonicalizeImports(
+      Importer importer, Uri canonicalUrl) {
+    var changed = <StylesheetNode>{};
+    for (var node in nodes.values) {
+      var newUpstream = _recanonicalizeImportsForNode(
+          node, importer, canonicalUrl,
+          forImport: false);
+      var newUpstreamImports = _recanonicalizeImportsForNode(
+          node, importer, canonicalUrl,
+          forImport: true);
+
+      if (newUpstream.isNotEmpty || newUpstreamImports.isNotEmpty) {
+        changed.add(node);
+        node._replaceUpstream(mergeMaps(node.upstream, newUpstream),
+            mergeMaps(node.upstreamImports, newUpstreamImports));
+      }
     }
 
     // Rather than spending time computing exactly which modification times
     // should be updated, just clear the cache and let it be computed again
     // later.
-    _transitiveModificationTimes.clear();
+    if (changed.isNotEmpty) _transitiveModificationTimes.clear();
 
-    importCache.clearImport(canonicalUrl);
-    node._remove();
+    return changed;
+  }
+
+  /// Re-runs canonicaliation for all URLs in [node]'s upstream nodes that could
+  /// possibly refer to [canonicalUrl] (which was loaded via [importer]) and
+  /// returns a map from differently-canonicalized URLs to their new nodes.
+  ///
+  /// If [forImport] is `true`, this re-runs canonicalization for
+  /// [node.upstreamImports]. Otherwise, it re-runs canonicalization for
+  /// [node.upstream].
+  Map<Uri, StylesheetNode> _recanonicalizeImportsForNode(
+      StylesheetNode node, Importer importer, Uri canonicalUrl,
+      {@required bool forImport}) {
+    var map = forImport ? node.upstreamImports : node.upstream;
+    var newMap = <Uri, StylesheetNode>{};
+    map.forEach((url, upstream) {
+      if (!importer.couldCanonicalize(url, canonicalUrl)) return;
+      importCache.clearCanonicalize(url);
+
+      // If the import produces a different canonicalized URL than it did
+      // before, it changed and the stylesheet needs to be recompiled.
+      Tuple3<AsyncImporter, Uri, Uri> result;
+      try {
+        result = importCache.canonicalize(url,
+            baseImporter: node.importer,
+            baseUrl: node.canonicalUrl,
+            forImport: forImport);
+      } catch (_) {
+        // If the call to canonicalize failed, we ignore the error so that
+        // it can be surfaced more gracefully when [node]'s stylesheet is
+        // recompiled.
+      }
+
+      var newCanonicalUrl = result?.item2;
+      if (newCanonicalUrl == upstream?.canonicalUrl) return;
+
+      newMap[url] = result == null ? null : nodes[result.item2];
+    });
+    return newMap;
   }
 
   /// Returns the [StylesheetNode] for the stylesheet at the given [url], which
@@ -199,17 +292,6 @@ class StylesheetGraph {
     return node;
   }
 
-  /// Clears the cached canonical version of the given [url] in [importCache].
-  ///
-  /// Also resets the cached modification times for stylesheets in the graph.
-  void clearCanonicalize(Uri url) {
-    // Rather than spending time computing exactly which modification times
-    // should be updated, just clear the cache and let it be computed again
-    // later.
-    _transitiveModificationTimes.clear();
-    importCache.clearCanonicalize(url);
-  }
-
   /// Runs [callback] and returns its result.
   ///
   /// If [callback] throws any errors, ignores them and returns `null`. This is
@@ -269,19 +351,15 @@ class StylesheetNode {
     }
   }
 
-  /// 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<Uri, StylesheetNode>, Map<Uri, StylesheetNode>> allUpstream) {
+  /// Updates [upstream] and [upstreamImports] from [newUpstream] and
+  /// [newUpstreamImports] and adjusts upstream nodes' [downstream] fields
+  /// accordingly.
+  void _replaceUpstream(Map<Uri, StylesheetNode> newUpstream,
+      Map<Uri, StylesheetNode> newUpstreamImports) {
     var oldUpstream = {...upstream.values, ...upstreamImports.values}
       ..remove(null);
-    var newUpstreamSet = {
-      ...allUpstream.item1.values,
-      ...allUpstream.item2.values
-    }..remove(null);
+    var newUpstreamSet = {...newUpstream.values, ...newUpstreamImports.values}
+      ..remove(null);
 
     for (var removed in oldUpstream.difference(newUpstreamSet)) {
       var wasRemoved = removed._downstream.remove(this);
@@ -293,8 +371,8 @@ class StylesheetNode {
       assert(wasAdded);
     }
 
-    _upstream = allUpstream.item1;
-    _upstreamImports = allUpstream.item2;
+    _upstream = newUpstream;
+    _upstreamImports = newUpstreamImports;
   }
 
   /// Removes [this] as an upstream and downstream node from all the nodes that
@@ -321,4 +399,6 @@ class StylesheetNode {
       }
     }
   }
+
+  String toString() => p.prettyUri(stylesheet.span.sourceUrl);
 }

pubspec.yaml

@@ -1,5 +1,5 @@
 name: sass
-version: 1.26.0
+version: 1.26.1
 description: A Sass implementation in Dart.
 author: Sass Team
 homepage: https://github.com/sass/dart-sass

test/cli/shared/watch.dart

@@ -233,6 +233,43 @@ void sharedTests(Future<TestProcess> runSass(Iterable<String> arguments)) {
                 .file("out.css", equalsIgnoringWhitespace("x { y: z; }"))
                 .validate();
           });
+
+          // Regression test for #550
+          test("with an error that's later fixed", () async {
+            await d.file("_other.scss", "a {b: c}").create();
+            await d.file("test.scss", "@import 'other'").create();
+
+            var sass = await watch(["test.scss:out.css"]);
+            await expectLater(
+                sass.stdout, emits('Compiled test.scss to out.css.'));
+            await expectLater(sass.stdout, _watchingForChanges);
+            await tickIfPoll();
+
+            await d.file("_other.scss", "a {b: }").create();
+            await expectLater(
+                sass.stderr, emits('Error: Expected expression.'));
+            await expectLater(
+                sass.stderr, emitsThrough(contains('test.scss 1:9')));
+            await tickIfPoll();
+
+            await d.file("_other.scss", "q {r: s}").create();
+            await expectLater(
+                sass.stdout, emits('Compiled test.scss to out.css.'));
+            await tick;
+            await d
+                .file("out.css", equalsIgnoringWhitespace("q { r: s; }"))
+                .validate();
+
+            await d.file("_other.scss", "x {y: z}").create();
+            await expectLater(
+                sass.stdout, emits('Compiled test.scss to out.css.'));
+            await tick;
+            await d
+                .file("out.css", equalsIgnoringWhitespace("x { y: z; }"))
+                .validate();
+
+            await sass.kill();
+          });
         });
 
         test("when it's deleted and re-added", () async {