Track stack traces through rethrows (#1545)

This requires a lot of manual machinery when displaying stack traces to the user, but it should make debugging errors (especially in JS) much easier. Works around dart-lang/sdk#10297 using expandos.
sass · Nov 5, 2021 · 55cb4fd · 55cb4fd
1 parent 6622eeb
commit 55cb4fd
Show file tree

Hide file tree

Showing 22 changed files with 336 additions and 168 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,3 +1,10 @@
+## 1.43.5
+
+### JS API
+
+* Print more detailed JS stack traces. This is mostly useful for the Sass team's
+  own debugging purposes.
+
 ## 1.43.4
 
 ### JS API

diff --git a/bin/sass.dart b/bin/sass.dart
@@ -16,6 +16,7 @@ import 'package:sass/src/executable/watch.dart';
 import 'package:sass/src/import_cache.dart';
 import 'package:sass/src/io.dart';
 import 'package:sass/src/stylesheet_graph.dart';
+import 'package:sass/src/utils.dart';
 
 Future<void> main(List<String> args) async {
   var printedError = false;
@@ -79,7 +80,7 @@ Future<void> main(List<String> args) async {
         }();
 
         printError(error.toString(color: options.color),
-            options.trace ? stackTrace : null);
+            options.trace ? getTrace(error) ?? stackTrace : null);
 
         // Exit code 65 indicates invalid data per
         // http://www.freebsd.org/cgi/man.cgi?query=sysexits.
@@ -93,7 +94,7 @@ Future<void> main(List<String> args) async {
             path == null
                 ? error.message
                 : "Error reading ${p.relative(path)}: ${error.message}.",
-            options.trace ? stackTrace : null);
+            options.trace ? getTrace(error) ?? stackTrace : null);
 
         // Error 66 indicates no input.
         exitCode = 66;
@@ -114,7 +115,7 @@ Future<void> main(List<String> args) async {
     buffer.writeln();
     buffer.writeln(error);
 
-    printError(buffer.toString(), stackTrace);
+    printError(buffer.toString(), getTrace(error) ?? stackTrace);
     exitCode = 255;
   }
 }

diff --git a/lib/src/executable/repl.dart b/lib/src/executable/repl.dart
@@ -14,6 +14,7 @@ import '../import_cache.dart';
 import '../importer/filesystem.dart';
 import '../logger/tracking.dart';
 import '../parse/parser.dart';
+import '../utils.dart';
 import '../visitor/evaluate.dart';
 
 /// Runs an interactive SassScript shell according to [options].
@@ -42,7 +43,8 @@ Future<void> repl(ExecutableOptions options) async {
         print(evaluator.evaluate(Expression.parse(line, logger: logger)));
       }
     } on SassException catch (error, stackTrace) {
-      _logError(error, stackTrace, line, repl, options, logger);
+      _logError(
+          error, getTrace(error) ?? stackTrace, line, repl, options, logger);
     }
   }
 }

diff --git a/lib/src/executable/watch.dart b/lib/src/executable/watch.dart
@@ -14,6 +14,7 @@ import '../importer/filesystem.dart';
 import '../io.dart';
 import '../stylesheet_graph.dart';
 import '../util/multi_dir_watcher.dart';
+import '../utils.dart';
 import 'compile_stylesheet.dart';
 import 'options.dart';
 
@@ -78,7 +79,8 @@ class _Watcher {
       return true;
     } on SassException catch (error, stackTrace) {
       if (!_options.emitErrorCss) _delete(destination);
-      _printError(error.toString(color: _options.color), stackTrace);
+      _printError(
+          error.toString(color: _options.color), getTrace(error) ?? stackTrace);
       exitCode = 65;
       return false;
     } on FileSystemException catch (error, stackTrace) {
@@ -87,7 +89,7 @@ class _Watcher {
           path == null
               ? error.message
               : "Error reading ${p.relative(path)}: ${error.message}.",
-          stackTrace);
+          getTrace(error) ?? stackTrace);
       exitCode = 66;
       return false;
     }

diff --git a/lib/src/extend/extension_store.dart b/lib/src/extend/extension_store.dart
@@ -187,11 +187,13 @@ class ExtensionStore {
       try {
         selector = _extendList(
             originalSelector, selectorSpan, _extensions, mediaContext);
-      } on SassException catch (error) {
-        throw SassException(
-            "From ${error.span.message('')}\n"
-            "${error.message}",
-            error.span);
+      } on SassException catch (error, stackTrace) {
+        throwWithTrace(
+            SassException(
+                "From ${error.span.message('')}\n"
+                "${error.message}",
+                error.span),
+            stackTrace);
       }
     }
 
@@ -330,11 +332,13 @@ class ExtensionStore {
         selectors = _extendComplex(extension.extender.selector,
             extension.extender.span, newExtensions, extension.mediaContext);
         if (selectors == null) continue;
-      } on SassException catch (error) {
-        throw SassException(
-            "From ${extension.extender.span.message('')}\n"
-            "${error.message}",
-            error.span);
+      } on SassException catch (error, stackTrace) {
+        throwWithTrace(
+            SassException(
+                "From ${extension.extender.span.message('')}\n"
+                "${error.message}",
+                error.span),
+            stackTrace);
       }
 
       var containsExtension = selectors.first == extension.extender.selector;
@@ -391,12 +395,14 @@ class ExtensionStore {
       try {
         selector.value = _extendList(selector.value, selector.span,
             newExtensions, _mediaContexts[selector]);
-      } on SassException catch (error) {
+      } on SassException catch (error, stackTrace) {
         // TODO(nweiz): Make this a MultiSpanSassException.
-        throw SassException(
-            "From ${selector.span.message('')}\n"
-            "${error.message}",
-            error.span);
+        throwWithTrace(
+            SassException(
+                "From ${selector.span.message('')}\n"
+                "${error.message}",
+                error.span),
+            stackTrace);
       }
 
       // If no extends actually happened (for example because unification

diff --git a/lib/src/io/vm.dart b/lib/src/io/vm.dart
@@ -12,6 +12,7 @@ import 'package:source_span/source_span.dart';
 import 'package:watcher/watcher.dart';
 
 import '../exception.dart';
+import '../utils.dart';
 
 export 'dart:io' show exitCode, FileSystemException;
 
@@ -41,16 +42,18 @@ String readFile(String path) {
 
   try {
     return utf8.decode(bytes);
-  } on FormatException catch (error) {
+  } on FormatException catch (error, stackTrace) {
     var decodedUntilError =
         utf8.decode(bytes.sublist(0, error.offset), allowMalformed: true);
     var stringOffset = decodedUntilError.length;
     if (decodedUntilError.endsWith("�")) stringOffset--;
 
     var decoded = utf8.decode(bytes, allowMalformed: true);
     var sourceFile = SourceFile.fromString(decoded, url: p.toUri(path));
-    throw SassException(
-        "Invalid UTF-8.", sourceFile.location(stringOffset).pointSpan());
+    throwWithTrace(
+        SassException(
+            "Invalid UTF-8.", sourceFile.location(stringOffset).pointSpan()),
+        stackTrace);
   }
 }
 

diff --git a/lib/src/node/compile.dart b/lib/src/node/compile.dart
@@ -4,7 +4,7 @@
 
 import 'package:js/js.dart';
 import 'package:node_interop/js.dart';
-import 'package:node_interop/util.dart';
+import 'package:node_interop/util.dart' hide futureToPromise;
 import 'package:term_glyph/term_glyph.dart' as glyph;
 
 import '../../sass.dart';
@@ -42,8 +42,8 @@ NodeCompileResult compile(String path, [CompileOptions? options]) {
             ascii: ascii),
         importers: options?.importers?.map(_parseImporter));
     return _convertResult(result);
