1305566 - Various parser compliance nits

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

1) class method named "static" with escapes should be accepted
```
(class { st\u0061tic() {} } )
```
Expected: No SyntaxError
Actual: Throws "SyntaxError: keywords must be written literally, without embedded escapes"


2) Object destructuring pattern with CoverInitializedName not accepted in for-in/of loops
```
for ({a = 0} of []);
```
Expected: No SyntaxError
Actual: "SyntaxError: missing : after property id"


3) Misplaced error marker for invalid CoverInitializedName.
```
js> ({a = 1 + 2 + 3});
typein:1:14 SyntaxError: missing : after property id:
typein:1:14 ({a = 1 + 2 + 3});
typein:1:14 ..............^
```
Expected: Error marker points at equals sign.
Actual: Error marker points at last token of the initializer expression


4) Incorrect yield-as-keyword handling for function expression nested in generator:
```
function*g() { void function yield () {} }
```
Expected: No SyntaxError
Actual: Throws "SyntaxError: yield is a reserved identifier"


5) Incorrect yield-as-name for generator expression nested in non-generator:
```
function f() { void function* yield () {} }
```
Expected: Throws SyntaxError
Actual: No SyntaxError


6) Incorrect yield-as-name for arrow function nested in generator:
```
function*g() { (yield) => {} }
```
Expected: Throws SyntaxError
Actual: No SyntaxError


7) yield expression not allowed in for-in initializer:
```
function*g() { for (var a = yield in {}) ; }
```
Expected: No SyntaxError
Actual: Throws "SyntaxError: expected expression, got keyword 'in'"


8) Dead/unused code:
https://dxr.mozilla.org/mozilla-central/rev/c55bcb7c777ea09431b4d16903ed079ae5632648/js/src/frontend/Parser.cpp#6278-6281
https://dxr.mozilla.org/mozilla-central/rev/c55bcb7c777ea09431b4d16903ed079ae5632648/js/src/frontend/Parser.h#1365-1368


Patches are already finished, just need to split them up and write test cases.

Comment 1

[André Bargull [:anba]]

Attachment: bug1305566-part1.patch

We only need to treat 'static' as a keyword and ensure it contains no escape sequences when we actually have a static class method. Otherwise 'static' with escapes is ok.

Comment 2

[André Bargull [:anba]]

Attachment: bug1305566-part2.patch

validateForInOrOfLHSExpression() needs to clear CoverInitName errors from PossibleError when a valid object destructuring assignment pattern was found.

Comment 3

[André Bargull [:anba]]

Attachment: bug1305566-part3.patch

This patch only moves the error handling code.

Comment 4

[André Bargull [:anba]]

Attachment: bug1305566-part4.patch

I didn't add the TOK_IN case for legacy generators, because they're already deprecated and I didn't want to spend time validating whether or not it's ok to have a legacy generator yield-expression after a for-in initializer.

Comment 5

[André Bargull [:anba]]

Attachment: bug1305566-part5.patch

Ugh, this one (and part 8) are ugly. When |yield| appears in object-destructuring CoverInitName or shorthand-initializer, |yield| is parsed as TOK_NAME instead of TOK_YIELD. labelOrIdentifierReference() didn't allow this case, so I've simply added a boolean "yieldAsName" parameter which acts as an override.

Comment 6

[André Bargull [:anba]]

Attachment: bug1305566-part6.patch

In the spec, ArrowParameters inherit the [Yield] production parameter from the enclosing context. But we didn't get this right. Fixing this involved passing the YieldHandling through the various innerFunction() methods (at least for the most part :-) ).

Comment 7

[André Bargull [:anba]]

Attachment: bug1305566-part7.patch

The optional BindingIdentifier in FunctionExpression/GeneratorExpression doesn't inherit [Yield] handling from the enclosing context. Two changes were necessary here:
- Use the correct YieldHandling in functionExpr()
- And only treat |yield| as a keyword when YieldHandling says it's a keyword. In all other cases (even in generators!), |yield| can be a name.

Comment 8

[André Bargull [:anba]]

Attachment: bug1305566-part8.patch

More ugliness. |yield| and strict reserved words are allowed to have escape sequences in certain cases and are then parsed as names. Strict reserved words are easy to handle,  |yield| requires a bit more work (cf. nextTokenContinuesLetDeclaration(), labelOrIdentifierReference() and bindingIdentifier()). :-(

Comment 9

[André Bargull [:anba]]

Attachment: bug1305566-part9.patch

Yeah, only removal of unused/dead code! :-D

Comment 10

[Tooru Fujisawa [:arai]]

Comment on attachment 8795385 [details] [diff] [review]
bug1305566-part1.patch

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

Thank you :D

::: js/src/frontend/Parser.cpp
@@ +6332,1 @@
>  

Can you remove this extra blank line?

Comment 11

[Tooru Fujisawa [:arai]]

Comment on attachment 8795386 [details] [diff] [review]
bug1305566-part2.patch

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

::: js/src/tests/ecma_6/Statements/for-inof-coverinitname-destr-assign.js
@@ +17,5 @@
> +
> +// for-of statement, object destructuring.
> +var forOf;
> +for ({forOf = defaultValue} of [{}]);
> +assertEq(forOf, defaultValue);

it would be nice to test a case when the object has |forOf| key.

@@ +22,5 @@
> +
> +// for-of statement, array destructuring.
> +forOf = undefined;
> +for ([forOf = defaultValue] of [[]]);
> +assertEq(forOf, defaultValue);

here too, can you add a case when the array has an element?

Comment 12

[Tooru Fujisawa [:arai]]

Comment on attachment 8795389 [details] [diff] [review]
bug1305566-part5.patch

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

can |yieldAsName| name be more descriptive?

Comment 13

[Tooru Fujisawa [:arai]]

Comment on attachment 8795391 [details] [diff] [review]
bug1305566-part6.patch

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

about yield handling in function name/parameter/body, I'm going to fix it as a part of bug 1185106...
give me some time before reviewing this part.

Comment 14

[Tooru Fujisawa [:arai]]

Comment on attachment 8795391 [details] [diff] [review]
bug1305566-part6.patch

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

okay, I think I can rebase on this.
parameters are a bit different but almost same.

Comment 15

[Tooru Fujisawa [:arai]]

Comment on attachment 8795394 [details] [diff] [review]
bug1305566-part9.patch

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

thanks! :)

Comment 16

[André Bargull [:anba]]

Attachment: bug1305566-part1.patch

Update patch 1 per review comments, carrying r+ from arai.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58477b66af97127f02063372492e915ae0816189

Comment 17

[André Bargull [:anba]]

Attachment: bug1305566-part2.patch

Update patch 2 per review comments, carrying r+ from arai.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58477b66af97127f02063372492e915ae0816189

Comment 18

[André Bargull [:anba]]

Attachment: bug1305566-part3.patch

Update patch 3 due to changes in parts 1-2, carrying r+ from arai.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58477b66af97127f02063372492e915ae0816189

Comment 19

[André Bargull [:anba]]

Attachment: bug1305566-part4.patch

Update patch 4 due to changes in parts 1-2, carrying r+ from arai.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58477b66af97127f02063372492e915ae0816189

Comment 20

[André Bargull [:anba]]

Attachment: bug1305566-part5.patch

Update patch 5 per review comments, carrying r+ from arai.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58477b66af97127f02063372492e915ae0816189

Comment 21

[André Bargull [:anba]]

Attachment: bug1305566-part6.patch

Update patch 6 due to changes in parts 1-2 & 5, carrying r+ from arai.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58477b66af97127f02063372492e915ae0816189

Comment 22

[André Bargull [:anba]]

Attachment: bug1305566-part7.patch

Update patch 7 due to changes in parts 1-2 & 5, carrying r+ from arai.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58477b66af97127f02063372492e915ae0816189

Comment 23

[André Bargull [:anba]]

Attachment: bug1305566-part8.patch

Update patch 8 due to changes in parts 1-2 & 5, carrying r+ from arai.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58477b66af97127f02063372492e915ae0816189

Comment 24

[André Bargull [:anba]]

Attachment: bug1305566-part9.patch

Update patch 9 due to changes in parts 1-2 & 5, carrying r+ from arai.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58477b66af97127f02063372492e915ae0816189

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/620687347d7f
Part 1: Allow 'static' with escape sequences as method name. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/48a62f57b3cb
Part 2: Handle CoverInitName for assignment destructuring in for-in/of loop head. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/207337c9f7b6
Part 3: Show error marker for invalid CoverInitName at correct position. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/b407fd6bc948
Part 4: Allow yield without value expression in for-in initializer. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/3deb3fe0e16c
Part 5: Allow yield in object destructuring shorthand and CoverInitName. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/aaf34bc07b55
Part 6: Pass correct yieldHandling to function parameters parser to handle yield in arrow parameters. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b82ab78248c
Part 7: Only treat yield as a keyword when YieldIsKeyword is used. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/1298061af721
Part 8: Allow escape sequences in strict-reserved-words and yield. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdac94ae694f
Part 9: Remove unused/dead code in Parser.{h,cpp}. r=arai

Comment 26

[(Out Sep 17-23) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/620687347d7f
https://hg.mozilla.org/mozilla-central/rev/48a62f57b3cb
https://hg.mozilla.org/mozilla-central/rev/207337c9f7b6
https://hg.mozilla.org/mozilla-central/rev/b407fd6bc948
https://hg.mozilla.org/mozilla-central/rev/3deb3fe0e16c
https://hg.mozilla.org/mozilla-central/rev/aaf34bc07b55
https://hg.mozilla.org/mozilla-central/rev/6b82ab78248c
https://hg.mozilla.org/mozilla-central/rev/1298061af721
https://hg.mozilla.org/mozilla-central/rev/cdac94ae694f

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d430f4a563b
Add testing of names in the destructuring context to the main reserved-word/keyword test.  r=test

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1d430f4a563b