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: exponentiation operator SyntaxError #7380

Closed
wants to merge 1 commit into from
Closed

repl: exponentiation operator SyntaxError #7380

wants to merge 1 commit into from

Conversation

italoacasas
Copy link
Contributor

@italoacasas italoacasas commented Jun 23, 2016

Checklist
  • make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
  • a test and/or benchmark is included
  • the commit message follows commit guidelines
Affected core subsystem(s)

repl

Description of change

description: #7372
more info: #7332

Cases to handle:
  • Inside a string literal
  • Ignore if is inside a comment
  • Later in the command string

I don't know if this is the best implementation, I never look in the repl.js file before, I would love to hear some feedback.

Sorry, something went wrong.

@nodejs-github-bot nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Jun 23, 2016
@mscdex
Copy link
Contributor

mscdex commented Jun 23, 2016

This kind of check makes a lot of assumptions that won't work for many cases (including ** inside a string literal or comment or ** that occurs later in the command string).

@Trott
Copy link
Member

Trott commented Jun 23, 2016

The SyntaxError message shouldn't be hard-coded into repl.js. It should come from v8 the same way the ReferenceError message comes from v8 and not repl.js here:

$ node
> foo
ReferenceError: foo is not defined
    at repl:1:1
    at REPLServer.defaultEval (repl.js:272:27)
    at bound (domain.js:280:14)
    at REPLServer.runBound [as eval] (domain.js:293:12)
    at REPLServer.<anonymous> (repl.js:441:10)
    at emitOne (events.js:101:20)
    at REPLServer.emit (events.js:188:7)
    at REPLServer.Interface._onLine (readline.js:224:10)
    at REPLServer.Interface._line (readline.js:566:8)
    at REPLServer.Interface._ttyWrite (readline.js:843:14)
>

@mscdex
Copy link
Contributor

mscdex commented Jun 23, 2016

Perhaps it's easiest to just whitelist exceptions with the .message of SyntaxError: Unexpected token * when the last command includes **? Although that may not necessarily be foolproof either...

On an unrelated note, it feels like these kinds of problems are always cropping up in one form or another with the repl. Maybe we need to just be able to parse javascript to avoid all of these kinds of issues?

@princejwesley
Copy link
Contributor

@mscdex Once editor mode PR is merged, we can add a provision to turn on/off multiline mode and turn it off by default. If multiline mode is turned off, all errors are non recoverable error.

@Trott
Copy link
Member

Trott commented Jun 24, 2016

@princejwesley Yes, that might be the solution to many of these sorts of issues that keep coming up. I will be very happy to see that land. (That's not to say that we shouldn't fix this for non-editor mode if we can do it reliably etc.)

@princejwesley
Copy link
Contributor

princejwesley commented Jun 26, 2016

@Trott Agreed. I have few concerns.

  • All error messages are specific to v8 engine. We may have to handle it differently for chakra(Per vm).
  • We can't deduce More Input Expected scenario reliably without having an incremental JS parser.
  • Even if we built an Incremental JS parser, some cases it will be ambiguous.

e.g.

let echo = e => e;
let x;
x = echo(2); // 2
x = echo // both echo and echo\n(2) are completely valid expression 
(2) // 2
Suggested Solution

Introduce Ctrl + Enter option to turn on multiline mode per line. (Hand over the problem to user 😄 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants