errors: print original exception context

bcoe · Benjamin Coe · commit 458677f5ef2b · 2020-05-24T22:03:34.000-07:00
When --enable-source-maps is set, the error context displayed above the stack trace now shows original source rather than transpiled. PR-URL: #33491 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
diff --git a/lib/internal/bootstrap/pre_execution.js b/lib/internal/bootstrap/pre_execution.js
@@ -26,14 +26,6 @@ function prepareMainThreadExecution(expandArgv1 = false) {
       setupCoverageHooks(process.env.NODE_V8_COVERAGE);
   }
 
-  // If source-map support has been enabled, we substitute in a new
-  // prepareStackTrace method, replacing the default in errors.js.
-  if (getOptionValue('--enable-source-maps')) {
-    const { prepareStackTrace } =
-      require('internal/source_map/prepare_stack_trace');
-    const { setPrepareStackTraceCallback } = internalBinding('errors');
-    setPrepareStackTraceCallback(prepareStackTrace);
-  }
 
   setupDebugEnv();
 
diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
@@ -50,6 +50,7 @@ const {
 
 const { NativeModule } = require('internal/bootstrap/loaders');
 const {
+  getSourceMapsEnabled,
   maybeCacheSourceMap,
   rekeySourceMap
 } = require('internal/source_map/source_map_cache');
@@ -71,7 +72,6 @@ const {
   loadNativeModule
 } = require('internal/modules/cjs/helpers');
 const { getOptionValue } = require('internal/options');
-const enableSourceMaps = getOptionValue('--enable-source-maps');
 const preserveSymlinks = getOptionValue('--preserve-symlinks');
 const preserveSymlinksMain = getOptionValue('--preserve-symlinks-main');
 const manifest = getOptionValue('--experimental-policy') ?
@@ -941,7 +941,7 @@ Module._load = function(request, parent, isMain) {
     // Intercept exceptions that occur during the first tick and rekey them
     // on error instance rather than module instance (which will immediately be
     // garbage collected).
-    if (enableSourceMaps) {
+    if (getSourceMapsEnabled()) {
       try {
         module.load(filename);
       } catch (err) {
diff --git a/lib/internal/source_map/prepare_stack_trace.js b/lib/internal/source_map/prepare_stack_trace.js
@@ -5,6 +5,8 @@ const {
 } = primordials;
 
 const debug = require('internal/util/debuglog').debuglog('source_map');
+const { getStringWidth } = require('internal/util/inspect');
+const { readFileSync } = require('fs');
 const { findSourceMap } = require('internal/source_map/source_map_cache');
 const {
   kNoOverride,
@@ -34,7 +36,17 @@ const prepareStackTrace = (globalThis, error, trace) => {
   if (trace.length === 0) {
     return errorString;
   }
+
+  let errorSource = '';
+  let firstSource;
+  let firstLine;
+  let firstColumn;
   const preparedTrace = trace.map((t, i) => {
+    if (i === 0) {
+      firstLine = t.getLineNumber();
+      firstColumn = t.getColumnNumber();
+      firstSource = t.getFileName();
+    }
     let str = i !== 0 ? '\n    at ' : '';
     str = `${str}${t}`;
     try {
@@ -49,18 +61,57 @@ const prepareStackTrace = (globalThis, error, trace) => {
         } = sm.findEntry(t.getLineNumber() - 1, t.getColumnNumber() - 1);
         if (originalSource && originalLine !== undefined &&
             originalColumn !== undefined) {
-          str +=
-`\n        -> ${originalSource.replace('file://', '')}:${originalLine + 1}:${originalColumn + 1}`;
+          const originalSourceNoScheme = originalSource
+            .replace(/^file:\/\//, '');
+          if (i === 0) {
+            firstLine = originalLine + 1;
+            firstColumn = originalColumn + 1;
+            firstSource = originalSourceNoScheme;
+            // Show error in original source context to help user pinpoint it:
+            errorSource = getErrorSource(firstSource, firstLine, firstColumn);
+          }
+          // Show both original and transpiled stack trace information:
+          str += `\n        -> ${originalSourceNoScheme}:${originalLine + 1}:` +
+            `${originalColumn + 1}`;
         }
       }
     } catch (err) {
       debug(err.stack);
     }
     return str;
   });
-  return `${errorString}\n    at ${preparedTrace.join('')}`;
+  return `${errorSource}${errorString}\n    at ${preparedTrace.join('')}`;
 };
 
+// Places a snippet of code from where the exception was originally thrown
+// above the stack trace. This logic is modeled after GetErrorSource in
+// node_errors.cc.
+function getErrorSource(firstSource, firstLine, firstColumn) {
+  let exceptionLine = '';
+  let source;
+  try {
+    source = readFileSync(firstSource, 'utf8');
+  } catch (err) {
+    debug(err);
+    return exceptionLine;
+  }
+  const lines = source.split(/\r?\n/, firstLine);
+  const line = lines[firstLine - 1];
+  if (!line) return exceptionLine;
+
+  // Display ^ in appropriate position, regardless of whether tabs or
+  // spaces are used:
+  let prefix = '';
+  for (const character of line.slice(0, firstColumn)) {
+    prefix += (character === '\t') ? '\t' :
+      ' '.repeat(getStringWidth(character));
+  }
+  prefix = prefix.slice(0, -1); // The last character is the '^'.
+
+  exceptionLine = `${firstSource}:${firstLine}\n${line}\n${prefix}^\n\n`;
+  return exceptionLine;
+}
+
 module.exports = {
   prepareStackTrace,
 };
diff --git a/lib/internal/source_map/source_map_cache.js b/lib/internal/source_map/source_map_cache.js
@@ -39,12 +39,28 @@ const { fileURLToPath, URL } = require('url');
 let Module;
 let SourceMap;
 
-let experimentalSourceMaps;
-function maybeCacheSourceMap(filename, content, cjsModuleInstance) {
-  if (experimentalSourceMaps === undefined) {
-    experimentalSourceMaps = getOptionValue('--enable-source-maps');
+let sourceMapsEnabled;
+function getSourceMapsEnabled() {
+  if (sourceMapsEnabled === undefined) {
+    sourceMapsEnabled = getOptionValue('--enable-source-maps');
+    if (sourceMapsEnabled) {
+      const {
+        enableSourceMaps,
+        setPrepareStackTraceCallback
+      } = internalBinding('errors');
+      const {
+        prepareStackTrace
+      } = require('internal/source_map/prepare_stack_trace');
+      setPrepareStackTraceCallback(prepareStackTrace);
+      enableSourceMaps();
+    }
   }
-  if (!(process.env.NODE_V8_COVERAGE || experimentalSourceMaps)) return;
+  return sourceMapsEnabled;
+}
+
+function maybeCacheSourceMap(filename, content, cjsModuleInstance) {
+  const sourceMapsEnabled = getSourceMapsEnabled();
+  if (!(process.env.NODE_V8_COVERAGE || sourceMapsEnabled)) return;
   let basePath;
   try {
     filename = normalizeReferrerURL(filename);
@@ -248,6 +264,7 @@ function findSourceMap(uri, error) {
 
 module.exports = {
   findSourceMap,
+  getSourceMapsEnabled,
   maybeCacheSourceMap,
   rekeySourceMap,
   sourceMapCacheToObject,
diff --git a/src/env-inl.h b/src/env-inl.h
@@ -795,6 +795,14 @@ void Environment::set_emit_insecure_umask_warning(bool on) {
   emit_insecure_umask_warning_ = on;
 }
 
+void Environment::set_source_maps_enabled(bool on) {
+  source_maps_enabled_ = on;
+}
+
+bool Environment::source_maps_enabled() const {
+  return source_maps_enabled_;
+}
+
 inline uint64_t Environment::thread_id() const {
   return thread_id_;
 }
diff --git a/src/env.h b/src/env.h
@@ -1059,6 +1059,9 @@ class Environment : public MemoryRetainer {
   inline bool emit_insecure_umask_warning() const;
   inline void set_emit_insecure_umask_warning(bool on);
 
+  inline void set_source_maps_enabled(bool on);
+  inline bool source_maps_enabled() const;
+
   inline void ThrowError(const char* errmsg);
   inline void ThrowTypeError(const char* errmsg);
   inline void ThrowRangeError(const char* errmsg);
@@ -1277,6 +1280,8 @@ class Environment : public MemoryRetainer {
   bool emit_err_name_warning_ = true;
   bool emit_filehandle_warning_ = true;
   bool emit_insecure_umask_warning_ = true;
+  bool source_maps_enabled_ = false;
+
   size_t async_callback_scope_depth_ = 0;
   std::vector<double> destroy_async_id_list_;
 
diff --git a/src/node_errors.cc b/src/node_errors.cc
@@ -57,6 +57,16 @@ static std::string GetErrorSource(Isolate* isolate,
   node::Utf8Value encoded_source(isolate, source_line_maybe.ToLocalChecked());
   std::string sourceline(*encoded_source, encoded_source.length());
 
+  // If source maps have been enabled, the exception line will instead be
+  // added in the JavaScript context:
+  Environment* env = Environment::GetCurrent(isolate);
+  const bool has_source_map_url =
+      !message->GetScriptOrigin().SourceMapUrl().IsEmpty();
+  if (has_source_map_url && env->source_maps_enabled()) {
+    *added_exception_line = false;
+    return sourceline;
+  }
+
   if (sourceline.find("node-do-not-add-exception-line") != std::string::npos) {
     *added_exception_line = false;
     return sourceline;
@@ -802,6 +812,11 @@ void SetPrepareStackTraceCallback(const FunctionCallbackInfo<Value>& args) {
   env->set_prepare_stack_trace_callback(args[0].As<Function>());
 }
 
+static void EnableSourceMaps(const FunctionCallbackInfo<Value>& args) {
+  Environment* env = Environment::GetCurrent(args);
+  env->set_source_maps_enabled(true);
+}
+
 static void SetEnhanceStackForFatalException(
     const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
@@ -840,6 +855,7 @@ void Initialize(Local<Object> target,
   Environment* env = Environment::GetCurrent(context);
   env->SetMethod(
       target, "setPrepareStackTraceCallback", SetPrepareStackTraceCallback);
+  env->SetMethod(target, "enableSourceMaps", EnableSourceMaps);
   env->SetMethod(target,
                  "setEnhanceStackForFatalException",
                  SetEnhanceStackForFatalException);
diff --git a/test/fixtures/source-map/icu.js b/test/fixtures/source-map/icu.js
diff --git a/test/fixtures/source-map/icu.jsx b/test/fixtures/source-map/icu.jsx
@@ -0,0 +1,12 @@
+const React = {
+  createElement: () => {
+    ("あ 🐕 🐕", throw Error("an error"));
+  }
+};
+
+const profile = (
+  <div>
+    <img src="avatar.png" className="profile" />
+    <h3>{["hello"]}</h3>
+  </div>
+);
diff --git a/test/fixtures/source-map/tabs.coffee b/test/fixtures/source-map/tabs.coffee
@@ -0,0 +1,29 @@
+# Assignment:
+number   = 42
+opposite = true
+
+# Conditions:
+number = -42 if opposite
+
+# Functions:
+square = (x) -> x * x
+
+# Arrays:
+list = [1, 2, 3, 4, 5]
+
+# Objects:
+math =
+	root:   Math.sqrt
+	square: square
+	cube:   (x) -> x * square x
+
+# Splats:
+race = (winner, runners...) ->
+	print winner, runners
+
+# Existence:
+if true
+	alert "I knew it!"
+
+# Array comprehensions:
+cubes = (math.cube num for num in list)
diff --git a/test/fixtures/source-map/tabs.js b/test/fixtures/source-map/tabs.js
diff --git a/test/message/source_map_reference_error_tabs.js b/test/message/source_map_reference_error_tabs.js
@@ -0,0 +1,5 @@
+// Flags:  --enable-source-maps
+
+'use strict';
+require('../common');
+require('../fixtures/source-map/tabs.js');
diff --git a/test/message/source_map_reference_error_tabs.out b/test/message/source_map_reference_error_tabs.out
@@ -0,0 +1,17 @@
+*tabs.coffee:26
+	alert "I knew it!"
+	^
+
+ReferenceError: alert is not defined
+    at Object.<anonymous> (*tabs.coffee:39:5)
+        -> *tabs.coffee:26:2
+    at Object.<anonymous> (*tabs.coffee:53:4)
+        -> *tabs.coffee:1:14
+    at Module._compile (internal/modules/cjs/loader.js:*
+    at Object.Module._extensions..js (internal/modules/cjs/loader.js:*
+    at Module.load (internal/modules/cjs/loader.js:*
+    at Function.Module._load (internal/modules/cjs/loader.js:*
+    at Module.require (internal/modules/cjs/loader.js:*
+    at require (internal/modules/cjs/helpers.js:*
+    at Object.<anonymous> (*source_map_reference_error_tabs.js:*
+    at Module._compile (internal/modules/cjs/loader.js:*
diff --git a/test/message/source_map_throw_catch.out b/test/message/source_map_throw_catch.out
@@ -1,4 +1,7 @@
 reachable
+*typescript-throw.ts:18
+    throw Error('an exception');
+          ^
 Error: an exception
     at branch (*typescript-throw.js:20:15)
         -> *typescript-throw.ts:18:11
diff --git a/test/message/source_map_throw_first_tick.out b/test/message/source_map_throw_first_tick.out
@@ -1,4 +1,7 @@
 reachable
+*typescript-throw.ts:18
+    throw Error('an exception');
+          ^
 Error: an exception
     at branch (*typescript-throw.js:20:15)
         -> *typescript-throw.ts:18:11
diff --git a/test/message/source_map_throw_icu.js b/test/message/source_map_throw_icu.js
diff --git a/test/message/source_map_throw_icu.out b/test/message/source_map_throw_icu.out
diff --git a/test/message/source_map_throw_set_immediate.out b/test/message/source_map_throw_set_immediate.out

Original file line number	Diff line number	Diff line change
`@@ -795,6 +795,14 @@ void Environment::set_emit_insecure_umask_warning(bool on) {`
`795`	`795`	`emit_insecure_umask_warning_ = on;`
`796`	`796`	`}`
`797`	`797`
	`798`	`+void Environment::set_source_maps_enabled(bool on) {`
	`799`	`+ source_maps_enabled_ = on;`
	`800`	`+}`
	`801`	`+`
	`802`	`+bool Environment::source_maps_enabled() const {`
	`803`	`+ return source_maps_enabled_;`
	`804`	`+}`
	`805`	`+`
`798`	`806`	`inline uint64_t Environment::thread_id() const {`
`799`	`807`	`return thread_id_;`
`800`	`808`	`}`