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 <mscdex@mscdex.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

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.lineParser = new LineParser();
-  self._currentStringLiteral = null;
   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;
+      cmd = self.lineParser.parseLine(cmd);
-      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);
     }
 
     // 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');

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 },
   ]);
 }