-  } on SassException catch (error) {
-    throwNodeException(error, color: color, ascii: ascii);
+  } on SassException catch (error, stackTrace) {
+    throwNodeException(error, color: color, ascii: ascii, trace: stackTrace);
   }
 }
 
@@ -70,8 +70,8 @@ NodeCompileResult compileString(String text, [CompileStringOptions? options]) {
         importer: options?.importer.andThen(_parseImporter) ??
             (options?.url == null ? NoOpImporter() : null));
     return _convertResult(result);
-  } on SassException catch (error) {
-    throwNodeException(error, color: color, ascii: ascii);
+  } on SassException catch (error, stackTrace) {
+    throwNodeException(error, color: color, ascii: ascii, trace: stackTrace);
   }
 }
 

diff --git a/lib/src/node/exception.dart b/lib/src/node/exception.dart
@@ -5,14 +5,20 @@
 import 'dart:js_util';
 
 import 'package:js/js.dart';
+import 'package:node_interop/node_interop.dart';
 import 'package:term_glyph/term_glyph.dart' as glyph;
 
 import '../exception.dart';
+import '../utils.dart';
 import 'utils.dart';
 
 @JS()
 @anonymous
-class _NodeException {
+class _NodeException extends JsError {
+  // Fake constructor to silence the no_generative_constructor_in_superclass
+  // error.
+  external factory _NodeException();
+
   external SassException get _dartException;
 }
 
@@ -58,15 +64,20 @@ var exceptionConstructor = () {
 ///
 /// If [ascii] is `false`, the thrown exception uses non-ASCII characters in its
 /// stringification.
+///
+/// If [trace] is passed, it's used as the stack trace for the JS exception.
 Never throwNodeException(SassException exception,
-    {required bool color, required bool ascii}) {
+    {required bool color, required bool ascii, StackTrace? trace}) {
   var wasAscii = glyph.ascii;
   glyph.ascii = ascii;
   try {
-    jsThrow(callConstructor(exceptionConstructor, [
+    var jsException = callConstructor(exceptionConstructor, [
       exception,
       exception.toString(color: color).replaceFirst('Error: ', '')
-    ]) as _NodeException);
+    ]) as _NodeException;
+    trace = getTrace(exception) ?? trace;
+    if (trace != null) attachJsStack(jsException, trace);
+    jsThrow(jsException);
   } finally {
     glyph.ascii = wasAscii;
   }

diff --git a/lib/src/node/legacy.dart b/lib/src/node/legacy.dart
@@ -24,6 +24,7 @@ import '../logger/node_to_dart.dart';
 import '../parse/scss.dart';
 import '../syntax.dart';
 import '../util/nullable.dart';
+import '../utils.dart';
 import '../value.dart';
 import '../visitor/serialize.dart';
 import 'function.dart';
@@ -56,9 +57,12 @@ void render(
       callback(null, result);
     }, onError: (Object error, StackTrace stackTrace) {
       if (error is SassException) {
-        callback(_wrapException(error), null);
+        callback(_wrapException(error, stackTrace), null);
       } else {
-        callback(_newRenderError(error.toString(), status: 3), null);
+        callback(
+            _newRenderError(error.toString(), getTrace(error) ?? stackTrace,
+                status: 3),
+            null);
       }
     });
   }
@@ -158,15 +162,16 @@ RenderResult renderSync(RenderOptions options) {
     }
 
     return _newRenderResult(options, result, start);
-  } on SassException catch (error) {
-    jsThrow(_wrapException(error));
-  } catch (error) {
-    jsThrow(_newRenderError(error.toString(), status: 3));
+  } on SassException catch (error, stackTrace) {
+    jsThrow(_wrapException(error, stackTrace));
+  } catch (error, stackTrace) {
+    jsThrow(_newRenderError(error.toString(), getTrace(error) ?? stackTrace,
+        status: 3));
   }
 }
 
 /// Converts an exception to a [JsError].
-JsError _wrapException(Object exception) {
+JsError _wrapException(Object exception, StackTrace stackTrace) {
   if (exception is SassException) {
     String file;
     var url = exception.span.sourceUrl;
@@ -179,12 +184,15 @@ JsError _wrapException(Object exception) {
     }
 
     return _newRenderError(exception.toString().replaceFirst("Error: ", ""),
+        getTrace(exception) ?? stackTrace,
         line: exception.span.start.line + 1,
         column: exception.span.start.column + 1,
         file: file,
         status: 1);
   } else {
-    return JsError(exception.toString());
+    var error = JsError(exception.toString());
+    attachJsStack(error, getTrace(exception) ?? stackTrace);
+    return error;
   }
 }
 
@@ -203,9 +211,11 @@ List<AsyncCallable> _parseFunctions(RenderOptions options, DateTime start,
     Tuple2<String, ArgumentDeclaration> tuple;
     try {
       tuple = ScssParser(signature as String).parseSignature();
-    } on SassFormatException catch (error) {
-      throw SassFormatException(
-          'Invalid signature "$signature": ${error.message}', error.span);
+    } on SassFormatException catch (error, stackTrace) {
+      throwWithTrace(
+          SassFormatException(
+              'Invalid signature "$signature": ${error.message}', error.span),
+          stackTrace);
     }
 
     var context = RenderContext(options: _contextOptions(options, start));
@@ -417,13 +427,14 @@ bool _enableSourceMaps(RenderOptions options) =>
 
 /// Creates a [JsError] with the given fields added to it so it acts like a Node
 /// Sass error.
-JsError _newRenderError(String message,
+JsError _newRenderError(String message, StackTrace stackTrace,
     {int? line, int? column, String? file, int? status}) {
   var error = JsError(message);
   setProperty(error, 'formatted', 'Error: $message');
   if (line != null) setProperty(error, 'line', line);
   if (column != null) setProperty(error, 'column', column);
   if (file != null) setProperty(error, 'file', file);
   if (status != null) setProperty(error, 'status', status);
+  attachJsStack(error, stackTrace);
   return error;
 }
diff --git a/lib/src/node/utils.dart b/lib/src/node/utils.dart
@@ -11,6 +11,7 @@ import 'package:js/js.dart';
 import 'package:js/js_util.dart';
 
 import '../syntax.dart';
+import '../utils.dart';
 import 'array.dart';
 import 'function.dart';
 import 'url.dart';
@@ -44,6 +45,20 @@ external Function get jsErrorConstructor;
 /// Returns whether [value] is a JS Error object.
 bool isJSError(Object value) => instanceof(value, jsErrorConstructor);
 
+/// Attaches [trace] to [error] as its stack trace.
+void attachJsStack(JsError error, StackTrace trace) {
+  // Stack traces in v8 contain the error message itself as well as the stack
+  // information, so we trim that out if it exists so we don't double-print it.
+  var traceString = trace.toString();
+  var firstRealLine = traceString.indexOf('\n    at');
+  if (firstRealLine != -1) {
+    // +1 to account for the newline before the first line.
+    traceString = traceString.substring(firstRealLine + 1);
+  }
+
+  setProperty(error, 'stack', "Error: ${error.message}\n$traceString");
+}
+
 /// Invokes [function] with [thisArg] as `this`.
 Object? call2(JSFunction function, Object thisArg, Object arg1, Object arg2) =>
     function.apply(thisArg, [arg1, arg2]);
@@ -169,6 +184,17 @@ external Function get _promiseClass;
 bool isPromise(Object? object) =>
     object != null && instanceof(object, _promiseClass);
 
+/// Like [futureToPromise] from `node_interop`, but stores the stack trace for
+/// errors using [throwWithTrace].
+Promise futureToPromise(Future<Object?> future) => Promise(allowInterop(
+        (void Function(Object?) resolve, void Function(Object?) reject) {
+      future.then((result) => resolve(result),
+          onError: (Object error, StackTrace stackTrace) {
+        attachTrace(error, stackTrace);
+        reject(error);
+      });
+    }));
+
 @JS('URL')
 external Function get _urlClass;