Produce better stack traces for syntax errors loaded via import/use (#…

…722)

We now wrap _withStackFrame() around wider sections of code, including
_loadStylesheet() which handles parse errors, so that the @use/@import
stack frames are available.

CHANGELOG.md

@@ -1,5 +1,8 @@
 ## 1.22.0
 
+* Produce better stack traces when importing a file that contains a syntax
+  error.
+
 ### Dart API
 
 * Add a `Value.realNull` getter, which returns Dart's `null` if the value is

lib/src/visitor/async_evaluate.dart

@@ -439,25 +439,31 @@ class _EvaluateVisitor
       return;
     }
 
-    var result = await inUseRuleAsync(
-        () => _loadStylesheet(url.toString(), nodeForSpan.span));
-    var importer = result.item1;
-    var stylesheet = result.item2;
+    await _withStackFrame(stackFrame, nodeForSpan, () async {
+      var result = await inUseRuleAsync(
+          () => _loadStylesheet(url.toString(), nodeForSpan.span));
+      var importer = result.item1;
+      var stylesheet = result.item2;
+
+      var canonicalUrl = stylesheet.span.sourceUrl;
+      if (_activeModules.contains(canonicalUrl)) {
+        throw _exception("Module loop: this module is already being loaded.");
+      }
+      _activeModules.add(canonicalUrl);
 
-    var canonicalUrl = stylesheet.span.sourceUrl;
-    if (_activeModules.contains(canonicalUrl)) {
-      throw _exception("Module loop: this module is already being loaded.",
-          nodeForSpan.span);
-    }
-    _activeModules.add(canonicalUrl);
+      Module module;
+      try {
+        module = await _execute(importer, stylesheet);
+      } finally {
+        _activeModules.remove(canonicalUrl);
+      }
 
-    var module = await _withStackFrame(
-        stackFrame, nodeForSpan, () => _execute(importer, stylesheet));
-    try {
-      await _addExceptionSpan(nodeForSpan, () => callback(module));
-    } finally {
-      _activeModules.remove(canonicalUrl);
-    }
+      try {
+        await callback(module);
+      } on SassScriptException catch (error) {
+        throw _exception(error.message);
+      }
+    });
   }
 
   /// Executes [stylesheet], loaded by [importer], to produce a module.
@@ -1089,25 +1095,25 @@ class _EvaluateVisitor
   }
 
   /// Adds the stylesheet imported by [import] to the current document.
