Update parsing of 'yield' to match latest spec

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: anba, Assigned: wingo)

Tracking

(Blocks 1 bug, {dev-doc-complete, feature, relnote})

Trunk
mozilla33
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [DocArea=JS])

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

5 years ago
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

Updated

5 years ago
Assignee: nobody → wingo
Assignee

Updated

5 years ago
Attachment #8446503 - Flags: review?(jorendorff)
Assignee

Comment 2

5 years ago
(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.
Reporter

Comment 3

5 years ago
(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.
Assignee

Comment 6

5 years ago
(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.
Assignee

Updated

5 years ago
Attachment #8446503 - Attachment is obsolete: true
Assignee

Comment 8

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

Comment 9

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

Updated

5 years ago
Attachment #8446543 - Attachment is obsolete: true
Assignee

Comment 11

5 years ago
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: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Keywords: feature, relnote
You need to log in before you can comment on or make changes to this bug.