Bug 170979 – test262: test262/test/language/expressions/generators/yield-as-label.js

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Joseph Pecoraro]

test262/test/language/expressions/generators/yield-as-label.js

Test:

    <script>
    function* foo() {
        yield: 1;
    }
    </script>
    <script>
    "use strict";
    yield: 1;
    </script>

Expected:
SyntaxError: Cannot use yield as a label in a general function

Actual:
No error.

Notes:
- Chrome:
    Uncaught SyntaxError: Unexpected token :
    Uncaught SyntaxError: Unexpected strict mode reserved word
- Firefox:
    SyntaxError: yield is a reserved identifier
    SyntaxError: yield is a reserved identifier

Spec:

https://tc39.github.io/ecma262/#sec-identifiers-static-semantics-early-errors
12.1.1 Static Semantics: Early Errors

> IdentifierReference : yield
> BindingIdentifier : yield
> LabelIdentifier : yield
> 
>     It is a Syntax Error if the code matched by this production is contained in strict mode code.
> 
> IdentifierReference : Identifier
> BindingIdentifier : Identifier
> LabelIdentifier : Identifier
> 
>     It is a Syntax Error if this production has a [Yield] parameter and StringValue of Identifier is "yield".
>     It is a Syntax Error if this production has an [Await] parameter and StringValue of Identifier is "await".

The second set of cases preventing the use of the keyword inside of generators which have the [Yield] parameter.

[Joseph Pecoraro]

While the label case turned out to be easy, checking surrounding code turned out to be hard. The particular interesting case is GeneratorDeclaration versus GeneratorExpression:

https://tc39.github.io/ecma262/#sec-generator-function-definitions
14.4 Generator Function Definitions

> GeneratorMethod[Yield, Await]:
>     * PropertyName[?Yield, ?Await] ( UniqueFormalParameters[+Yield, ~Await] ) { GeneratorBody }

Here the function name 'yield' is allowed because PropertyName doesn't prevent it.

> GeneratorDeclaration[Yield, Await, Default]:
>     function * BindingIdentifier[?Yield, ?Await] ( FormalParameters[+Yield, ~Await] ) { GeneratorBody }
>     [+Default] function * (FormalParameters[+Yield, ~Await]) { GeneratorBody }

Here the function name 'yield' is allowed because BindingIdentifier[?Yield].

> GeneratorExpression:
>     function* BindingIdentifier[+Yield, ~Await]opt ( FormalParameters[+Yield, ~Await] ) { GeneratorBody }

Here the function name 'yield' is not allowed because BindingIdentifier[+Yield].

--

In all cases:

- the name yield is not allowed in parameters or GeneratorBody.
- yield is not allowed in nested cases (because we would be +Yield from the start)

[Joseph Pecoraro]

Created attachment 307470 [details]
[PATCH] Proposed Fix

