Skip to content

Commit

Permalink
repl: handle comments properly
Browse files Browse the repository at this point in the history 
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: #3421
PR-URL: #3515
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
  • Loading branch information
@thefourtheye @jasnell
thefourtheye authored and jasnell committed Oct 28, 2015
1 parent 40bb802 commit d918838
Show file tree
Hide file tree
Showing 2 changed files with 122 additions and 78 deletions.
172 changes: 94 additions & 78 deletions lib/repl.js
View file
Expand Up @@ -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_,
Expand Down Expand Up @@ -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();
Expand All @@ -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 = [];

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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) {
Expand All @@ -406,15 +422,15 @@ 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;
}

// 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
Expand All @@ -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)
Expand Down Expand Up @@ -971,7 +987,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();
}
Expand All @@ -986,7 +1002,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');
Expand Down
28 changes: 28 additions & 0 deletions test/parallel/test-repl.js
View file
Expand Up @@ -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 },
]);
}

Expand Down

0 comments on commit d918838

Please sign in to comment.