From c14512b9a5661cf3f1275d4425880ec003fdcd89 Mon Sep 17 00:00:00 2001 From: bcoe Date: Sun, 8 Nov 2020 08:43:49 -0800 Subject: [PATCH] errors: display original symbol name If symbol names array has been populated in source map, include original symbol name in error message. Fixes https://github.com/nodejs/node/issues/35325 PR-URL: https://github.com/nodejs/node/pull/36042 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Antoine du Hamel --- doc/api/module.md | 1 + .../source_map/prepare_stack_trace.js | 124 ++++++++++++------ lib/internal/source_map/source_map.js | 16 ++- .../source-map/enclosing-call-site-min.js | 3 + .../source-map/enclosing-call-site.js | 27 ++++ .../source-map/enclosing-call-site.js.map | 8 ++ test/message/source_map_enclosing_function.js | 5 + .../message/source_map_enclosing_function.out | 20 +++ .../source_map_reference_error_tabs.out | 4 +- test/message/source_map_throw_catch.out | 4 +- test/message/source_map_throw_first_tick.out | 4 +- test/message/source_map_throw_icu.out | 4 +- .../source_map_throw_set_immediate.out | 4 +- test/parallel/test-source-map-enable.js | 20 ++- 14 files changed, 178 insertions(+), 66 deletions(-) create mode 100644 test/fixtures/source-map/enclosing-call-site-min.js create mode 100644 test/fixtures/source-map/enclosing-call-site.js create mode 100644 test/fixtures/source-map/enclosing-call-site.js.map create mode 100644 test/message/source_map_enclosing_function.js create mode 100644 test/message/source_map_enclosing_function.out diff --git a/doc/api/module.md b/doc/api/module.md index fdcb987d57bd26..03d235c74e5ae4 100644 --- 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 index 267f5dc91a9b4f..60bcb2fcadd1a1 100644 --- 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 index 7d21e02429a086..37912a2c9750bc 100644 --- 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 new file mode 100644 index 00000000000000..45b7ed2b219b86 --- /dev/null +++ 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 { + 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 new file mode 100644 index 00000000000000..d0c785f26091cc --- /dev/null +++ b/test/fixtures/source-map/enclosing-call-site.js.map @@ -0,0 +1,8 @@ +{ +"version":3, +"file":"enclosing-call-site-min.js", +"lineCount":1, +"mappings":"AAAA,IAAMA,UAAYA,QAAA,EAAM,CACtBC,SAAA,EADsB,CAIxBA,SAASA,UAAS,EAAG,CACnBC,SAAA,EADmB,CAIrB,IAAMA,UAAYA,QAAA,EAAM,CACtBC,SAAA,EADsB,CAAxB,CAIMA,UAAYA,QAAA,EAAM,CAEpB,GAAoB,CAApB,CAAIC,IAAA,CAAKC,MAAL,EAAJ,CACE,KAAUC,MAAJ,CAAU,WAAV,CAAN,CAHkB,CAJxB,CAYMC,QAAUP,SAEhB,IAAI,CACFO,SAAA,EADE,CAEF,MAAOC,CAAP,CAAY,CACZ,KAAMA,EAAN,CADY;", +"sources":["enclosing-call-site.js"], +"names":["functionA","functionB","functionC","functionD","Math","random","Error","thrower","err"] +} diff --git a/test/message/source_map_enclosing_function.js b/test/message/source_map_enclosing_function.js new file mode 100644 index 00000000000000..43b6931bf1e19e --- /dev/null +++ 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 new file mode 100644 index 00000000000000..4cb969dc38bf38 --- /dev/null +++ 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. (*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 index 24ef2ee21b7182..e0adcbc442aeef 100644 --- 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. (*tabs.coffee:39:5) - -> *tabs.coffee:26:2 + -> *tabs.coffee:26:2* at Object. (*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 index 5bc180c5c593e5..893e75f92bbf5c 100644 --- 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. (*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 index 37666b74dc5b04..4fc5aae55bf169 100644 --- 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. (*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 index c080bb94402353..ba5e94044cb2ed 100644 --- 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. (*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 index d3a795757f5e24..4893b856f0fe81 100644 --- 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. (*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 index cea597b4f7b0c0..6435aa99d2c752 100644 --- 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