Merge pull request #349 from sass/update-fix

Fix some --update bugs

CHANGELOG.md

@@ -2,6 +2,17 @@
 
 * Produce better errors when expected tokens are missing before a closing brace.
 
+* Avoid crashing when compiling a non-partial stylesheet that exists on the
+  filesystem next to a partial with the same name.
+
+### Command-Line Interface
+
+* When using `--update`, surface errors when an import doesn't exist even if the
+  file containing the import hasn't been modified.
+
+* When compilation fails, delete the output file rather than leaving an outdated
+  version.
+
 ## 1.5.1
 
 * Fix a bug where an absolute Windows path would be considered an `input:output`

lib/src/executable.dart

@@ -62,6 +62,16 @@ main(List<String> args) async {
       try {
         await _compileStylesheet(options, graph, source, destination);
       } on SassException catch (error, stackTrace) {
+        // This is an immediately-invoked function expression to work around
+        // dart-lang/sdk#33400.
+        () {
+          try {
+            if (destination != null) deleteFile(destination);
+          } on FileSystemException {
+            // If the file doesn't exist, that's fine.
+          }
+        }();
+
         printError(error.toString(color: options.color),
             options.trace ? stackTrace : null);
 

lib/src/io/interface.dart

@@ -51,6 +51,11 @@ String readFile(String path) => null;
 /// Throws a [FileSystemException] if writing fails.
 void writeFile(String path, String contents) => null;
 
+/// Deletes the file at [path].
+///
+/// Throws a [FileSystemException] if deletion fails.
+void deleteFile(String path) => null;
+
 /// Reads from the standard input for the current process until it closes,
 /// returning the contents.
 Future<String> readStdin() async => null;

lib/src/io/node.dart

@@ -19,6 +19,7 @@ class _FS {
   external bool existsSync(String path);
   external void mkdirSync(String path);
   external _Stat statSync(String path);
+  external void unlinkSync(String path);
   external List<String> readdirSync(String path);
 }
 
@@ -114,6 +115,9 @@ _readFile(String path, [String encoding]) =>
 void writeFile(String path, String contents) =>
     _systemErrorToFileSystemException(() => _fs.writeFileSync(path, contents));
 
+void deleteFile(String path) =>
+    _systemErrorToFileSystemException(() => _fs.unlinkSync(path));
+
 Future<String> readStdin() async {
   var completer = new Completer<String>();
   String contents;

lib/src/io/vm.dart

@@ -44,6 +44,8 @@ String readFile(String path) {
 void writeFile(String path, String contents) =>
     new io.File(path).writeAsStringSync(contents);
 
+void deleteFile(String path) => new io.File(path).deleteSync();
+
 Future<String> readStdin() async {
   var completer = new Completer<String>();
   completer.complete(await io.SYSTEM_ENCODING.decodeStream(io.stdin));

lib/src/stylesheet_graph.dart

@@ -2,6 +2,8 @@
 // MIT-style license that can be found in the LICENSE file or at
 // https://opensource.org/licenses/MIT.
 
+import 'dart:collection';
+
 import 'ast/sass.dart';
 import 'import_cache.dart';
 import 'importer.dart';
@@ -33,8 +35,12 @@ class StylesheetGraph {
     DateTime transitiveModificationTime(_StylesheetNode node) {
       return _transitiveModificationTimes.putIfAbsent(node.canonicalUrl, () {
         var latest = node.importer.modificationTime(node.canonicalUrl);
-        for (var upstream in node.upstream) {
-          var upstreamTime = transitiveModificationTime(upstream);
+        for (var upstream in node.upstream.values) {
+          // If an import is missing, always recompile so we show the user the
+          // error.
+          var upstreamTime = upstream == null
+              ? new DateTime.now()
+              : transitiveModificationTime(upstream);
           if (upstreamTime.isAfter(latest)) latest = upstreamTime;
         }
         return latest;
@@ -54,27 +60,35 @@ class StylesheetGraph {
   ///
   /// Returns `null` if the import cache can't find a stylesheet at [url].
   _StylesheetNode _add(Uri url, [Importer baseImporter, Uri baseUrl]) {
-    var tuple = importCache.canonicalize(url, baseImporter, baseUrl);
+    var tuple = _ignoreErrors(
+        () => importCache.canonicalize(url, baseImporter, baseUrl));
     if (tuple == null) return null;
     var importer = tuple.item1;
     var canonicalUrl = tuple.item2;
 
     return _nodes.putIfAbsent(canonicalUrl, () {
-      var stylesheet = importCache.importCanonical(importer, canonicalUrl, url);
+      var stylesheet = _ignoreErrors(
+          () => importCache.importCanonical(importer, canonicalUrl, url));
       if (stylesheet == null) return null;
 
-      var active = new Set<Uri>.from([canonicalUrl]);
-      return new _StylesheetNode(
-          stylesheet,
-          importer,
-          canonicalUrl,
-          findImports(stylesheet)
-              .map((import) => _nodeFor(
-                  Uri.parse(import.url), importer, canonicalUrl, active))
-              .where((node) => node != null));
+      return new _StylesheetNode(stylesheet, importer, canonicalUrl,
+          _upstreamNodes(stylesheet, importer, canonicalUrl));
     });
   }
 
+  /// Returns a map from non-canonicalized imported URLs in [stylesheet], which
+  /// appears within [baseUrl] imported by [baseImporter].
+  Map<Uri, _StylesheetNode> _upstreamNodes(
+      Stylesheet stylesheet, Importer baseImporter, Uri baseUrl) {
+    var active = new Set<Uri>.from([baseUrl]);
+    var upstream = <Uri, _StylesheetNode>{};
+    for (var import in findImports(stylesheet)) {
+      var url = Uri.parse(import.url);
+      upstream[url] = _nodeFor(url, baseImporter, baseUrl, active);
+    }
+    return upstream;
+  }
+
   /// Returns the [StylesheetNode] for the stylesheet at the given [url], which
   /// appears within [baseUrl] imported by [baseImporter].
   ///
@@ -82,7 +96,8 @@ class StylesheetGraph {
   /// being imported. It's used to detect circular imports.
   _StylesheetNode _nodeFor(
       Uri url, Importer baseImporter, Uri baseUrl, Set<Uri> active) {
-    var tuple = importCache.canonicalize(url, baseImporter, baseUrl);
+    var tuple = _ignoreErrors(
+        () => importCache.canonicalize(url, baseImporter, baseUrl));
 
     // If an import fails, let the evaluator surface that error rather than
     // surfacing it here.
@@ -98,22 +113,30 @@ class StylesheetGraph {
     /// error will be produced during compilation.
     if (active.contains(canonicalUrl)) return null;
 
-    var stylesheet = importCache.importCanonical(importer, canonicalUrl, url);
+    var stylesheet = _ignoreErrors(
+        () => importCache.importCanonical(importer, canonicalUrl, url));
     if (stylesheet == null) return null;
 
     active.add(canonicalUrl);
-    var node = new _StylesheetNode(
-        stylesheet,
-        importer,
-        canonicalUrl,
-        findImports(stylesheet)
-            .map((import) =>
-                _nodeFor(Uri.parse(import.url), importer, canonicalUrl, active))
-            .where((node) => node != null));
+    var node = new _StylesheetNode(stylesheet, importer, canonicalUrl,
+        _upstreamNodes(stylesheet, importer, canonicalUrl));
     active.remove(canonicalUrl);
     _nodes[canonicalUrl] = node;
     return node;
   }
+
+  /// Runs [callback] and returns its result.
+  ///
+  /// If [callback] throws any errors, ignores them and returns `null`. This is
+  /// used to wrap calls to the import cache, since importer errors should be
+  /// surfaced by the compilation process rather than the graph.
+  T _ignoreErrors<T>(T callback()) {
+    try {
+      return callback();
+    } catch (_) {
+      return null;
+    }
+  }
 }
 
 /// A node in a [StylesheetGraph] that tracks a single stylesheet and all the
@@ -132,8 +155,11 @@ class _StylesheetNode {
   /// The canonical URL of [stylesheet].
   final Uri canonicalUrl;
 
-  /// The stylesheets that [stylesheet] imports.
-  final List<_StylesheetNode> upstream;
+  /// A map from non-canonicalized import URLs in [stylesheet] to the
+  /// stylesheets those imports refer to.
+  ///
+  /// This may have `null` values, which indicate failed imports.
+  final Map<Uri, _StylesheetNode> upstream;
 
   /// The stylesheets that import [stylesheet].
   ///
@@ -142,17 +168,17 @@ class _StylesheetNode {
   final downstream = new Set<_StylesheetNode>();
 
   _StylesheetNode(this.stylesheet, this.importer, this.canonicalUrl,
-      Iterable<_StylesheetNode> upstream)
-      : upstream = new List.unmodifiable(upstream) {
-    for (var node in upstream) {
-      node.downstream.add(this);
+      Map<Uri, _StylesheetNode> upstream)
+      : upstream = new Map.unmodifiable(upstream) {
+    for (var node in upstream.values) {
+      if (node != null) node.downstream.add(this);
     }
   }
 
   /// Removes [this] as a downstream node from all the upstream nodes that it
   /// imports.
   void remove() {
-    for (var node in upstream) {
+    for (var node in upstream.values) {
       var wasRemoved = node.downstream.remove(this);
       assert(wasRemoved);
     }

lib/src/visitor/async_evaluate.dart

@@ -309,9 +309,6 @@ class _EvaluateVisitor
           _includedFiles.add(_baseUrl.toString());
         }
       }
-
-      var canonicalUrl = await _importer?.canonicalize(_baseUrl);
-      if (canonicalUrl != null) _activeImports.add(canonicalUrl);
     }
 
     await visitStylesheet(node);

lib/src/visitor/evaluate.dart

@@ -5,7 +5,7 @@
 // DO NOT EDIT. This file was generated from async_evaluate.dart.
 // See tool/synchronize.dart for details.
 //
-// Checksum: bb333ed7c02c663af1977b3eb6c666b7bed92c1a
+// Checksum: dff931bdeda830c46fb13eac40e1d783f86c7aec
 
 import 'async_evaluate.dart' show EvaluateResult;
 export 'async_evaluate.dart' show EvaluateResult;
@@ -314,9 +314,6 @@ class _EvaluateVisitor
           _includedFiles.add(_baseUrl.toString());
         }
       }
-
-      var canonicalUrl = _importer?.canonicalize(_baseUrl);
-      if (canonicalUrl != null) _activeImports.add(canonicalUrl);
     }
 
     visitStylesheet(node);

test/cli/shared.dart

@@ -194,6 +194,21 @@ void sharedTests(Future<TestProcess> runSass(Iterable<String> arguments)) {
     await sass.shouldExit(65);
   });
 
+  test("gracefully handles a non-partial next to a partial", () async {
+    await d.file("test.scss", "a {b: c}").create();
+    await d.file("_test.scss", "x {y: z}").create();
+
+    var sass = await runSass(["test.scss"]);
+    expect(
+        sass.stdout,
+        emitsInOrder([
+          "a {",
+          "  b: c;",
+          "}",
+        ]));
+    await sass.shouldExit(0);
+  });
+
   test("emits warnings on standard error", () async {
     await d.file("test.scss", "@warn 'aw beans'").create();
 

test/cli/shared/update.dart

@@ -3,11 +3,14 @@
 // https://opensource.org/licenses/MIT.
 
 import 'dart:async';
+import 'dart:io';
 
 import 'package:test/test.dart';
 import 'package:test_descriptor/test_descriptor.dart' as d;
 import 'package:test_process/test_process.dart';
 
+import 'package:sass/src/util/path.dart';
+
 import '../../utils.dart';
 
 /// Defines test that are shared between the Dart and Node.js CLI test suites.
@@ -149,6 +152,62 @@ void sharedTests(Future<TestProcess> runSass(Iterable<String> arguments)) {
 
       await d.file("out1.css", "x {y: z}").validate();
     });
+
+    test("with a missing import", () async {
+      await d.file("test.scss", "@import 'other'").create();
+
+      var sass = await update(["test.scss:out.css"]);
+      expect(sass.stderr, emits("Error: Can't find stylesheet to import."));
+      expect(sass.stderr, emitsThrough(contains("test.scss 1:9")));
+      await sass.shouldExit(65);
+
+      await d.nothing("out.css").validate();
+    });
+
+    test("with a conflicting import", () async {
+      await d.file("test.scss", "@import 'other'").create();
+      await d.file("other.scss", "a {b: c}").create();
+      await d.file("_other.scss", "x {y: z}").create();
+
+      var sass = await update(["test.scss:out.css"]);
+      expect(sass.stderr,
+          emits("Error: It's not clear which file to import. Found:"));
+      expect(sass.stderr, emitsThrough(contains("test.scss 1:9")));
+      await sass.shouldExit(65);
+
+      await d.nothing("out.css").validate();
+    });
+  });
+
+  group("removes a CSS file", () {
+    test("when a file has an error", () async {
+      await d.file("test.scss", "a {b: c}").create();
+      await (await update(["test.scss:out.css"])).shouldExit(0);
+      await d.file("out.css", anything).validate();
+
+      await d.file("test.scss", "a {b: }").create();
+      var sass = await update(["test.scss:out.css"]);
+      expect(sass.stderr, emits("Error: Expected expression."));
+      expect(sass.stderr, emitsThrough(contains("test.scss 1:7")));
+      await sass.shouldExit(65);
+
+      await d.nothing("out.css").validate();
+    });
+
+    test("when an import is removed", () async {
+      await d.file("test.scss", "@import 'other'").create();
+      await d.file("_other.scss", "a {b: c}").create();
+      await (await update(["test.scss:out.css"])).shouldExit(0);
+      await d.file("out.css", anything).validate();
+
+      new File(p.join(d.sandbox, "_other.scss")).deleteSync();
+      var sass = await update(["test.scss:out.css"]);
+      expect(sass.stderr, emits("Error: Can't find stylesheet to import."));
+      expect(sass.stderr, emitsThrough(contains("test.scss 1:9")));
+      await sass.shouldExit(65);
+
+      await d.nothing("out.css").validate();
+    });
   });
 
   group("doesn't allow", () {