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.

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

repl hates comments with quotes in indented code #3421

Closed
mscdex opened this issue Oct 18, 2015 · 8 comments
Closed

repl hates comments with quotes in indented code #3421

mscdex opened this issue Oct 18, 2015 · 8 comments
Labels
confirmed-bug Issues with confirmed bugs. repl Issues and PRs related to the REPL subsystem.

Comments

@mscdex
Copy link
Contributor

mscdex commented Oct 18, 2015

$ node
> function foo() {
... //'
SyntaxError: Unexpected end of input
    at Object.exports.createScript (vm.js:24:10)
    at REPLServer.defaultEval (repl.js:137:25)
    at bound (domain.js:280:14)
    at REPLServer.runBound [as eval] (domain.js:293:12)
    at REPLServer.<anonymous> (repl.js:393:12)
    at emitOne (events.js:82:20)
    at REPLServer.emit (events.js:169:7)
    at REPLServer.Interface._onLine (readline.js:210:10)
    at REPLServer.Interface._line (readline.js:549:8)
    at REPLServer.Interface._ttyWrite (readline.js:826:14)
>

Same happens with //"

@mscdex mscdex added the repl Issues and PRs related to the REPL subsystem. label Oct 18, 2015
@ChuckLangford
Copy link
Contributor

I played around with this code today and found there are two other scenarios where we can get this same error.

$ node
> function foo() { //'
SyntaxError: Unexpected end of input
    at Object.exports.createScript (vm.js:24:10)
    at REPLServer.defaultEval (repl.js:137:25)
    at bound (domain.js:280:14)
    at REPLServer.runBound [as eval] (domain.js:293:12)
    at REPLServer.<anonymous> (repl.js:393:12)
    at emitOne (events.js:82:20)
    at REPLServer.emit (events.js:169:7)
    at REPLServer.Interface._onLine (readline.js:210:10)
    at REPLServer.Interface._line (readline.js:549:8)
    at REPLServer.Interface._ttyWrite (readline.js:826:14)

The other scenario:

$ node
> function foo() {
... var i = "'";
SyntaxError: Unexpected end of input
    at Object.exports.createScript (vm.js:24:10)
    at REPLServer.defaultEval (repl.js:137:25)
    at bound (domain.js:280:14)
    at REPLServer.runBound [as eval] (domain.js:293:12)
    at REPLServer.<anonymous> (repl.js:393:12)
    at emitOne (events.js:82:20)
    at REPLServer.emit (events.js:169:7)
    at REPLServer.Interface._onLine (readline.js:210:10)
    at REPLServer.Interface._line (readline.js:549:8)
    at REPLServer.Interface._ttyWrite (readline.js:826:14)

@silverwind
Copy link
Contributor

cc @thefourtheye

@Fishrock123 Fishrock123 added the confirmed-bug Issues with confirmed bugs. label Oct 19, 2015
@ChuckLangford
Copy link
Contributor

After reviewing the repl code, I think I have a fix. I've included repl's parseLine function with my annotated changes below. Is this ready for a pull request @thefourtheye ?

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);

       // Issue 3421: If a comment is detected and there is no string
       // literal, then it's safe to stop parsing the rest of the line
       if (previous === '/' && current === '/' &&
           currentStringLiteral === null) {
         break;
       } else if (current === currentStringLiteral) {
         currentStringLiteral = null;
       } 
       // Issue 3421: Operator precedence caused a double quote string to fail
       // when a single quote was inside the double quote string.
       else if ((current === '\'' ||
                  current === '"') &&
                  currentStringLiteral === null) {
         currentStringLiteral = current;
       }
       previous = current;
     }

     return currentStringLiteral;
   }

@mscdex
Copy link
Contributor Author

mscdex commented Oct 22, 2015

@ChuckLangford I think it's going to be more complicated than that? For example, this fails also, but with a different exception:

> function foo(){
... /*'
SyntaxError: Unexpected token ILLEGAL
    at Object.exports.createScript (vm.js:24:10)
    at REPLServer.defaultEval (repl.js:137:25)
    at bound (domain.js:280:14)
    at REPLServer.runBound [as eval] (domain.js:293:12)
    at REPLServer.<anonymous> (repl.js:393:12)
    at emitOne (events.js:82:20)
    at REPLServer.emit (events.js:169:7)
    at REPLServer.Interface._onLine (readline.js:210:10)
    at REPLServer.Interface._line (readline.js:549:8)
    at REPLServer.Interface._ttyWrite (readline.js:826:14)
>

@thefourtheye
Copy link
Contributor

@ChuckLangford Yes, what @mscdex said is true. I am working on a fix with a little of bit of refactoring as well. I think I'll send a PR today or tomorrow.

@ChuckLangford
Copy link
Contributor

Awesome. Thanks for the feedback.

As a coding exercise, I'm going to continue working on this problem. @mscdex @thefourtheye have either of you found any other examples of comment related code that will crash repl?

@mscdex
Copy link
Contributor Author

mscdex commented Oct 22, 2015

@ChuckLangford I haven't had time to do any exhaustive testing yet, so it's possible there may be some other cases lurking...

@thefourtheye
Copy link
Contributor

PTAL at the proposed fix #3515

thefourtheye added a commit to thefourtheye/io.js that referenced this issue Oct 26, 2015
As it is, the comments are not handled properly in REPL. So, if the
comments have `'` or `"`, then they are treated as string literals and
the error is thrown in REPL.

This patch refactors the existing logic and groups everything in a
class.

Fixes: nodejs#3421
thefourtheye added a commit that referenced this issue Oct 28, 2015
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>
rvagg pushed a commit to rvagg/io.js that referenced this issue Oct 29, 2015
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: nodejs#3421
PR-URL: nodejs#3515
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
thefourtheye added a commit that referenced this issue Oct 29, 2015
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

No branches or pull requests

5 participants