Skip to content
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

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 29 additions & 5 deletions lib/repl.js
Original file line number Diff line number Diff line change
Expand Up @@ -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, '');
Copy link
Member

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?

Copy link
Member Author

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 a SyntaxError specifically, it's a string before we have an opportunity to filter the stack frames in prepareStackTrace(). So I left it rather than rework more of the overall error handling.

Copy link
Member Author

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 handle SyntaxError as a Recoverable 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.

} else if (isError && self.replMode === exports.REPL_MODE_STRICT) {
e.stack = e.stack.replace(/(\s+at\s+repl:)(\d+)/,
(_, pre, line) => pre + (line - 1));
Expand Down Expand Up @@ -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 ');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lance return frames.reverse() is sufficient?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@princejwesley Error.prepareStacktrace() is used to format the stack trace. It’s not required to return a string, but that is the behavior of the default implementation, and I think it would be unexpected if users suddenly started getting an array from the Error.stack property.

};
}

function _parseREPLKeyword(keyword, rest) {
var cmd = this.commands[keyword];
if (cmd) {
Expand Down Expand Up @@ -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 {
Expand Down
19 changes: 19 additions & 0 deletions test/fixtures/repl-pretty-stack.js
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();
79 changes: 79 additions & 0 deletions test/parallel/test-repl-pretty-custom-stack.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
'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!--->
repl:9:24--->
d (repl:12:3)--->
c (repl:9:3)--->
b (repl:6:3)--->
a (repl:3:3)
`
},
{
command: 'let x y;',
expected: `let x y;
^

SyntaxError: Unexpected identifier
`
},
{
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);
65 changes: 65 additions & 0 deletions test/parallel/test-repl-pretty-stack.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
'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!
at repl:9:24
at d (repl:12:3)
at c (repl:9:3)
at b (repl:6:3)
at a (repl:3:3)
`
},
{
command: 'let x y;',
expected: `let x y;
^

SyntaxError: Unexpected identifier

`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not a huge fan of the multi line template string and would rather see \n

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I disagree. Template strings, to my mind, tend to make the test example easier to scan/read quickly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know this was mainly discouraged so far and the code does not have any multi line template strings. I think it is more difficult to see whitespace with this. I do not want to block the PR with this comment though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm definitely not a fan of multi-line template strings and we've avoided them consistently in the past. It's definitely something that we can revisit. Ping @nodejs/tsc ... any feelings on this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's possible to align the first line with the others (maybe with a tagged literal that removes the first line terminator), then I'm +1.

},
{
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);
2 changes: 1 addition & 1 deletion test/parallel/test-repl.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ function clean_up() {
function strict_mode_error_test() {
send_expect([
{ client: client_unix, send: 'ref = 1',
expect: /^ReferenceError:\sref\sis\snot\sdefined\n\s+at\srepl:1:5/ },
expect: /^ReferenceError:\sref\sis\snot\sdefined\n/ },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the new end of the error? If yes, we might mark that in the RegExp and if not, I would just replace it with the new at.

]);
}

Expand Down