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

Generators in REPL can't be specified using function *gen() {} #9850

Closed
mgtitimoli opened this issue Nov 30, 2016 · 8 comments · Fixed by #9852
Closed

Generators in REPL can't be specified using function *gen() {} #9850

mgtitimoli opened this issue Nov 30, 2016 · 8 comments · Fixed by #9852
Assignees
Labels
confirmed-bug Issues with confirmed bugs. repl Issues and PRs related to the REPL subsystem.

Comments

@mgtitimoli
Copy link

mgtitimoli commented Nov 30, 2016

image

As you can see in the attached image, generators can only be specified with the star placed right next to the function keyword, as if it is placed in another place (right before the function name for example), it enters in multi-line edition mode.

@mscdex mscdex added the repl Issues and PRs related to the REPL subsystem. label Nov 30, 2016
@not-an-aardvark not-an-aardvark added the confirmed-bug Issues with confirmed bugs. label Nov 30, 2016
@not-an-aardvark
Copy link
Contributor

It looks like this affects v6+, but not v4.

@not-an-aardvark not-an-aardvark self-assigned this Nov 30, 2016
@princejwesley
Copy link
Contributor

@not-an-aardvark its a preprocess issue

@princejwesley
Copy link
Contributor

princejwesley commented Nov 30, 2016

@not-an-aardvark this may help

node 🙈 ₹ git:(upstream ⚡ repl.9743)  git diff
diff --git a/lib/repl.js b/lib/repl.js
index 70428ce..ff37cfb 100644
--- a/lib/repl.js
+++ b/lib/repl.js
@@ -249,8 +249,8 @@ function REPLServer(prompt,
       self.wrappedCmd = true;
     } else {
       // Mitigate https://github.com/nodejs/node/issues/548
-      cmd = cmd.replace(/^\s*function\s+([^(]+)/,
-                (_, name) => `var ${name} = function ${name}`);
+      cmd = cmd.replace(/^\s*function(\s+(?:\*\s*)?|\*\s+)([^(]+)/,
+                (_, gen, name) => `var ${name} = function${gen}${name}`);
     }
     // Append a \n so that it will be either
     // terminated, or continued onto the next expression if it's an
node 🙈 ₹ git:(upstream ⚡ repl.9743) 

@not-an-aardvark
Copy link
Contributor

@princejwesley Thanks, that's very helpful. However, I think your diff fails with:

function*foo() {}

because whitespace is not required before/after a generator star.

I think I have a fix; I'll make a PR shortly.

@princejwesley
Copy link
Contributor

@not-an-aardvark Remember to land your PR after this

@not-an-aardvark
Copy link
Contributor

not-an-aardvark commented Nov 30, 2016

While fixing this, I noticed another bug:

function foo() {} foo()

This is a SyntaxError in the REPL, but it's valid JS. It occurs because it gets preprocessed to

var foo = function foo() {} foo()

...which is a syntax error.

To fix this, we need to put a semicolon after the var foo = function foo() {} declaration. I'll add another commit to the PR.

@princejwesley
Copy link
Contributor

@not-an-aardvark User input can be a partial/multi-line function. We can't insert semicolon here. Here is one more edge case

@not-an-aardvark
Copy link
Contributor

Ah, good point. Okay, I'll just submit the PR with the generator function fix then.

not-an-aardvark added a commit to not-an-aardvark/node that referenced this issue Dec 2, 2016
Function declarations in the REPL are preprocessed into variable
declarations before being evaluated. However, the preprocessing logic
did not account for the star in a generator function declaration, which
caused the preprocessor to output invalid syntax in some circumstances.

PR-URL: nodejs#9852
Fixes: nodejs#9850
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
addaleax pushed a commit that referenced this issue Dec 5, 2016
Function declarations in the REPL are preprocessed into variable
declarations before being evaluated. However, the preprocessing logic
did not account for the star in a generator function declaration, which
caused the preprocessor to output invalid syntax in some circumstances.

PR-URL: #9852
Fixes: #9850
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
jmdarling pushed a commit to jmdarling/node that referenced this issue Dec 8, 2016
Function declarations in the REPL are preprocessed into variable
declarations before being evaluated. However, the preprocessing logic
did not account for the star in a generator function declaration, which
caused the preprocessor to output invalid syntax in some circumstances.

PR-URL: nodejs#9852
Fixes: nodejs#9850
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins pushed a commit that referenced this issue Dec 20, 2016
Function declarations in the REPL are preprocessed into variable
declarations before being evaluated. However, the preprocessing logic
did not account for the star in a generator function declaration, which
caused the preprocessor to output invalid syntax in some circumstances.

PR-URL: #9852
Fixes: #9850
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins pushed a commit that referenced this issue Dec 21, 2016
Function declarations in the REPL are preprocessed into variable
declarations before being evaluated. However, the preprocessing logic
did not account for the star in a generator function declaration, which
caused the preprocessor to output invalid syntax in some circumstances.

PR-URL: #9852
Fixes: #9850
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.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

Successfully merging a pull request may close this issue.

4 participants