Closed Bug 981599 Opened 7 years ago Closed 6 years ago

Update parsing of 'yield' to match latest spec

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: anba, Assigned: wingo)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, feature, relnote, Whiteboard: [DocArea=JS])

Attachments

(1 file, 2 obsolete files)

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
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
Blocks: es6
Assignee: nobody → wingo
Attachment #8446503 - Flags: review?(jorendorff)
(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.
(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 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?
Attachment #8446503 - Flags: review?(jorendorff) → review+
(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.
(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.
Attachment #8446503 - Attachment is obsolete: true
Thanks for the review.  The updated patch incorporates your fine words in a comment in Parser.cpp and fixes the test comment.  Will commit.
Yes, I'm only concerned about 'yield' in generator comprehensions. Or in generator function default parameter expressions and similar obscure special cases.
Attachment #8446543 - Attachment is obsolete: true
Updated patch fixed comments.  Landed as https://hg.mozilla.org/integration/mozilla-inbound/rev/4fb43d3e1db1.
Flags: in-testsuite+
Target Milestone: --- → mozilla33
https://hg.mozilla.org/mozilla-central/rev/4fb43d3e1db1
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Keywords: feature, relnote
You need to log in before you can comment on or make changes to this bug.