Keep recompiling downstream dependencies after an error in --watch

Prior to this, the watcher handled all the logic around recompiling
stylesheets if an upstream file was deleted or added in a way that
could affect their import resolution. This left a gap where the
stylesheet graph wouldn't be aware that a newly-added file had become
upstream dependency of an existing downstream file, leading to
recompilation failures.

This commit fixes that by moving all that logic into the stylesheet
graph. The graph now has full and sole responsibility for providing a
consistent view of which stylesheets depend on one another even as the
shape of the graph changes, and the watcher is just a client of that
logic.

Closes #550

Author: nex3

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';
@@ -165,32 +164,26 @@ 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].
   ///
   /// 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 +271,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:...";
 }