981599 - Update parsing of 'yield' to match latest spec

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

Currently not supported in SpiderMonkey, but valid per current ES6 draft (rev22):

(1) Optional AssignmentExpression for yield
(2) No line terminator restriction for yield*
(3) yield as identifier in generator comprehensions


Test cases:

function* testCase1() {
  yield;
}
Expected: No SyntaxError
Actual: SyntaxError


function* testCase2() {
  yield
  *[];
}
Expected: SyntaxError
Actual: No SyntaxError


function testCase3_1() {
  // testCase3_1 is not a generator function!
  (for (v of []) yield);
}
function testCase3_2() {
  // testCase3_2 is not a generator function!
  (for (v of []) yield(0));
}
Expected: No SyntaxError
Actual: SyntaxError

Comment 1

[Andy Wingo [:wingo]]

Attachment: Update parsing of 'yield' to match latest spec

Comment 2

[Andy Wingo [:wingo]]

(In reply to André Bargull from comment #0)

> (3) yield as identifier in generator comprehensions
> function testCase3_1() {
>   // testCase3_1 is not a generator function!
>   (for (v of []) yield);
> }
> function testCase3_2() {
>   // testCase3_2 is not a generator function!
>   (for (v of []) yield(0));
> }
> Expected: No SyntaxError
> Actual: SyntaxError

This seems bogus to me.  Shouldn't this be prohibited by the spec?  It would lead to very confusing code.

Comment 3

[André Bargull [:anba]]

(In reply to Andy Wingo [:wingo] from comment #2)
> This seems bogus to me.  Shouldn't this be prohibited by the spec?  It would
> lead to very confusing code.

On reflection it may be more sensible to defer implementing the "'yield' as identifier" feature, because that part of the specification may not have received enough review yet. Well, unless you plan to check the various ways how the [Yield] and [GeneratorParameter] production parameters are propagated through the production rules. ;-)

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 8446503 [details] [diff] [review]
Update parsing of 'yield' to match latest spec

Review of attachment 8446503 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: js/src/frontend/Parser.cpp
@@ +4839,5 @@
> +          case TOK_RC:
> +          case TOK_RB:
> +          case TOK_RP:
> +          case TOK_COLON:
> +          case TOK_COMMA:

OK, I figured this out. It was perplexing to me because recursive descent parsers usually correspond so transparently to the BNF, and there's no list like this in the spec.

TOK_EOL is special; it implements the [no LineTerminator here] quirk in the grammar.

The rest of these make up the complete set of tokens that can appear after any of the places where AssignmentExpression is used throughout the grammar. Conveniently, none of them can also be the start an expression.

Can you add a comment about this? I seem to remember having reverse-engineered the intent before.

::: js/src/jit-test/tests/parser/yield-without-operand.js
@@ +1,1 @@
>  // yield without an operand causes a warning. See bug 885463.

Fix this comment?

Comment 5

[Jason Orendorff [:jorendorff]]

(In reply to André Bargull from comment #3)
> On reflection it may be more sensible to defer implementing the "'yield' as
> identifier" feature, because that part of the specification may not have
> received enough review yet.

It's a backwards compatibility feature. We can't not implement it.

But I can defer worrying about it.

Comment 6

[Andy Wingo [:wingo]]

(In reply to Jason Orendorff [:jorendorff] from comment #5)
> (In reply to André Bargull from comment #3)
> > On reflection it may be more sensible to defer implementing the "'yield' as
> > identifier" feature, because that part of the specification may not have
> > received enough review yet.
> 
> It's a backwards compatibility feature. We can't not implement it.
> 
> But I can defer worrying about it.

As I understand it, the feature that André is concerned about is using "yield" as an identifier *within an ES6 generator expression* -- so it's not really a legacy concern as such.  Yield can be prohibited as a keyword in generator expressions without breaking compatibility.

Comment 7

[Andy Wingo [:wingo]]

Attachment: Update parsing of 'yield' to match latest spec

Comment 8

[Andy Wingo [:wingo]]

Thanks for the review.  The updated patch incorporates your fine words in a comment in Parser.cpp and fixes the test comment.  Will commit.

Comment 9

[André Bargull [:anba]]

Yes, I'm only concerned about 'yield' in generator comprehensions. Or in generator function default parameter expressions and similar obscure special cases.

Comment 10

[Andy Wingo [:wingo]]

Attachment: Update parsing of 'yield' to match latest spec

Comment 11

[Andy Wingo [:wingo]]

Updated patch fixed comments.  Landed as https://hg.mozilla.org/integration/mozilla-inbound/rev/4fb43d3e1db1.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4fb43d3e1db1

Comment 13

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/Firefox/Releases/33#JavaScript

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/yield#Firefox-specific_notes
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/yield*#Firefox-specific_notes