If you want I can probably split this up into 2 patches. (Can't => Cannot) and (yield).

[Joseph Pecoraro]

Created attachment 307471 [details]
[PATCH] Proposed Fix

Rebaseline a few other tests.

[Joseph Pecoraro]

Comment on attachment 307471 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=307471&action=review

> JSTests/stress/yield-reserved-word.js:80
> +// GeneratorDeclaration inside a generator now has +Yield.
> +checkClassicSyntaxError(`
> +function* foo() { function* yield(){} }
> +`, `SyntaxError: Cannot use the keyword 'yield' as a generator function name.`);

Clearing review because I want to add more tests like this for nested functions to ensure that yield inside of a (Function / ArrowFunction / AsyncFunction) nested inside of a Generator function is allowed.

I'll also use that opportunity to break this up into a couple patches to avoid the clutter it currently has.

[Joseph Pecoraro]

Created attachment 307579 [details]
[PATCH] Proposed Fix

Wrote far more exhaustive tests and discovered another bug! (Bug: 171048)

[Joseph Pecoraro]

Comment on attachment 307579 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=307579&action=review

> JSTests/stress/yield-reserved-word.js:158
> +// FIXME: <https://webkit.org/b/171048> Should not be able to use a keyword in object literal shorthand syntax
> +// checkClassicSyntaxError(`
> +// function *get() { var o = { yield }; }
> +// `, `SyntaxError: ...);

I'm going to address this here. Since I have the patch for it and it modified other related error messages.

[Joseph Pecoraro]

*** Bug 171048 has been marked as a duplicate of this bug. ***

[Joseph Pecoraro]

Created attachment 307760 [details]
[PATCH] Proposed Fix

Getting pretty bigger now. This now covers a bunch of couple issues with keywords.

[Build Bot]

Comment on attachment 307760 [details]
[PATCH] Proposed Fix

Attachment 307760 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/3577585

New failing tests:
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-no-cjit
stress/catch-parameter-syntax.js.ftl-no-cjit-validate-sampling-profiler
stress/catch-parameter-syntax.js.ftl-no-cjit-no-inline-validate
stress/catch-parameter-syntax.js.ftl-no-cjit-b3o1
stress/yield-named-variable-generator.js.ftl-no-cjit-small-pool
stress/yield-named-variable-generator.js.dfg-eager
stress/catch-parameter-syntax.js.default
jsc-layout-tests.yaml/js/script-tests/let-syntax.js.layout-ftl-no-cjit
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-ftl-no-cjit
jsc-layout-tests.yaml/js/script-tests/let-syntax.js.layout
stress/yield-named-variable-generator.js.ftl-no-cjit-validate-sampling-profiler
stress/yield-named-variable-generator.js.no-llint
stress/yield-named-variable.js.ftl-no-cjit-b3o1
stress/yield-named-variable.js.dfg-eager-no-cjit-validate
stress/catch-parameter-syntax.js.no-llint
stress/yield-named-variable.js.default
stress/catch-parameter-syntax.js.ftl-no-cjit-small-pool
stress/yield-named-variable.js.no-cjit-collect-continuously
stress/yield-named-variable.js.ftl-no-cjit-no-inline-validate
jsc-layout-tests.yaml/js/script-tests/let-syntax.js.layout-dfg-eager-no-cjit
stress/catch-parameter-syntax.js.no-cjit-validate-phases
stress/catch-parameter-syntax.js.ftl-eager-no-cjit-b3o1
stress/yield-named-variable.js.ftl-no-cjit-validate-sampling-profiler
stress/catch-parameter-syntax.js.ftl-eager-no-cjit
stress/catch-parameter-syntax.js.no-ftl
stress/yield-named-variable-generator.js.ftl-no-cjit-no-inline-validate
stress/yield-named-variable.js.no-cjit-validate-phases
stress/yield-named-variable.js.ftl-eager-no-cjit-b3o1
stress/yield-named-variable-generator.js.default
jsc-layout-tests.yaml/js/script-tests/let-syntax.js.layout-no-llint
stress/yield-named-variable.js.ftl-no-cjit-small-pool
stress/yield-named-variable.js.no-llint
stress/yield-named-variable-generator.js.dfg-maximal-flush-validate-no-cjit
stress/catch-parameter-syntax.js.ftl-no-cjit-no-put-stack-validate
jsc-layout-tests.yaml/js/script-tests/let-syntax.js.layout-ftl-eager-no-cjit
ChakraCore.yaml/ChakraCore/test/strict/23.reservedWords_sm.js.default
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-no-ftl
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-dfg-eager-no-cjit
stress/catch-parameter-syntax.js.no-cjit-collect-continuously
stress/yield-named-variable-generator.js.ftl-no-cjit-no-put-stack-validate
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout
jsc-layout-tests.yaml/js/script-tests/let-syntax.js.layout-no-ftl
stress/yield-named-variable-generator.js.dfg-eager-no-cjit-validate
stress/yield-named-variable-generator.js.ftl-eager-no-cjit
stress/yield-named-variable-generator.js.ftl-eager
stress/yield-named-variable-generator.js.no-cjit-collect-continuously
stress/yield-named-variable.js.dfg-eager
stress/yield-named-variable-generator.js.ftl-no-cjit-b3o1
stress/catch-parameter-syntax.js.dfg-maximal-flush-validate-no-cjit
stress/yield-named-variable.js.no-ftl
jsc-layout-tests.yaml/js/script-tests/let-syntax.js.layout-no-cjit
stress/yield-named-variable-generator.js.no-ftl
stress/yield-named-variable.js.ftl-eager-no-cjit
stress/yield-named-variable.js.ftl-eager
stress/catch-parameter-syntax.js.ftl-eager
stress/catch-parameter-syntax.js.dfg-eager-no-cjit-validate
stress/yield-named-variable-generator.js.ftl-eager-no-cjit-b3o1
stress/yield-named-variable.js.dfg-maximal-flush-validate-no-cjit
stress/yield-named-variable-generator.js.no-cjit-validate-phases
stress/catch-parameter-syntax.js.dfg-eager
stress/yield-named-variable.js.ftl-no-cjit-no-put-stack-validate

[Joseph Pecoraro]

Comment on attachment 307760 [details]
[PATCH] Proposed Fix

Probably more rebaselines needed.

[Joseph Pecoraro]

Created attachment 307878 [details]
[PATCH] Proposed Fix

[Build Bot]

Attachment 307878 [details] did not pass style-queue:


ERROR: JSTests/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
ERROR: JSTests/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
ERROR: JSTests/ChangeLog:14:  Line contains tab character.  [whitespace/tab] [5]
ERROR: JSTests/ChangeLog:12:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: JSTests/ChangeLog:13:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: JSTests/ChangeLog:14:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: JSTests/ChangeLog:20:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 7 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 307878 [details]
[PATCH] Proposed Fix

Attachment 307878 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3582060

New failing tests:
js/dom/reserved-words-as-property.html

[Build Bot]

Created attachment 307884 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307878 [details]
[PATCH] Proposed Fix

Attachment 307878 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3582172

New failing tests:
js/dom/reserved-words-as-property.html

[Build Bot]

Created attachment 307888 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307878 [details]
[PATCH] Proposed Fix

Attachment 307878 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3582141

New failing tests:
js/dom/reserved-words-as-property.html

[Build Bot]

Created attachment 307889 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

Created attachment 307893 [details]
[PATCH] Proposed Fix

[Build Bot]

Attachment 307893 [details] did not pass style-queue:


ERROR: JSTests/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
ERROR: JSTests/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
ERROR: JSTests/ChangeLog:14:  Line contains tab character.  [whitespace/tab] [5]
ERROR: JSTests/ChangeLog:12:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: JSTests/ChangeLog:13:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: JSTests/ChangeLog:14:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: JSTests/ChangeLog:20:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 7 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

What the heck happened here. Commit queue somehow landed this patch but I don't see any record of it getting reviewed... I'm going to roll out and investigate what happened.

[Joseph Pecoraro]

Okay it appears I put what was a new version of a patch intended for this bug on bug 171191. Saam seems to have reviewed the patch on that bug but I'm not sure that was intentional given what the bug was titled and the original patch I had on it.

I'm still going to roll out just to be safe.

[Joseph Pecoraro]

Created attachment 307958 [details]
[PATCH] Proposed Fix

Correctly putting this patch on this bug. This addresses the style bot warnings.

[WebKit Commit Bot]

Comment on attachment 307958 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 307958

Committed r215682: <http://trac.webkit.org/changeset/215682>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.