From 8febecb8babf40b6d763aa54138381a3b4e35880 Mon Sep 17 00:00:00 2001 From: Lance Ball Date: Mon, 11 Sep 2017 18:24:21 -0400 Subject: [PATCH 1/6] repl: remove internal frames from runtime errors When a user executes code in the REPLServer which generates an exception, there is no need to display the REPLServer internal stack frames. --- lib/repl.js | 17 ++++++-- test/fixtures/repl-pretty-stack.js | 15 +++++++ test/parallel/test-repl-pretty-stack.js | 58 +++++++++++++++++++++++++ test/parallel/test-repl.js | 2 +- 4 files changed, 88 insertions(+), 4 deletions(-) create mode 100644 test/fixtures/repl-pretty-stack.js create mode 100644 test/parallel/test-repl-pretty-stack.js diff --git a/lib/repl.js b/lib/repl.js index 0d1aca6aa546c9..977e9a9dbcf1d8 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -209,9 +209,9 @@ function REPLServer(prompt, // preserve original error for wrapped command const error = wrappedErr || e; if (isRecoverableError(error, code)) - err = new Recoverable(error); + err = prettifyStackTrace(new Recoverable(error)); else - err = error; + err = prettifyStackTrace(error); } break; } @@ -263,7 +263,7 @@ function REPLServer(prompt, if (err && process.domain) { debug('not recoverable, send to domain'); - process.domain.emit('error', err); + process.domain.emit('error', prettifyStackTrace(err)); process.domain.exit(); return; } @@ -374,6 +374,17 @@ function REPLServer(prompt, }; } + function prettifyStackTrace(err) { + if (err.stack) { + const regex = new RegExp(/\s+repl:/); + const stack = err.stack.split('\n'); + const idx = stack.findIndex((frame) => regex.test(frame)); + if (idx > 0) + err.stack = stack.splice(0, idx).join('\n'); + } + return err; + } + function _parseREPLKeyword(keyword, rest) { var cmd = this.commands[keyword]; if (cmd) { diff --git a/test/fixtures/repl-pretty-stack.js b/test/fixtures/repl-pretty-stack.js new file mode 100644 index 00000000000000..3c090d0428b1fb --- /dev/null +++ b/test/fixtures/repl-pretty-stack.js @@ -0,0 +1,15 @@ +'use strict'; + +function a() { + b(); +} + +function b() { + c(); +} + +function c() { + throw new Error('Whoops!'); +} + +a(); \ No newline at end of file diff --git a/test/parallel/test-repl-pretty-stack.js b/test/parallel/test-repl-pretty-stack.js new file mode 100644 index 00000000000000..770692e74c440e --- /dev/null +++ b/test/parallel/test-repl-pretty-stack.js @@ -0,0 +1,58 @@ +'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 c (repl:9:9) + at b (repl:6:3) + at 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' + } +]; + +tests.forEach(run); diff --git a/test/parallel/test-repl.js b/test/parallel/test-repl.js index 6df856a20ec410..9d0c6ede9e5ea0 100644 --- a/test/parallel/test-repl.js +++ b/test/parallel/test-repl.js @@ -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/ }, ]); } From 1dd23f68590257ebeb76f9445891952cbbadb224 Mon Sep 17 00:00:00 2001 From: Lance Ball Date: Tue, 12 Sep 2017 17:01:58 -0400 Subject: [PATCH 2/6] repl: use Error.prepareStackTrace to make pretty --- lib/repl.js | 27 ++++++++++++++----------- test/fixtures/repl-pretty-stack.js | 6 +++++- test/parallel/test-repl-pretty-stack.js | 9 ++++++++- 3 files changed, 28 insertions(+), 14 deletions(-) diff --git a/lib/repl.js b/lib/repl.js index 977e9a9dbcf1d8..e63a39cd462641 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -209,9 +209,9 @@ function REPLServer(prompt, // preserve original error for wrapped command const error = wrappedErr || e; if (isRecoverableError(error, code)) - err = prettifyStackTrace(new Recoverable(error)); + err = new Recoverable(error); else - err = prettifyStackTrace(error); + err = error; } break; } @@ -263,7 +263,7 @@ function REPLServer(prompt, if (err && process.domain) { debug('not recoverable, send to domain'); - process.domain.emit('error', prettifyStackTrace(err)); + process.domain.emit('error', err); process.domain.exit(); return; } @@ -374,17 +374,20 @@ function REPLServer(prompt, }; } - function prettifyStackTrace(err) { - if (err.stack) { - const regex = new RegExp(/\s+repl:/); - const stack = err.stack.split('\n'); - const idx = stack.findIndex((frame) => regex.test(frame)); - if (idx > 0) - err.stack = stack.splice(0, idx).join('\n'); - } - return err; + function prettifyStackTrace(error, structuredStack) { + // search from the bottom of the call stack to + // find the first frame with a null function name + const idx = structuredStack.reverse().findIndex( + (frame) => frame.getFunctionName() === null); + + // if found, get rid of it and everything below it + structuredStack = structuredStack.splice(idx + 1); + structuredStack.push(error); + return structuredStack.reverse().join('\n at '); } + Error.prepareStackTrace = prettifyStackTrace; + function _parseREPLKeyword(keyword, rest) { var cmd = this.commands[keyword]; if (cmd) { diff --git a/test/fixtures/repl-pretty-stack.js b/test/fixtures/repl-pretty-stack.js index 3c090d0428b1fb..26e72beecaefdd 100644 --- a/test/fixtures/repl-pretty-stack.js +++ b/test/fixtures/repl-pretty-stack.js @@ -9,7 +9,11 @@ function b() { } function c() { - throw new Error('Whoops!'); + d(function() { throw new Error('Whoops!'); }); +} + +function d(f) { + f(); } a(); \ No newline at end of file diff --git a/test/parallel/test-repl-pretty-stack.js b/test/parallel/test-repl-pretty-stack.js index 770692e74c440e..9c150c9fcb413c 100644 --- a/test/parallel/test-repl-pretty-stack.js +++ b/test/parallel/test-repl-pretty-stack.js @@ -31,7 +31,9 @@ const tests = [ // test .load for a file that throws command: `.load ${fixtures.path('repl-pretty-stack.js')}`, expected: `Error: Whoops! - at c (repl:9:9) + at repl:9:24 + at d (repl:12:3) + at c (repl:9:3) at b (repl:6:3) at a (repl:3:3) ` @@ -52,6 +54,11 @@ SyntaxError: Unexpected identifier { 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' } ]; From 59e2637b78151127d4af41de8ed52661e69deb96 Mon Sep 17 00:00:00 2001 From: Lance Ball Date: Tue, 12 Sep 2017 17:27:56 -0400 Subject: [PATCH 3/6] repl: inline Error.prepareStackTrace function --- lib/repl.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/repl.js b/lib/repl.js index e63a39cd462641..3b76a1ef6a2ec7 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -374,7 +374,7 @@ function REPLServer(prompt, }; } - function prettifyStackTrace(error, structuredStack) { + Error.prepareStackTrace = function prepareStackTrace(error, structuredStack) { // search from the bottom of the call stack to // find the first frame with a null function name const idx = structuredStack.reverse().findIndex( @@ -384,9 +384,7 @@ function REPLServer(prompt, structuredStack = structuredStack.splice(idx + 1); structuredStack.push(error); return structuredStack.reverse().join('\n at '); - } - - Error.prepareStackTrace = prettifyStackTrace; + }; function _parseREPLKeyword(keyword, rest) { var cmd = this.commands[keyword]; From 99b2187434fd46c9a85bed6ca0076b3430ce851e Mon Sep 17 00:00:00 2001 From: Lance Ball Date: Tue, 26 Sep 2017 20:37:34 -0400 Subject: [PATCH 4/6] repl: allow user-defined Error.prepareStackTrace --- lib/repl.js | 30 ++++--- .../parallel/test-repl-pretty-custom-stack.js | 79 +++++++++++++++++++ 2 files changed, 100 insertions(+), 9 deletions(-) create mode 100644 test/parallel/test-repl-pretty-custom-stack.js diff --git a/lib/repl.js b/lib/repl.js index 3b76a1ef6a2ec7..42d50ae627e303 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -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,17 +376,29 @@ function REPLServer(prompt, }; } - Error.prepareStackTrace = function prepareStackTrace(error, structuredStack) { + 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); - structuredStack.push(error); - return structuredStack.reverse().join('\n at '); - }; + 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 '); + }; + } function _parseREPLKeyword(keyword, rest) { var cmd = this.commands[keyword]; @@ -954,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 { diff --git a/test/parallel/test-repl-pretty-custom-stack.js b/test/parallel/test-repl-pretty-custom-stack.js new file mode 100644 index 00000000000000..0104b9529e10be --- /dev/null +++ b/test/parallel/test-repl-pretty-custom-stack.js @@ -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); From 63fc83e8d99b5b82b922d08af579b470ad603fd6 Mon Sep 17 00:00:00 2001 From: Lance Ball Date: Fri, 29 Sep 2017 11:01:32 -0400 Subject: [PATCH 5/6] repl: remove templated strings from stack tests --- test/parallel/test-repl-pretty-custom-stack.js | 15 +++------------ test/parallel/test-repl-pretty-stack.js | 16 +++------------- 2 files changed, 6 insertions(+), 25 deletions(-) diff --git a/test/parallel/test-repl-pretty-custom-stack.js b/test/parallel/test-repl-pretty-custom-stack.js index 0104b9529e10be..be102c1d677a9c 100644 --- a/test/parallel/test-repl-pretty-custom-stack.js +++ b/test/parallel/test-repl-pretty-custom-stack.js @@ -45,21 +45,12 @@ 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) -` + 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; - ^ - -SyntaxError: Unexpected identifier -` + expected: 'let x y;\n ^\n\nSyntaxError: Unexpected identifier\n' }, { command: 'throw new Error(\'Whoops!\')', diff --git a/test/parallel/test-repl-pretty-stack.js b/test/parallel/test-repl-pretty-stack.js index 9c150c9fcb413c..0fc6b3ada04c79 100644 --- a/test/parallel/test-repl-pretty-stack.js +++ b/test/parallel/test-repl-pretty-stack.js @@ -30,22 +30,12 @@ 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) -` + 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; - ^ - -SyntaxError: Unexpected identifier - -` + expected: 'let x y;\n ^\n\nSyntaxError: Unexpected identifier\n\n' }, { command: 'throw new Error(\'Whoops!\')', From f710db82f7b2d86b0f936dd6eb201b72f94b775c Mon Sep 17 00:00:00 2001 From: Lance Ball Date: Fri, 29 Sep 2017 13:52:42 -0400 Subject: [PATCH 6/6] repl: more precise error checking --- test/parallel/test-repl.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-repl.js b/test/parallel/test-repl.js index 9d0c6ede9e5ea0..54ad848b5b3927 100644 --- a/test/parallel/test-repl.js +++ b/test/parallel/test-repl.js @@ -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/ }, + expect: /^ReferenceError:\sref\sis\snot\sdefined\nnode via Unix socket> $/ }, ]); }