Various parser compliance nits

RESOLVED FIXED in Firefox 52

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: anba, Assigned: anba)

Tracking

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

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(9 attachments, 9 obsolete attachments)

3.43 KB, patch
anba
: review+
Details | Diff | Splinter Review
6.76 KB, patch
anba
: review+
Details | Diff | Splinter Review
4.09 KB, patch
anba
: review+
Details | Diff | Splinter Review
1.85 KB, patch
anba
: review+
Details | Diff | Splinter Review
33.91 KB, patch
anba
: review+
Details | Diff | Splinter Review
20.22 KB, patch
anba
: review+
Details | Diff | Splinter Review
4.21 KB, patch
anba
: review+
Details | Diff | Splinter Review
17.00 KB, patch
anba
: review+
Details | Diff | Splinter Review
3.50 KB, patch
anba
: review+
Details | Diff | Splinter Review
(Assignee)

Description

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

Comment 1

2 years ago
Created attachment 8795385 [details] [diff] [review]
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.
Attachment #8795385 - Flags: review?(arai.unmht)
(Assignee)

Comment 2

2 years ago
Created attachment 8795386 [details] [diff] [review]
bug1305566-part2.patch

validateForInOrOfLHSExpression() needs to clear CoverInitName errors from PossibleError when a valid object destructuring assignment pattern was found.
Attachment #8795386 - Flags: review?(arai.unmht)
(Assignee)

Comment 3

2 years ago
Created attachment 8795387 [details] [diff] [review]
bug1305566-part3.patch

This patch only moves the error handling code.
Attachment #8795387 - Flags: review?(arai.unmht)
(Assignee)

Comment 4

2 years ago
Created attachment 8795388 [details] [diff] [review]
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.
Attachment #8795388 - Flags: review?(arai.unmht)
(Assignee)

Comment 5

2 years ago
Created attachment 8795389 [details] [diff] [review]
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.
Attachment #8795389 - Flags: review?(arai.unmht)
(Assignee)

Comment 6

2 years ago
Created attachment 8795391 [details] [diff] [review]
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 :-) ).
Attachment #8795391 - Flags: review?(arai.unmht)
(Assignee)

Comment 7

2 years ago
Created attachment 8795392 [details] [diff] [review]
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.
Attachment #8795392 - Flags: review?(arai.unmht)
(Assignee)

Comment 8

2 years ago
Created attachment 8795393 [details] [diff] [review]
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()). :-(
Attachment #8795393 - Flags: review?(arai.unmht)
(Assignee)

Comment 9

2 years ago
Created attachment 8795394 [details] [diff] [review]
bug1305566-part9.patch

Yeah, only removal of unused/dead code! :-D
Attachment #8795394 - Flags: review?(arai.unmht)
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?
Attachment #8795385 - Flags: review?(arai.unmht) → review+
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?
Attachment #8795386 - Flags: review?(arai.unmht) → review+

Updated

2 years ago
Attachment #8795387 - Flags: review?(arai.unmht) → review+

Updated

2 years ago
Attachment #8795388 - Flags: review?(arai.unmht) → review+
Comment on attachment 8795389 [details] [diff] [review]
bug1305566-part5.patch

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

can |yieldAsName| name be more descriptive?
Attachment #8795389 - Flags: review?(arai.unmht) → review+
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 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.
Attachment #8795391 - Flags: review?(arai.unmht) → review+

Updated

2 years ago
Attachment #8795392 - Flags: review?(arai.unmht) → review+

Updated

2 years ago
Attachment #8795393 - Flags: review?(arai.unmht) → review+
Comment on attachment 8795394 [details] [diff] [review]
bug1305566-part9.patch

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

thanks! :)
Attachment #8795394 - Flags: review?(arai.unmht) → review+
(Assignee)

Updated

2 years ago
Blocks: 1233767
(Assignee)

Updated

2 years ago
Blocks: 1288507
(Assignee)

Updated

2 years ago
Blocks: 1204024
(Assignee)

Comment 16

2 years ago
Created attachment 8798775 [details] [diff] [review]
bug1305566-part1.patch

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

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58477b66af97127f02063372492e915ae0816189
Attachment #8795385 - Attachment is obsolete: true
Attachment #8798775 - Flags: review+
(Assignee)

Comment 17

2 years ago
Created attachment 8798776 [details] [diff] [review]
bug1305566-part2.patch

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

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58477b66af97127f02063372492e915ae0816189
Attachment #8795386 - Attachment is obsolete: true
Attachment #8798776 - Flags: review+
(Assignee)

Comment 18

2 years ago
Created attachment 8798777 [details] [diff] [review]
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
Attachment #8795387 - Attachment is obsolete: true
Attachment #8798777 - Flags: review+
(Assignee)

Comment 19

2 years ago
Created attachment 8798778 [details] [diff] [review]
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
Attachment #8795388 - Attachment is obsolete: true
Attachment #8798778 - Flags: review+
(Assignee)

Comment 20

2 years ago
Created attachment 8798779 [details] [diff] [review]
bug1305566-part5.patch

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

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58477b66af97127f02063372492e915ae0816189
Attachment #8795389 - Attachment is obsolete: true
Attachment #8798779 - Flags: review+
(Assignee)

Comment 21

2 years ago
Created attachment 8798781 [details] [diff] [review]
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
Attachment #8795391 - Attachment is obsolete: true
Attachment #8798781 - Flags: review+
(Assignee)

Comment 22

2 years ago
Created attachment 8798782 [details] [diff] [review]
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
Attachment #8795392 - Attachment is obsolete: true
Attachment #8798782 - Flags: review+
(Assignee)

Comment 23

2 years ago
Created attachment 8798783 [details] [diff] [review]
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
Attachment #8795393 - Attachment is obsolete: true
Attachment #8798783 - Flags: review+
(Assignee)

Comment 24

2 years ago
Created attachment 8798784 [details] [diff] [review]
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
Attachment #8795394 - Attachment is obsolete: true
Attachment #8798784 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 25

2 years ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed

Comment 27

a year ago
Pushed by jwalden@mit.edu:
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
Duplicate of this bug: 1288507
You need to log in before you can comment on or make changes to this bug.