From bd4311bc9cd2eaede03a99396ad0f396b83e8c4f Mon Sep 17 00:00:00 2001 From: Sakthipriyan Vairamani Date: Sat, 24 Oct 2015 13:16:08 +0530 Subject: [PATCH] repl: handle comments properly As it is, the comments are not handled properly in REPL. So, if the comments have `'` or `"`, then they are treated as incomplete string literals and the error is thrown in REPL. This patch refactors the existing logic and groups everything in a class. Fixes: https://github.com/nodejs/node/issues/3421 PR-URL: https://github.com/nodejs/node/pull/3515 Reviewed-By: Brian White Reviewed-By: Jeremiah Senkpiel --- lib/repl.js | 172 ++++++++++++++++++++----------------- test/parallel/test-repl.js | 28 ++++++ 2 files changed, 122 insertions(+), 78 deletions(-) diff --git a/lib/repl.js b/lib/repl.js index ed3df92e08526b..f17a9260772ebf 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -70,6 +70,88 @@ const BLOCK_SCOPED_ERROR = 'Block-scoped declarations (let, ' + 'const, function, class) not yet supported outside strict mode'; +class LineParser { + + constructor() { + this.reset(); + } + + reset() { + this._literal = null; + this.shouldFail = false; + this.blockComment = false; + } + + parseLine(line) { + var previous = null; + this.shouldFail = false; + const wasWithinStrLiteral = this._literal !== null; + + for (const current of line) { + if (previous === '\\') { + // valid escaping, skip processing. previous doesn't matter anymore + previous = null; + continue; + } + + if (!this._literal) { + if (previous === '*' && current === '/') { + if (this.blockComment) { + this.blockComment = false; + previous = null; + continue; + } else { + this.shouldFail = true; + break; + } + } + + // ignore rest of the line if `current` and `previous` are `/`s + if (previous === current && previous === '/' && !this.blockComment) { + break; + } + + if (previous === '/' && current === '*') { + this.blockComment = true; + previous = null; + } + } + + if (this.blockComment) continue; + + if (current === this._literal) { + this._literal = null; + } else if (current === '\'' || current === '"') { + this._literal = this._literal || current; + } + + previous = current; + } + + const isWithinStrLiteral = this._literal !== null; + + if (!wasWithinStrLiteral && !isWithinStrLiteral) { + // Current line has nothing to do with String literals, trim both ends + line = line.trim(); + } else if (wasWithinStrLiteral && !isWithinStrLiteral) { + // was part of a string literal, but it is over now, trim only the end + line = line.trimRight(); + } else if (isWithinStrLiteral && !wasWithinStrLiteral) { + // was not part of a string literal, but it is now, trim only the start + line = line.trimLeft(); + } + + const lastChar = line.charAt(line.length - 1); + + this.shouldFail = this.shouldFail || + ((!this._literal && lastChar === '\\') || + (this._literal && lastChar !== '\\')); + + return line; + } +} + + function REPLServer(prompt, stream, eval_, @@ -193,7 +275,7 @@ function REPLServer(prompt, debug('domain error'); const top = replMap.get(self); top.outputStream.write((e.stack || e) + '\n'); - top._currentStringLiteral = null; + top.lineParser.reset(); top.bufferedCommand = ''; top.lines.level = []; top.displayPrompt(); @@ -220,8 +302,7 @@ function REPLServer(prompt, self.outputStream = output; self.resetContext(); - // Initialize the current string literal found, to be null - self._currentStringLiteral = null; + self.lineParser = new LineParser(); self.bufferedCommand = ''; self.lines.level = []; @@ -280,87 +361,22 @@ function REPLServer(prompt, sawSIGINT = false; } - self._currentStringLiteral = null; + self.lineParser.reset(); self.bufferedCommand = ''; self.lines.level = []; self.displayPrompt(); }); - function parseLine(line, currentStringLiteral) { - var previous = null, current = null; - - for (var i = 0; i < line.length; i += 1) { - if (previous === '\\') { - // if it is a valid escaping, then skip processing and the previous - // character doesn't matter anymore. - previous = null; - continue; - } - - current = line.charAt(i); - if (current === currentStringLiteral) { - currentStringLiteral = null; - } else if (current === '\'' || - current === '"' && - currentStringLiteral === null) { - currentStringLiteral = current; - } - previous = current; - } - - return currentStringLiteral; - } - - function getFinisherFunction(cmd, defaultFn) { - if ((self._currentStringLiteral === null && - cmd.charAt(cmd.length - 1) === '\\') || - (self._currentStringLiteral !== null && - cmd.charAt(cmd.length - 1) !== '\\')) { - - // If the line continuation is used outside string literal or if the - // string continuation happens with out line continuation, then fail hard. - // Even if the error is recoverable, get the underlying error and use it. - return function(e, ret) { - var error = e instanceof Recoverable ? e.err : e; - - if (arguments.length === 2) { - // using second argument only if it is actually passed. Otherwise - // `undefined` will be printed when invalid REPL commands are used. - return defaultFn(error, ret); - } - - return defaultFn(error); - }; - } - return defaultFn; - } - self.on('line', function(cmd) { debug('line %j', cmd); sawSIGINT = false; var skipCatchall = false; - var finisherFn = finish; // leading whitespaces in template literals should not be trimmed. if (self._inTemplateLiteral) { self._inTemplateLiteral = false; } else { - const wasWithinStrLiteral = self._currentStringLiteral !== null; - self._currentStringLiteral = parseLine(cmd, self._currentStringLiteral); - const isWithinStrLiteral = self._currentStringLiteral !== null; - - if (!wasWithinStrLiteral && !isWithinStrLiteral) { - // Current line has nothing to do with String literals, trim both ends - cmd = cmd.trim(); - } else if (wasWithinStrLiteral && !isWithinStrLiteral) { - // was part of a string literal, but it is over now, trim only the end - cmd = cmd.trimRight(); - } else if (isWithinStrLiteral && !wasWithinStrLiteral) { - // was not part of a string literal, but it is now, trim only the start - cmd = cmd.trimLeft(); - } - - finisherFn = getFinisherFunction(cmd, finish); + cmd = self.lineParser.parseLine(cmd); } // Check to see if a REPL keyword was used. If it returns true, @@ -393,9 +409,9 @@ function REPLServer(prompt, } debug('eval %j', evalCmd); - self.eval(evalCmd, self.context, 'repl', finisherFn); + self.eval(evalCmd, self.context, 'repl', finish); } else { - finisherFn(null); + finish(null); } function finish(e, ret) { @@ -406,7 +422,7 @@ function REPLServer(prompt, self.outputStream.write('npm should be run outside of the ' + 'node repl, in your normal shell.\n' + '(Press Control-D to exit.)\n'); - self._currentStringLiteral = null; + self.lineParser.reset(); self.bufferedCommand = ''; self.displayPrompt(); return; @@ -414,7 +430,7 @@ function REPLServer(prompt, // If error was SyntaxError and not JSON.parse error if (e) { - if (e instanceof Recoverable) { + if (e instanceof Recoverable && !self.lineParser.shouldFail) { // Start buffering data like that: // { // ... x: 1 @@ -423,12 +439,12 @@ function REPLServer(prompt, self.displayPrompt(); return; } else { - self._domain.emit('error', e); + self._domain.emit('error', e.err || e); } } // Clear buffer if no SyntaxErrors - self._currentStringLiteral = null; + self.lineParser.reset(); self.bufferedCommand = ''; // If we got any output - print it (if no error) @@ -985,7 +1001,7 @@ function defineDefaultCommands(repl) { repl.defineCommand('break', { help: 'Sometimes you get stuck, this gets you out', action: function() { - this._currentStringLiteral = null; + this.lineParser.reset(); this.bufferedCommand = ''; this.displayPrompt(); } @@ -1000,7 +1016,7 @@ function defineDefaultCommands(repl) { repl.defineCommand('clear', { help: clearMessage, action: function() { - this._currentStringLiteral = null; + this.lineParser.reset(); this.bufferedCommand = ''; if (!this.useGlobal) { this.outputStream.write('Clearing context...\n'); diff --git a/test/parallel/test-repl.js b/test/parallel/test-repl.js index 43e144d87ce441..5234d8e009ee58 100644 --- a/test/parallel/test-repl.js +++ b/test/parallel/test-repl.js @@ -250,6 +250,34 @@ function error_test() { { client: client_unix, send: 'function x() {\nreturn \'\\\\\';\n }', expect: prompt_multiline + prompt_multiline + 'undefined\n' + prompt_unix }, + // regression tests for https://github.com/nodejs/node/issues/3421 + { client: client_unix, send: 'function x() {\n//\'\n }', + expect: prompt_multiline + prompt_multiline + + 'undefined\n' + prompt_unix }, + { client: client_unix, send: 'function x() {\n//"\n }', + expect: prompt_multiline + prompt_multiline + + 'undefined\n' + prompt_unix }, + { client: client_unix, send: 'function x() {//\'\n }', + expect: prompt_multiline + 'undefined\n' + prompt_unix }, + { client: client_unix, send: 'function x() {//"\n }', + expect: prompt_multiline + 'undefined\n' + prompt_unix }, + { client: client_unix, send: 'function x() {\nvar i = "\'";\n }', + expect: prompt_multiline + prompt_multiline + + 'undefined\n' + prompt_unix }, + { client: client_unix, send: 'function x(/*optional*/) {}', + expect: 'undefined\n' + prompt_unix }, + { client: client_unix, send: 'function x(/* // 5 */) {}', + expect: 'undefined\n' + prompt_unix }, + { client: client_unix, send: '// /* 5 */', + expect: 'undefined\n' + prompt_unix }, + { client: client_unix, send: '"//"', + expect: '\'//\'\n' + prompt_unix }, + { client: client_unix, send: '"data /*with*/ comment"', + expect: '\'data /*with*/ comment\'\n' + prompt_unix }, + { client: client_unix, send: 'function x(/*fn\'s optional params*/) {}', + expect: 'undefined\n' + prompt_unix }, + { client: client_unix, send: '/* \'\n"\n\'"\'\n*/', + expect: 'undefined\n' + prompt_unix }, ]); }