-  Future<void> _visitDynamicImport(DynamicImport import) async {
-    var result = await _loadStylesheet(import.url, import.span);
-    var importer = result.item1;
-    var stylesheet = result.item2;
-
-    var url = stylesheet.span.sourceUrl;
-    if (!_activeModules.add(url)) {
-      throw _exception("This file is already being loaded.", import.span);
-    }
+  Future<void> _visitDynamicImport(DynamicImport import) {
+    return _withStackFrame("@import", import, () async {
+      var result = await _loadStylesheet(import.url, import.span);
+      var importer = result.item1;
+      var stylesheet = result.item2;
+
+      var url = stylesheet.span.sourceUrl;
+      if (!_activeModules.add(url)) {
+        throw _exception("This file is already being loaded.");
+      }
 
-    _activeModules.add(url);
+      _activeModules.add(url);
 
-    // TODO(nweiz): If [stylesheet] contains no `@use` or `@forward` rules, just
-    // evaluate it directly in [_root] rather than making a new
-    // [ModifiableCssStylesheet] and manually copying members.
+      // TODO(nweiz): If [stylesheet] contains no `@use` or `@forward` rules, just
+      // evaluate it directly in [_root] rather than making a new
+      // [ModifiableCssStylesheet] and manually copying members.
 
-    List<ModifiableCssNode> children;
-    var environment = _environment.global();
-    await _withStackFrame("@import", import, () async {
+      List<ModifiableCssNode> children;
+      var environment = _environment.global();
       await _withEnvironment(environment, () async {
         var oldImporter = _importer;
         var oldStylesheet = _stylesheet;
@@ -1132,30 +1138,30 @@ class _EvaluateVisitor
         _endOfImports = oldEndOfImports;
         _outOfOrderImports = oldOutOfOrderImports;
       });
-    });
-
-    // Create a dummy module with empty CSS and no extensions to make forwarded
-    // members available in the current import context and to combine all the
-    // CSS from modules used by [stylesheet].
-    var module = environment.toModule(
-        CssStylesheet(const [], stylesheet.span), Extender.empty);
-    _environment.importForwards(module);
 
-    if (module.transitivelyContainsCss) {
-      // If any transitively used module contains extensions, we need to clone
-      // all modules' CSS. Otherwise, it's possible that they'll be used or
-      // imported from another location that shouldn't have the same extensions
-      // applied.
-      await _combineCss(module, clone: module.transitivelyContainsExtensions)
-          .accept(this);
-    }
+      // Create a dummy module with empty CSS and no extensions to make forwarded
+      // members available in the current import context and to combine all the
+      // CSS from modules used by [stylesheet].
+      var module = environment.toModule(
+          CssStylesheet(const [], stylesheet.span), Extender.empty);
+      _environment.importForwards(module);
+
+      if (module.transitivelyContainsCss) {
+        // If any transitively used module contains extensions, we need to clone
+        // all modules' CSS. Otherwise, it's possible that they'll be used or
+        // imported from another location that shouldn't have the same extensions
+        // applied.
+        await _combineCss(module, clone: module.transitivelyContainsExtensions)
+            .accept(this);
+      }
 
-    var visitor = _ImportedCssVisitor(this);
-    for (var child in children) {
-      child.accept(visitor);
-    }
+      var visitor = _ImportedCssVisitor(this);
+      for (var child in children) {
+        child.accept(visitor);
+      }
 
-    _activeModules.remove(url);
+      _activeModules.remove(url);
+    });
   }
 
   /// Loads the [Stylesheet] identified by [url], or throws a
@@ -1183,16 +1189,15 @@ class _EvaluateVisitor
         throw "Can't find stylesheet to import.";
       }
     } on SassException catch (error) {
-      var frames = [...error.trace.frames, ..._stackTrace(span).frames];
-      throw SassRuntimeException(error.message, error.span, Trace(frames));
+      throw _exception(error.message, error.span);
     } catch (error) {
       String message;
       try {
         message = error.message as String;
       } catch (_) {
         message = error.toString();
       }
-      throw _exception(message, span);
+      throw _exception(message);
     } finally {
       _importSpan = null;
     }
@@ -2522,11 +2527,11 @@ class _EvaluateVisitor
 
   /// Returns a stack trace at the current point.
   ///
-  /// [span] is the current location, used for the bottom-most stack frame.
-  Trace _stackTrace(FileSpan span) {
+  /// If [span] is passed, it's used for the innermost stack frame.
+  Trace _stackTrace([FileSpan span]) {
     var frames = [
       ..._stack.map((tuple) => _stackFrame(tuple.item1, tuple.item2.span)),
-      _stackFrame(_member, span)
+      if (span != null) _stackFrame(_member, span)
     ];
     return Trace(frames.reversed);
   }
@@ -2536,9 +2541,12 @@ class _EvaluateVisitor
       _logger.warn(message,
           span: span, trace: _stackTrace(span), deprecation: deprecation);
 
-  /// Throws a [SassRuntimeException] with the given [message] and [span].
-  SassRuntimeException _exception(String message, FileSpan span) =>
-      SassRuntimeException(message, span, _stackTrace(span));
+  /// Throws a [SassRuntimeException] with the given [message].
+  ///
+  /// If [span] is passed, it's used for the innermost stack frame.
+  SassRuntimeException _exception(String message, [FileSpan span]) =>
+      SassRuntimeException(
+          message, span ?? _stack.last.item2.span, _stackTrace(span));
 
   /// Runs [callback], and adjusts any [SassFormatException] to be within
   /// [nodeWithSpan]'s source span.