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

yield* is not properly captured inside of expressions #151

Closed
bmeck opened this issue Dec 9, 2014 · 18 comments
Closed

yield* is not properly captured inside of expressions #151

bmeck opened this issue Dec 9, 2014 · 18 comments

Comments

@bmeck
Copy link

bmeck commented Dec 9, 2014

The following code does not transform as expected:

function* one() {return 1};
function* two() {return yield* one() + yield* one(); };
var get=two();
var val = get.next();
var results = [val];
while (val.done == false) {
  val = get.next();
  results.push(val);
}
console.log(results);
@benjamn
Copy link
Collaborator

benjamn commented Dec 9, 2014

What do you expect?

@bmeck
Copy link
Author

bmeck commented Dec 9, 2014

  1. It should not compile since yield* is an assignment level expression, but does and leaves the second yield* in the generated code
  2. Just got explained how I need parens to do what I thought it would do since it is an assignment expression ala:
function* one() {return 1};
function* two() {return (yield* one()) + (yield* one()); };
var get=two();
var val = get.next();
var results = [val];
while (val.done == false) {
  val = get.next();
  results.push(val);
}
console.log(results);

@bmeck
Copy link
Author

bmeck commented Dec 9, 2014

need to do more research on this. spec BNF is confusing me.

this looks like it should tokenize as:

return yield* one() + yield * one()

@benjamn
Copy link
Collaborator

benjamn commented Dec 9, 2014

Here's the best I can do for a minimized testcase:

function* a() {
  return 1 + yield* f();
}

which transforms to

var a = regeneratorRuntime.mark(function a() {
  return regeneratorRuntime.wrap(function a$(context$1$0) {
    while (1) switch (context$1$0.prev = context$1$0.next) {
    case 0:
      return context$1$0.abrupt("return", 1 + yield* f());
    case 1:
    case "end":
      return context$1$0.stop();
    }
  }, a, this);
});

which is obviously wrong because the yield* is still there.

@benjamn
Copy link
Collaborator

benjamn commented Dec 9, 2014

Oh wow, Esprima is parsing yield* f() as a binary multiplication expression with * as the operator, so this behavior is arguably correct? I don't like it.

@bmeck
Copy link
Author

bmeck commented Dec 9, 2014

inside generators yield is a reserved word so this is def not correct

On Tue, Dec 9, 2014 at 11:38 AM, Ben Newman notifications@github.com
wrote:

Oh wow, Esprima is parsing yield* f() as a binary multiplication
expression with * as the operator, so this behavior is arguably correct?
I don't like it.


Reply to this email directly or view it on GitHub
#151 (comment)
.

@jmar777
Copy link

jmar777 commented Dec 9, 2014

V8 pukes as well:

$ node --harmony-generators -e 'function* a() { return 1 + yield* f(); }'
[eval]:1
function* a() { return 1 + yield* f(); }
                           ^^^^^
SyntaxError: Unexpected identifier
    at Object.exports.runInThisContext (vm.js:69:16)
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (module.js:449:26)
    at evalScript (node.js:428:25)
    at startup (node.js:85:7)
    at node.js:807:3

@jmar777
Copy link

jmar777 commented Dec 9, 2014

Although, maybe that should fail anyway (just perhaps with a different error message).

Due to operator precedence, wouldn't:

function* a() {
  return 1 + yield* f();
}

...be equivalent to:

function* a() {
  return (1 + yield)* f();
}

@domenic
Copy link

domenic commented Dec 9, 2014

Notably 1 + (yield* f()) works.

It seems like either the spec or V8 is disallowing 1 + yield* f(). I wonder if that is intentional? @allenwb could probably give us a quick answer?

@jmar777
Copy link

jmar777 commented Dec 9, 2014

Oops, I think operator precedence would actually make it equivalent to:

return 1 + (yield * <result-of-f()>);

@benjamn
Copy link
Collaborator

benjamn commented Dec 9, 2014

To clarify: by "arguably correct" I just meant the output from Regenerator is correct given the input from the parser, which definitely disagrees with V8 and the spec.

@bmeck
Copy link
Author

bmeck commented Dec 9, 2014

I think validity would depend if yield* is treated as a single token in generators, the BNF does not appear to keep them as the same token so multiplication is what I think is supposed to happen. Would want someone from spec to confirm this though. It would be a very sneaky wart if yield* is not a single token.

@allenwb
Copy link

allenwb commented Dec 9, 2014

The * in yield * is not a separate operator, instead yield * should be considered as a single unary operator. However, yield * must also be treated as two lexical tokens in that arbitrary (non-line terminating) white space is allowed between them. See the grammar at http://people.mozilla.org/~jorendorff/es6-draft.html#sec-generator-function-definitions

You may need to recognize them as two tokens in you lexer, but then combining into a single token for parsing the expression.

Of course this is all context sensitive and only applies generator function: http://people.mozilla.org/~jorendorff/es6-draft.html#sec-assignment-operators

@domenic
Copy link

domenic commented Dec 9, 2014

@allenwb so it sounds like 1 + yield* f() should indeed work, and V8 is wrong?

@allenwb
Copy link

allenwb commented Dec 9, 2014

@domenic no, because yield (and yield *) has the precedence of AssignmentExpression rather than UnaryExpression. So, you need to say 1 + (yield * f()).

1 + yield x or 1+ yield * f() are syntax errors because MultiplicativeExpression doesn't derive anything that begins with yield.

@domenic
Copy link

domenic commented Dec 9, 2014

Hmm I see. It feels strange then that yield* f() + 1 works. But thanks so much for helping us clear it up.

@bmeck
Copy link
Author

bmeck commented Dec 12, 2014

benjamn added a commit that referenced this issue Dec 20, 2014
For the record, #151 seems not to be a bug, and the parentheses around the
yield* expression are mandatory.
@ghost
Copy link

ghost commented Aug 4, 2015

Thank you for reporting this issue and appreciate your patience. We've notified the core team for an update on this issue. We're looking for a response within the next 30 days or the issue may be closed.

@bmeck bmeck closed this as completed Mar 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants