errors: display original symbol name

If symbol names array has been populated in source map, include original symbol name in error message. Fixes #35325 PR-URL: #36042 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
nodejs · Dec 7, 2020 · c14512b · c14512b
1 parent 5f41f1b
commit c14512b
Show file tree

Hide file tree

Showing 14 changed files with 178 additions and 66 deletions.
diff --git a/doc/api/module.md b/doc/api/module.md
@@ -203,6 +203,7 @@ consists of the following keys:
 * originalSource: {string}
 * originalLine: {number}
 * originalColumn: {number}
+* name: {string}
 
 [CommonJS]: modules.md
 [ES Modules]: esm.md

diff --git a/lib/internal/source_map/prepare_stack_trace.js b/lib/internal/source_map/prepare_stack_trace.js
@@ -47,45 +47,48 @@ const prepareStackTrace = (globalThis, error, trace) => {
   }
 
   let errorSource = '';
-  let firstLine;
-  let firstColumn;
+  let lastSourceMap;
+  let lastFileName;
   const preparedTrace = ArrayPrototypeJoin(ArrayPrototypeMap(trace, (t, i) => {
-    if (i === 0) {
-      firstLine = t.getLineNumber();
-      firstColumn = t.getColumnNumber();
-    }
     let str = i !== 0 ? '\n    at ' : '';
     str = `${str}${t}`;
     try {
-      const sm = findSourceMap(t.getFileName());
+      // A stack trace will often have several call sites in a row within the
+      // same file, cache the source map and file content accordingly:
+      const fileName = t.getFileName();
+      const sm = fileName === lastFileName ?
+        lastSourceMap :
+        findSourceMap(fileName);
+      lastSourceMap = sm;
+      lastFileName = fileName;
       if (sm) {
-        // Source Map V3 lines/columns use zero-based offsets whereas, in
-        // stack traces, they start at 1/1.
+        // Source Map V3 lines/columns start at 0/0 whereas stack traces
+        // start at 1/1:
         const {
           originalLine,
           originalColumn,
-          originalSource
+          originalSource,
         } = sm.findEntry(t.getLineNumber() - 1, t.getColumnNumber() - 1);
         if (originalSource && originalLine !== undefined &&
             originalColumn !== undefined) {
+          const name = getOriginalSymbolName(sm, trace, i);
           if (i === 0) {
-            firstLine = originalLine + 1;
-            firstColumn = originalColumn + 1;
-
-            // Show error in original source context to help user pinpoint it:
             errorSource = getErrorSource(
-              sm.payload,
+              sm,
               originalSource,
-              firstLine,
-              firstColumn
+              originalLine,
+              originalColumn
             );
           }
           // Show both original and transpiled stack trace information:
+          const prefix = (name && name !== t.getFunctionName()) ?
+            `\n        -> at ${name}` :
+            '\n        ->';
           const originalSourceNoScheme =
             StringPrototypeStartsWith(originalSource, 'file://') ?
               fileURLToPath(originalSource) : originalSource;
-          str += `\n        -> ${originalSourceNoScheme}:${originalLine + 1}:` +
-            `${originalColumn + 1}`;
+          str += `${prefix} (${originalSourceNoScheme}:${originalLine + 1}:` +
+            `${originalColumn + 1})`;
         }
       }
     } catch (err) {
@@ -96,18 +99,69 @@ const prepareStackTrace = (globalThis, error, trace) => {
   return `${errorSource}${errorString}\n    at ${preparedTrace}`;
 };
 
+// Transpilers may have removed the original symbol name used in the stack
+// trace, if possible restore it from the names field of the source map:
+function getOriginalSymbolName(sourceMap, trace, curIndex) {
+  // First check for a symbol name associated with the enclosing function:
+  const enclosingEntry = sourceMap.findEntry(
+    trace[curIndex].getEnclosingLineNumber() - 1,
+    trace[curIndex].getEnclosingColumnNumber() - 1
+  );
+  if (enclosingEntry.name) return enclosingEntry.name;
+  // Fallback to using the symbol name attached to the next stack frame:
+  const currentFileName = trace[curIndex].getFileName();
+  const nextCallSite = trace[curIndex + 1];
+  if (nextCallSite && currentFileName === nextCallSite.getFileName()) {
+    const { name } = sourceMap.findEntry(
+      nextCallSite.getLineNumber() - 1,
+      nextCallSite.getColumnNumber() - 1
+    );
+    return name;
+  }
+}
+
 // 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(payload, originalSource, firstLine, firstColumn) {
+function getErrorSource(
+  sourceMap,
+  originalSourcePath,
+  originalLine,
+  originalColumn
+) {
   let exceptionLine = '';
-  const originalSourceNoScheme =
-    StringPrototypeStartsWith(originalSource, 'file://') ?
-      fileURLToPath(originalSource) : originalSource;
+  const originalSourcePathNoScheme =
+    StringPrototypeStartsWith(originalSourcePath, 'file://') ?
+      fileURLToPath(originalSourcePath) : originalSourcePath;
+  const source = getOriginalSource(
+    sourceMap.payload,
+    originalSourcePathNoScheme
+  );
+  const lines = StringPrototypeSplit(source, /\r?\n/, originalLine + 1);
+  const line = lines[originalLine];
+  if (!line) return exceptionLine;
+
+  // Display ^ in appropriate position, regardless of whether tabs or
+  // spaces are used:
+  let prefix = '';
+  for (const character of StringPrototypeSlice(line, 0, originalColumn + 1)) {
+    prefix += (character === '\t') ? '\t' :
+      StringPrototypeRepeat(' ', getStringWidth(character));
+  }
+  prefix = StringPrototypeSlice(prefix, 0, -1); // The last character is '^'.
 
+  exceptionLine =
+   `${originalSourcePathNoScheme}:${originalLine + 1}\n${line}\n${prefix}^\n\n`;
+  return exceptionLine;
+}
+
+function getOriginalSource(payload, originalSourcePath) {
   let source;
+  const originalSourcePathNoScheme =
+    StringPrototypeStartsWith(originalSourcePath, 'file://') ?
+      fileURLToPath(originalSourcePath) : originalSourcePath;
   const sourceContentIndex =
-    ArrayPrototypeIndexOf(payload.sources, originalSource);
+    ArrayPrototypeIndexOf(payload.sources, originalSourcePath);
   if (payload.sourcesContent?.[sourceContentIndex]) {
     // First we check if the original source content was provided in the
     // source map itself:
@@ -116,29 +170,13 @@ function getErrorSource(payload, originalSource, firstLine, firstColumn) {
     // If no sourcesContent was found, attempt to load the original source
     // from disk:
     try {
-      source = readFileSync(originalSourceNoScheme, 'utf8');
+      source = readFileSync(originalSourcePathNoScheme, 'utf8');
     } catch (err) {
       debug(err);
-      return '';
+      source = '';
     }
   }
-
-  const lines = StringPrototypeSplit(source, /\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 StringPrototypeSlice(line, 0, firstColumn)) {
-    prefix += (character === '\t') ? '\t' :
-      StringPrototypeRepeat(' ', getStringWidth(character));
-  }
-  prefix = StringPrototypeSlice(prefix, 0, -1); // The last character is '^'.
-
-  exceptionLine =
-              `${originalSourceNoScheme}:${firstLine}\n${line}\n${prefix}^\n\n`;
-  return exceptionLine;
+  return source;
 }
 
 module.exports = {

diff --git a/lib/internal/source_map/source_map.js b/lib/internal/source_map/source_map.js
@@ -203,7 +203,8 @@ class SourceMap {
       generatedColumn: entry[1],
       originalSource: entry[2],
       originalLine: entry[3],
-      originalColumn: entry[4]
+      originalColumn: entry[4],
+      name: entry[5],
     };
   }
 
@@ -214,6 +215,7 @@ class SourceMap {
     let sourceIndex = 0;
     let sourceLineNumber = 0;
     let sourceColumnNumber = 0;
+    let nameIndex = 0;
 
     const sources = [];
     const originalToCanonicalURLMap = {};
@@ -229,7 +231,6 @@ class SourceMap {
 
     const stringCharIterator = new StringCharIterator(map.mappings);
     let sourceURL = sources[sourceIndex];
-
     while (true) {
       if (stringCharIterator.peek() === ',')
         stringCharIterator.next();
@@ -256,12 +257,15 @@ class SourceMap {
       }
       sourceLineNumber += decodeVLQ(stringCharIterator);
       sourceColumnNumber += decodeVLQ(stringCharIterator);
-      if (!isSeparator(stringCharIterator.peek()))
-        // Unused index into the names list.
-        decodeVLQ(stringCharIterator);
+
+      let name;
+      if (!isSeparator(stringCharIterator.peek())) {
+        nameIndex += decodeVLQ(stringCharIterator);
+        name = map.names?.[nameIndex];
+      }
 
       this.#mappings.push([lineNumber, columnNumber, sourceURL,
-                           sourceLineNumber, sourceColumnNumber]);
+                           sourceLineNumber, sourceColumnNumber, name]);
     }
   };
 }

diff --git a/test/fixtures/source-map/enclosing-call-site-min.js b/test/fixtures/source-map/enclosing-call-site-min.js
@@ -0,0 +1,3 @@
+var functionA=function(){functionB()};function functionB(){functionC()}var functionC=function(){functionD()},functionD=function(){if(0<Math.random())throw Error("an error!");},thrower=functionA;try{functionA()}catch(a){throw a;};
+//# sourceMappingURL=enclosing-call-site.js.map
+
diff --git a/test/fixtures/source-map/enclosing-call-site.js b/test/fixtures/source-map/enclosing-call-site.js
@@ -0,0 +1,27 @@
+const functionA = () => {
+  functionB()
+}
+
+function functionB() {
+  functionC()
+}
+
+const functionC = () => {
+  functionD()
+}
+
+const functionD = () => {
+  (function functionE () {
+    if (Math.random() > 0) {
+      throw new Error('an error!')
+    }
+  })()
+}
+
+const thrower = functionA
+
+try {
+  thrower()
+} catch (err) {
+  throw err
+}
diff --git a/test/fixtures/source-map/enclosing-call-site.js.map b/test/fixtures/source-map/enclosing-call-site.js.map
diff --git a/test/message/source_map_enclosing_function.js b/test/message/source_map_enclosing_function.js
@@ -0,0 +1,5 @@
+// Flags:  --enable-source-maps
+
+'use strict';
+require('../common');
+require('../fixtures/source-map/enclosing-call-site-min.js');
diff --git a/test/message/source_map_enclosing_function.out b/test/message/source_map_enclosing_function.out
@@ -0,0 +1,20 @@
+*enclosing-call-site.js:16
+      throw new Error('an error!')
+                ^
+
+Error: an error!
+    at functionD (*enclosing-call-site-min.js:1:156)
+        -> (*enclosing-call-site.js:16:17)
+    at functionC (*enclosing-call-site-min.js:1:97)
+        -> (*enclosing-call-site.js:10:3)
+    at functionB (*enclosing-call-site-min.js:1:60)
+        -> (*enclosing-call-site.js:6:3)
+    at functionA (*enclosing-call-site-min.js:1:26)
+        -> (*enclosing-call-site.js:2:3)
+    at Object.<anonymous> (*enclosing-call-site-min.js:1:199)
+        -> (*enclosing-call-site.js:24:3)
+    at Module._compile (node:internal/modules/cjs/loader:*)
+    at Object.Module._extensions..js (node:internal/modules/cjs/loader:*)
+    at Module.load (node:internal/modules/cjs/loader:*)
+    at Function.Module._load (node:internal/modules/cjs/loader:*)
+    at Module.require (node:internal/modules/cjs/loader:*)
diff --git a/test/message/source_map_reference_error_tabs.out b/test/message/source_map_reference_error_tabs.out
@@ -4,9 +4,9 @@
 
 ReferenceError: alert is not defined
     at Object.<anonymous> (*tabs.coffee:39:5)
-        -> *tabs.coffee:26:2
+        -> *tabs.coffee:26:2*
     at Object.<anonymous> (*tabs.coffee:53:4)
-        -> *tabs.coffee:1:14
+        -> *tabs.coffee:1:14*
     at Module._compile (node:internal/modules/cjs/loader:*
     at Object.Module._extensions..js (node:internal/modules/cjs/loader:*
     at Module.load (node:internal/modules/cjs/loader:*

diff --git a/test/message/source_map_throw_catch.out b/test/message/source_map_throw_catch.out
@@ -4,9 +4,9 @@ reachable
           ^
 Error: an exception
     at branch (*typescript-throw.js:20:15)
-        -> *typescript-throw.ts:18:11
+        -> *typescript-throw.ts:18:11*
     at Object.<anonymous> (*typescript-throw.js:26:1)
-        -> *typescript-throw.ts:24:1
+        -> *typescript-throw.ts:24:1*
     at Module._compile (node:internal/modules/cjs/loader:*)
     at Object.Module._extensions..js (node:internal/modules/cjs/loader:*)
     at Module.load (node:internal/modules/cjs/loader:*)

diff --git a/test/message/source_map_throw_first_tick.out b/test/message/source_map_throw_first_tick.out
@@ -4,9 +4,9 @@ reachable
           ^
 Error: an exception
     at branch (*typescript-throw.js:20:15)
-        -> *typescript-throw.ts:18:11
+        -> *typescript-throw.ts:18:11*
     at Object.<anonymous> (*typescript-throw.js:26:1)
-        -> *typescript-throw.ts:24:1
+        -> *typescript-throw.ts:24:1*
     at Module._compile (node:internal/modules/cjs/loader:*)
     at Object.Module._extensions..js (node:internal/modules/cjs/loader:*)
     at Module.load (node:internal/modules/cjs/loader:*)

diff --git a/test/message/source_map_throw_icu.out b/test/message/source_map_throw_icu.out
@@ -4,9 +4,9 @@
 
 Error: an error
     at Object.createElement (*icu.js:5:7)
-        -> *icu.jsx:3:23
+        -> *icu.jsx:3:23*
     at Object.<anonymous> (*icu.js:8:82)
-        -> *icu.jsx:9:5
+        -> *icu.jsx:9:5*
     at Module._compile (node:internal/modules/cjs/loader:*
     at Object.Module._extensions..js (node:internal/modules/cjs/loader:*
     at Module.load (node:internal/modules/cjs/loader:*

diff --git a/test/message/source_map_throw_set_immediate.out b/test/message/source_map_throw_set_immediate.out
@@ -4,7 +4,7 @@
 
 Error: goodbye
     at *uglify-throw.js:1:43
-        -> *uglify-throw-original.js:5:9
+        -> at Hello *uglify-throw-original.js:5:9*
     at Immediate.<anonymous> (*uglify-throw.js:1:60)
-        -> *uglify-throw-original.js:9:3
+        -> *uglify-throw-original.js:9:3*
     at processImmediate (node:internal/timers:*)
diff --git a/test/parallel/test-source-map-enable.js b/test/parallel/test-source-map-enable.js
@@ -170,12 +170,15 @@ function nextdir() {
     '--enable-source-maps',
     require.resolve('../fixtures/source-map/uglify-throw.js')
   ]);
-  assert.ok(
-    output.stderr.toString().match(/->.*uglify-throw-original\.js:5:9/)
+  assert.match(
+    output.stderr.toString(),
+    /->.*uglify-throw-original\.js:5:9/
   );
-  assert.ok(
-    output.stderr.toString().match(/->.*uglify-throw-original\.js:9:3/)
+  assert.match(
+    output.stderr.toString(),
+    /->.*uglify-throw-original\.js:9:3/
   );
+  assert.match(output.stderr.toString(), /at Hello/);
 }
 
 // Applies source-maps generated by tsc to stack trace.
@@ -276,11 +279,14 @@ function nextdir() {
     require.resolve('../fixtures/source-map/webpack.js')
   ]);
   // Error in original context of source content:
-  assert.ok(
-    output.stderr.toString().match(/throw new Error\('oh no!'\)\r?\n.*\^/)
+  assert.match(
+    output.stderr.toString(),
+    /throw new Error\('oh no!'\)\r?\n.*\^/
   );
   // Rewritten stack trace:
-  assert.ok(output.stderr.toString().includes('webpack:///webpack.js:14:9'));
+  assert.match(output.stderr.toString(), /webpack:\/\/\/webpack\.js:14:9/);
+  assert.match(output.stderr.toString(), /at functionD.*14:9/);
+  assert.match(output.stderr.toString(), /at functionC.*10:3/);
 }
 
 // Stores and applies source map associated with file that throws while