-
Notifications
You must be signed in to change notification settings - Fork 29.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
repl: remove internal frames from runtime errors #15351
Changes from all commits
8febecb
1dd23f6
59e2637
99b2187
63fc83e
f710db8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -284,14 +284,16 @@ function REPLServer(prompt, | |
self._domain.on('error', function debugDomainError(e) { | ||
debug('domain error'); | ||
const top = replMap.get(self); | ||
|
||
const pstrace = Error.prepareStackTrace; | ||
Error.prepareStackTrace = prepareStackTrace(pstrace); | ||
internalUtil.decorateErrorStack(e); | ||
Error.prepareStackTrace = pstrace; | ||
const isError = internalUtil.isError(e); | ||
if (e instanceof SyntaxError && e.stack) { | ||
// remove repl:line-number and stack trace | ||
e.stack = e.stack | ||
.replace(/^repl:\d+\r?\n/, '') | ||
.replace(/^\s+at\s.*\n?/gm, ''); | ||
.replace(/^repl:\d+\r?\n/, '') | ||
.replace(/^\s+at\s.*\n?/gm, ''); | ||
} else if (isError && self.replMode === exports.REPL_MODE_STRICT) { | ||
e.stack = e.stack.replace(/(\s+at\s+repl:)(\d+)/, | ||
(_, pre, line) => pre + (line - 1)); | ||
|
@@ -374,6 +376,30 @@ function REPLServer(prompt, | |
}; | ||
} | ||
|
||
function filterInternalStackFrames(error, structuredStack) { | ||
// search from the bottom of the call stack to | ||
// find the first frame with a null function name | ||
if (typeof structuredStack !== 'object') | ||
return structuredStack; | ||
const idx = structuredStack.reverse().findIndex( | ||
(frame) => frame.getFunctionName() === null); | ||
|
||
// if found, get rid of it and everything below it | ||
structuredStack = structuredStack.splice(idx + 1); | ||
return structuredStack; | ||
} | ||
|
||
function prepareStackTrace(fn) { | ||
return (error, stackFrames) => { | ||
const frames = filterInternalStackFrames(error, stackFrames); | ||
if (fn) { | ||
return fn(error, frames); | ||
} | ||
frames.push(error); | ||
return frames.reverse().join('\n at '); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lance There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @princejwesley |
||
}; | ||
} | ||
|
||
function _parseREPLKeyword(keyword, rest) { | ||
var cmd = this.commands[keyword]; | ||
if (cmd) { | ||
|
@@ -942,8 +968,6 @@ function complete(line, callback) { | |
} else { | ||
const evalExpr = `try { ${expr} } catch (e) {}`; | ||
this.eval(evalExpr, this.context, 'repl', (e, obj) => { | ||
// if (e) console.log(e); | ||
|
||
if (obj != null) { | ||
if (typeof obj === 'object' || typeof obj === 'function') { | ||
try { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
'use strict'; | ||
|
||
function a() { | ||
b(); | ||
} | ||
|
||
function b() { | ||
c(); | ||
} | ||
|
||
function c() { | ||
d(function() { throw new Error('Whoops!'); }); | ||
} | ||
|
||
function d(f) { | ||
f(); | ||
} | ||
|
||
a(); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
'use strict'; | ||
const common = require('../common'); | ||
const fixtures = require('../common/fixtures'); | ||
const assert = require('assert'); | ||
const repl = require('repl'); | ||
|
||
|
||
function run({ command, expected }) { | ||
let accum = ''; | ||
|
||
const inputStream = new common.ArrayStream(); | ||
const outputStream = new common.ArrayStream(); | ||
|
||
outputStream.write = (data) => accum += data.replace('\r', ''); | ||
|
||
const r = repl.start({ | ||
prompt: '', | ||
input: inputStream, | ||
output: outputStream, | ||
terminal: false, | ||
useColors: false | ||
}); | ||
|
||
r.write(`${command}\n`); | ||
assert.strictEqual(accum, expected); | ||
r.close(); | ||
} | ||
|
||
const origPrepareStackTrace = Error.prepareStackTrace; | ||
Error.prepareStackTrace = (err, stack) => { | ||
if (err instanceof SyntaxError) | ||
return err.toString(); | ||
stack.push(err); | ||
return stack.reverse().join('--->\n'); | ||
}; | ||
|
||
process.on('uncaughtException', (e) => { | ||
Error.prepareStackTrace = origPrepareStackTrace; | ||
throw e; | ||
}); | ||
|
||
process.on('exit', () => (Error.prepareStackTrace = origPrepareStackTrace)); | ||
|
||
const tests = [ | ||
{ | ||
// test .load for a file that throws | ||
command: `.load ${fixtures.path('repl-pretty-stack.js')}`, | ||
expected: 'Error: Whoops!--->\nrepl:9:24--->\nd (repl:12:3)--->\nc ' + | ||
'(repl:9:3)--->\nb (repl:6:3)--->\na (repl:3:3)\n' | ||
}, | ||
{ | ||
command: 'let x y;', | ||
expected: 'let x y;\n ^\n\nSyntaxError: Unexpected identifier\n' | ||
}, | ||
{ | ||
command: 'throw new Error(\'Whoops!\')', | ||
expected: 'Error: Whoops!\n' | ||
}, | ||
{ | ||
command: 'foo = bar;', | ||
expected: 'ReferenceError: bar is not defined\n' | ||
}, | ||
// test anonymous IIFE | ||
{ | ||
command: '(function() { throw new Error(\'Whoops!\'); })()', | ||
expected: 'Error: Whoops!--->\nrepl:1:21\n' | ||
} | ||
]; | ||
|
||
tests.forEach(run); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
'use strict'; | ||
const common = require('../common'); | ||
const fixtures = require('../common/fixtures'); | ||
const assert = require('assert'); | ||
const repl = require('repl'); | ||
|
||
|
||
function run({ command, expected }) { | ||
let accum = ''; | ||
|
||
const inputStream = new common.ArrayStream(); | ||
const outputStream = new common.ArrayStream(); | ||
|
||
outputStream.write = (data) => accum += data.replace('\r', ''); | ||
|
||
const r = repl.start({ | ||
prompt: '', | ||
input: inputStream, | ||
output: outputStream, | ||
terminal: false, | ||
useColors: false | ||
}); | ||
|
||
r.write(`${command}\n`); | ||
assert.strictEqual(accum, expected); | ||
r.close(); | ||
} | ||
|
||
const tests = [ | ||
{ | ||
// test .load for a file that throws | ||
command: `.load ${fixtures.path('repl-pretty-stack.js')}`, | ||
expected: 'Error: Whoops!\n at repl:9:24\n at d (repl:12:3)\n ' + | ||
'at c (repl:9:3)\n at b (repl:6:3)\n at a (repl:3:3)\n' | ||
}, | ||
{ | ||
command: 'let x y;', | ||
expected: 'let x y;\n ^\n\nSyntaxError: Unexpected identifier\n\n' | ||
}, | ||
{ | ||
command: 'throw new Error(\'Whoops!\')', | ||
expected: 'Error: Whoops!\n' | ||
}, | ||
{ | ||
command: 'foo = bar;', | ||
expected: 'ReferenceError: bar is not defined\n' | ||
}, | ||
// test anonymous IIFE | ||
{ | ||
command: '(function() { throw new Error(\'Whoops!\'); })()', | ||
expected: 'Error: Whoops!\n at repl:1:21\n' | ||
} | ||
]; | ||
|
||
tests.forEach(run); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still necessary with the
prepareStackTrace
function in place? And if yes, cant we just move that part in the prepareStackTrace function as well?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can poke around on it more - the change in formatting is because that's what I was doing and missed noticing that on my local diff. It's tricky because of the way
Error.prepareStackTrace()
works. It's only ever called once for a given error, and the return value is by default (and typically in custom impls as well) a string. Because of the way errors are handled in the repl and domain, in the case of aSyntaxError
specifically, it's a string before we have an opportunity to filter the stack frames inprepareStackTrace()
. So I left it rather than rework more of the overall error handling.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BridgeAR I spent the morning looking into removing this and dealing with it in
Error.prepareStackTrace()
but the ramifications were wide reaching, especially with regard to how we handleSyntaxError
as aRecoverable
object so that defaulting into editor mode on multi-line input works as expected. I do think there could be some consolidation and overall improvement of the error handling, but I don't think this is the PR to do that in.