Warn instead of crashing for bad URLs (#63)

* Warn instead of crashing for bad URLs

* Combine warnings for division; error for module

* Error on invalid entrypoint

* Iterate based on review

* Resolve remaining comments

lib/src/migration_visitor.dart

@@ -13,6 +13,7 @@ import 'package:sass/src/ast/sass.dart';
 import 'package:sass/src/visitor/recursive_ast.dart';
 
 import 'package:meta/meta.dart';
+import 'package:source_span/source_span.dart';
 
 import 'patch.dart';
 import 'utils.dart';
@@ -34,8 +35,13 @@ abstract class MigrationVisitor extends RecursiveAstVisitor {
   /// True if dependencies should be migrated as well.
   final bool migrateDependencies;
 
+  /// Map of missing dependency URLs to the spans that import/use them.
+  Map<Uri, FileSpan> get missingDependencies =>
+      UnmodifiableMapView(_missingDependencies);
+  final _missingDependencies = <Uri, FileSpan>{};
+
   /// The patches to be applied to the stylesheet being migrated.
-  UnmodifiableListView<Patch> get patches => UnmodifiableListView(_patches);
+  List<Patch> get patches => UnmodifiableListView(_patches);
   List<Patch> _patches;
 
   MigrationVisitor({this.migrateDependencies = true});
@@ -67,9 +73,14 @@ abstract class MigrationVisitor extends RecursiveAstVisitor {
 
   /// Visits the stylesheet at [dependency], resolved relative to [source].
   @protected
-  void visitDependency(Uri dependency, Uri source) {
-    var stylesheet = parseStylesheet(source.resolveUri(dependency));
-    visitStylesheet(stylesheet);
+  void visitDependency(Uri dependency, Uri source, [FileSpan context]) {
+    var url = source.resolveUri(dependency);
+    var stylesheet = parseStylesheet(url);
+    if (stylesheet != null) {
+      visitStylesheet(stylesheet);
+    } else {
+      _missingDependencies.putIfAbsent(url, () => context);
+    }
   }
 
   /// Returns the migrated contents of this file, or null if the file does not
@@ -96,7 +107,8 @@ abstract class MigrationVisitor extends RecursiveAstVisitor {
     if (migrateDependencies) {
       for (var import in node.imports) {
         if (import is DynamicImport) {
-          visitDependency(Uri.parse(import.url), node.span.sourceUrl);
+          visitDependency(
+              Uri.parse(import.url), node.span.sourceUrl, import.span);
         }
       }
     }
@@ -108,7 +120,7 @@ abstract class MigrationVisitor extends RecursiveAstVisitor {
   visitUseRule(UseRule node) {
     super.visitUseRule(node);
     if (migrateDependencies) {
-      visitDependency(node.url, node.span.sourceUrl);
+      visitDependency(node.url, node.span.sourceUrl, node.span);
     }
   }
 }

lib/src/migrator.dart

@@ -6,6 +6,8 @@
 
 import 'package:args/command_runner.dart';
 import 'package:meta/meta.dart';
+import 'package:path/path.dart' as p;
+import 'package:source_span/source_span.dart';
 
 import 'utils.dart';
 
@@ -26,6 +28,14 @@ abstract class Migrator extends Command<Map<Uri, String>> {
   /// If true, dependencies will be migrated in addition to the entrypoints.
   bool get migrateDependencies => globalResults['migrate-deps'] as bool;
 
+  /// Map of missing dependency URLs to the spans that import/use them.
+  ///
+  /// Subclasses should add any missing dependencies to this when they are
+  /// encountered during migration. If using [MigrationVisitor], all items in
+  /// its `missingDependencies` property should be added to this after calling
+  /// `run`.
+  final missingDependencies = <Uri, FileSpan>{};
+
   /// Runs this migrator on [entrypoint] (and its dependencies, if the
   /// --migrate-deps flag is passed).
   ///
@@ -45,7 +55,13 @@ abstract class Migrator extends Command<Map<Uri, String>> {
   Map<Uri, String> run() {
     var allMigrated = Map<Uri, String>();
     for (var entrypoint in argResults.rest) {
-      var migrated = migrateFile(canonicalize(Uri.parse(entrypoint)));
+      var canonicalUrl = canonicalize(Uri.parse(entrypoint));
+      if (canonicalUrl == null) {
+        throw MigrationException(
+            "Error: Could not find Sass file at '$entrypoint'.");
+      }
+
+      var migrated = migrateFile(canonicalUrl);
       for (var file in migrated.keys) {
         if (allMigrated.containsKey(file) &&
             migrated[file] != allMigrated[file]) {
@@ -55,6 +71,33 @@ abstract class Migrator extends Command<Map<Uri, String>> {
         allMigrated[file] = migrated[file];
       }
     }
+
+    if (missingDependencies.isNotEmpty) _warnForMissingDependencies();
     return allMigrated;
   }
+
+  /// Prints warnings for any missing dependencies encountered during migration.
+  ///
+  /// By default, this prints a short warning with one line per missing
+  /// dependency.
+  ///
+  /// In verbose mode, this instead prints a full warning with the source span
+  /// for each missing dependency.
+  void _warnForMissingDependencies() {
+    if (globalResults['verbose'] as bool) {
+      for (var uri in missingDependencies.keys) {
+        emitWarning("Could not find Sass file at '${p.prettyUri(uri)}'.",
+            missingDependencies[uri]);
+      }
+    } else {
+      var count = missingDependencies.length;
+      emitWarning(
+          "$count dependenc${count == 1 ? 'y' : 'ies'} could not be found.");
+      for (var uri in missingDependencies.keys) {
+        var context = missingDependencies[uri];
+        print('  ${p.prettyUri(uri)} '
+            '@${p.prettyUri(context.sourceUrl)}:${context.start.line + 1}');
+      }
+    }
+  }
 }

lib/src/migrators/division.dart

@@ -36,9 +36,13 @@ More info: https://sass-lang.com/d/slash-div""";
   bool get isPessimistic => argResults['pessimistic'] as bool;
 
   @override
-  Map<Uri, String> migrateFile(Uri entrypoint) =>
-      _DivisionMigrationVisitor(this.isPessimistic, migrateDependencies)
-          .run(entrypoint);
+  Map<Uri, String> migrateFile(Uri entrypoint) {
+    var visitor =
+        _DivisionMigrationVisitor(this.isPessimistic, migrateDependencies);
+    var result = visitor.run(entrypoint);
+    missingDependencies.addAll(visitor.missingDependencies);
+    return result;
+  }
 }
 
 class _DivisionMigrationVisitor extends MigrationVisitor {

lib/src/migrators/module.dart

@@ -107,6 +107,20 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
     return uses.join() + results;
   }
 
+  /// Visits the stylesheet at [dependency], resolved relative to [source].
+  @override
+  void visitDependency(Uri dependency, Uri source, [FileSpan context]) {
+    var url = source.resolveUri(dependency);
+    var stylesheet = parseStylesheet(url);
+    if (stylesheet == null) {
+      throw MigrationException(
+          "Error: Could not find Sass file at '${p.prettyUri(url)}'.",
+          span: context);
+    }
+
+    visitStylesheet(stylesheet);
+  }
+
   /// Stores per-file state before visiting [node] and restores it afterwards.
   @override
   void visitStylesheet(Stylesheet node) {
@@ -270,7 +284,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
 
     var oldConfiguredVariables = _configuredVariables;
     _configuredVariables = Set();
-    visitDependency(Uri.parse(import.url), _currentUrl);
+    visitDependency(Uri.parse(import.url), _currentUrl, import.span);
     _namespaces[_lastUrl] = namespaceForPath(import.url);
 
     // Pass the variables that were configured by the importing file to `with`,

lib/src/runner.dart

@@ -13,6 +13,7 @@ import 'package:term_glyph/term_glyph.dart' as glyph;
 import 'io.dart';
 import 'migrators/division.dart';
 import 'migrators/module.dart';
+import 'utils.dart';
 
 /// A command runner that runs a migrator based on provided arguments.
 class MigratorRunner extends CommandRunner<Map<Uri, String>> {
@@ -55,8 +56,14 @@ class MigratorRunner extends CommandRunner<Map<Uri, String>> {
     if (argResults.wasParsed('unicode')) {
       glyph.ascii = !(argResults['unicode'] as bool);
     }
-
-    var migrated = await runCommand(argResults);
+    Map<Uri, String> migrated;
+    try {
+      migrated = await runCommand(argResults);
+    } on MigrationException catch (e) {
+      print(e);
+      print('Migration failed!');
+      return;
+    }
     if (migrated == null) return;
 
     if (migrated.isEmpty) {
@@ -78,7 +85,7 @@ class MigratorRunner extends CommandRunner<Map<Uri, String>> {
       for (var url in migrated.keys) {
         assert(url.scheme == null || url.scheme == "file",
             "$url is not a file path.");
-        if (argResults['verbose']) print("Mirating ${p.prettyUri(url)}");
+        if (argResults['verbose']) print("Migrating ${p.prettyUri(url)}");
         File(url.toFilePath()).writeAsStringSync(migrated[url]);
       }
     }

lib/src/utils.dart

@@ -25,7 +25,8 @@ Uri canonicalize(Uri url) => _filesystemImporter.canonicalize(url);
 
 /// Parses the file at [url] into a stylesheet.
 Stylesheet parseStylesheet(Uri url) {
-  var canonicalUrl = _filesystemImporter.canonicalize(url);
+  var canonicalUrl = canonicalize(url);
+  if (canonicalUrl == null) return null;
   var result = _filesystemImporter.load(canonicalUrl);
   return Stylesheet.parse(result.contents, result.syntax, url: canonicalUrl);
 }
@@ -58,9 +59,13 @@ Patch patchDelete(FileSpan span, {int start = 0, int end}) {
       span.file.span(span.start.offset + start, span.start.offset + end), "");
 }
 
-/// Emits a warning with [message] and [context];
-void emitWarning(String message, FileSpan context) {
-  print(context.message("WARNING - $message"));
+/// Emits a warning with [message] and optionally [context];
+void emitWarning(String message, [FileSpan context]) {
+  if (context == null) {
+    print("WARNING - $message");
+  } else {
+    print(context.message("WARNING - $message"));
+  }
 }
 
 /// An exception thrown by a migrator.

test/migrators/division/bad_paths.hrx

@@ -0,0 +1,12 @@
+<==> arguments
+--migrate-deps
+
+<==> input/entrypoint.scss
+@import "does_not_exist";
+@import "does_not_exist2";
+
+<==> log.txt
+WARNING - 2 dependencies could not be found.
+  does_not_exist @entrypoint.scss:1
+  does_not_exist2 @entrypoint.scss:2
+Nothing to migrate!

test/migrators/division/bad_paths_verbose.hrx

@@ -0,0 +1,19 @@
+<==> arguments
+--migrate-deps --verbose
+
+<==> input/entrypoint.scss
+@import "does_not_exist";
+@import "does_not_exist2";
+
+<==> log.txt
+line 1, column 9 of entrypoint.scss: WARNING - Could not find Sass file at 'does_not_exist'.
+  ,
+1 | @import "does_not_exist";
+  |         ^^^^^^^^^^^^^^^^
+  '
+line 2, column 9 of entrypoint.scss: WARNING - Could not find Sass file at 'does_not_exist2'.
+  ,
+2 | @import "does_not_exist2";
+  |         ^^^^^^^^^^^^^^^^^
+  '
+Nothing to migrate!

test/migrators/module/bad_paths.hrx

@@ -0,0 +1,11 @@
+<==> input/entrypoint.scss
+@import "does_not_exist";
+@import "does_not_exist2";
+
+<==> log.txt
+line 1, column 9 of entrypoint.scss: Error: Could not find Sass file at 'does_not_exist'.
+  ,
+1 | @import "does_not_exist";
+  |         ^^^^^^^^^^^^^^^^
+  '
+Migration failed!