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: backslashes confuse the multiline parser #2749

Closed
silverwind opened this issue Sep 9, 2015 · 10 comments
Closed

repl: backslashes confuse the multiline parser #2749

silverwind opened this issue Sep 9, 2015 · 10 comments
Labels
confirmed-bug Issues with confirmed bugs. repl Issues and PRs related to the REPL subsystem.

Comments

@silverwind
Copy link
Contributor

Pasting this into the REPL:

function x() {
  return '\n';
}

results in

> function x() {
...   return '\n';
SyntaxError: Unexpected end of input
    at Object.exports.createScript (vm.js:24:10)
    at REPLServer.defaultEval (repl.js:137:25)
    at bound (domain.js:250:14)
    at REPLServer.runBound [as eval] (domain.js:263:12)
    at REPLServer.<anonymous> (repl.js:392: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:546:8)
    at REPLServer.Interface._ttyWrite (readline.js:823:14)

cc: @thefourtheye
@silverwind silverwind added the repl Issues and PRs related to the REPL subsystem. label Sep 9, 2015
@thefourtheye
Copy link
Contributor

Mmmm, this is because the readline module parsing \ns as separate lines while parsing the pasted content. I'll look into it today. Glad you spotted it :-)

@silverwind
Copy link
Contributor Author

parsing \ns as separate lines

It's not just \ns, I actually encountered it with a \( first.

@silverwind
Copy link
Contributor Author

Looks to be a pretty recent regression, 0.12 works.

@silverwind silverwind added the confirmed-bug Issues with confirmed bugs. label Sep 9, 2015
@thefourtheye
Copy link
Contributor

@silverwind It's after this 81ea52a. Before that the string was accumulated no matter what, till the evaluation of accumulated string is successful. But that commit keeps track of the quotes to detect the string literals.

@vtenfys
Copy link

vtenfys commented Sep 16, 2015

Can confirm I got this problem too when trying the hello world server from the website about page in the REPL.

@thefourtheye
Copy link
Contributor

@silverwind I think we'll have to revert 81ea52a to fix this problem. cc @Fishrock123

@silverwind
Copy link
Contributor Author

👍 for the revert. Being unable to paste any multi-line code containing backslashes warrents it imho.

@silverwind
Copy link
Contributor Author

@thefourtheye want to attempt the revert yourself? (We could probably use some documentation for reverting)

thefourtheye added a commit to thefourtheye/io.js that referenced this issue Sep 18, 2015
@thefourtheye
repl: revert 81ea52a to fix newlines in copy-paste
6742c9b 
When some string is copied and pasted in REPL, if it has a newline (\n)
in it, then it is considered as end of line and REPL throws a
SyntaxError, because a line cannot end with a unclosed string literal.
This behavior is because of the fact that `readline` module breaks at
all the `\n`s in the input string and treats them as a seperate line.
So whenever it encounters a new line character it will emit a `line`
event.

This commit basically reverts
nodejs@81ea52a,
which was an attempt to improve the way string literals were handled
by REPL.

Fixes: nodejs#2749
@thefourtheye thefourtheye mentioned this issue Sep 18, 2015
Closed
@thefourtheye
Copy link
Contributor

@silverwind Sorry, I was out playing badminton :D Please take a look at the commit log of the bug fix and let me know if that has to be improved.

thefourtheye added a commit to thefourtheye/io.js that referenced this issue Sep 20, 2015
@thefourtheye
repl: backslash bug fix
8edd6b4 
The actual problem was with the line parsing logic for string literals.
When we use backslash in the string literals, it used to remember the
`\` as the previous character even after we parsed the character next
to it. This leads to REPL thinking that the end of string literals is
not reached.

This patch replaces the previous character with `null`, so that it will
properly skip the character next to it.

Previous Discussion: nodejs#2952
Fixes: nodejs#2749
@thefourtheye thefourtheye mentioned this issue Sep 20, 2015
Closed
thefourtheye added a commit that referenced this issue Sep 22, 2015
@thefourtheye @silverwind
repl: backslash bug fix
fcfd87d 
The actual problem was with the line parsing logic for string literals.
When we use backslash in the string literals, it used to remember the
`\` as the previous character even after we parsed the character next
to it. This leads to REPL thinking that the end of string literals is
not reached.

This patch replaces the previous character with `null`, so that it will
properly skip the character next to it.

Previous Discussion: #2952
Fixes: #2749
PR-URL: #2968
Reviewed-By: Roman Reiss <me@silverwind.io>
@silverwind
Copy link
Contributor Author

Fixed by fcfd87d

thefourtheye added a commit that referenced this issue Sep 22, 2015
@thefourtheye @rvagg
repl: backslash bug fix
cb971cc 
The actual problem was with the line parsing logic for string literals.
When we use backslash in the string literals, it used to remember the
`\` as the previous character even after we parsed the character next
to it. This leads to REPL thinking that the end of string literals is
not reached.

This patch replaces the previous character with `null`, so that it will
properly skip the character next to it.

Previous Discussion: #2952
Fixes: #2749
PR-URL: #2968
Reviewed-By: Roman Reiss <me@silverwind.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
confirmed-bug Issues with confirmed bugs. repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

repl: backslash bug fix thefourtheye/io.js
3 participants
@silverwind @thefourtheye @vtenfys