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.

Sign up for GitHub

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

Jump to bottom

repl: handle comments properly #3515

Closed
wants to merge 2 commits into from
Closed

repl: handle comments properly #3515

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5f656db
repl: handle comments properly
thefourtheye Oct 24, 2015
60aeee5
addressing targos's comments
thefourtheye Oct 26, 2015
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
172 changes: 94 additions & 78 deletions lib/repl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is for (... of ...) on a String? I was unaware that worked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Fishrock123 It actually works.

'use strict';

for (const chr of "abcd") {
  console.log(chr);
}

Copy link
Member

Choose a reason for hiding this comment

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

It works and has the advantage to handle unicode correctly:
image

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;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be simpler as a generator ala the readline parser? That way we can pause on comments for more data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Fishrock123 You mean like yielding tokens one at a time? We don't actually use the tokens as such, right? We just parse the line and find out if it has a string literal or a comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Eh, sounds fine.

}


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 @@ -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();
}
Expand All @@ -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');
Expand Down
28 changes: 28 additions & 0 deletions test/parallel/test-repl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
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