Bug 160483 - We don't properly propagate non-simple-parameter-list when parsing a setter
Summary: We don't properly propagate non-simple-parameter-list when parsing a setter
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-08-02 18:09 PDT by Saam Barati
Modified: 2016-09-29 11:31 PDT (History)
12 users (show)

See Also:


Attachments
patch (6.93 KB, patch)
2016-09-28 19:44 PDT, Saam Barati
joepeck: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2016-08-02 18:09:27 PDT
for example, we parse this, when we shouldn't:
```
let x = {
   set p(x = 20) { "use strict"; }
}
```
Comment 1 Saam Barati 2016-09-28 19:44:26 PDT
Created attachment 290164 [details]
patch
Comment 2 Saam Barati 2016-09-28 19:56:06 PDT
There might be some test262 tests that now pass. I'll check it out tonight (at airport now).
Comment 3 Joseph Pecoraro 2016-09-28 22:17:55 PDT
Comment on attachment 290164 [details]
patch

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

> Source/JavaScriptCore/parser/Parser.cpp:2003
> +        if (defaultValue || hasDestructuringPattern) {
> +            semanticFailIfTrue(duplicateParameter, "Duplicate parameter '", duplicateParameter->impl(), "' not allowed in function with non-simple parameter list");

Failing these cases with the SyntaxError message "Duplicate parameter" is misleading.

That said, up above in ArrowFunction parsing we will want to handle Duplicate parameters: `(x, x) => 1` should produce a duplicate parameter warning. You may want to fix that separately though.

> LayoutTests/js/parser-syntax-check-expected.txt:1052
> +PASS Invalid: "({set foo(x = 20) { 'use strict'; } })"
> +PASS Invalid: "function f() { ({set foo(x = 20) { 'use strict'; } }) }"
> +PASS Invalid: "({set foo({x}) { 'use strict'; } })"
> +PASS Invalid: "function f() { ({set foo({x}) { 'use strict'; } }) }"
> +PASS Invalid: "({set foo([x]) { 'use strict'; } })"
> +PASS Invalid: "function f() { ({set foo([x]) { 'use strict'; } }) }"
> +PASS Invalid: "({set foo(...x) {} })"

In all of these cases 'x' is not a duplicate. If that is the error message we get we should refine it.
Comment 4 Joseph Pecoraro 2016-09-28 22:19:29 PDT
Comment on attachment 290164 [details]
patch

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

>> Source/JavaScriptCore/parser/Parser.cpp:2003
>> +            semanticFailIfTrue(duplicateParameter, "Duplicate parameter '", duplicateParameter->impl(), "' not allowed in function with non-simple parameter list");
> 
> Failing these cases with the SyntaxError message "Duplicate parameter" is misleading.
> 
> That said, up above in ArrowFunction parsing we will want to handle Duplicate parameters: `(x, x) => 1` should produce a duplicate parameter warning. You may want to fix that separately though.

Err, I clearly misread this. The change here is setting the hasNonSimpleParameterList bit. Clearing r-
Comment 5 Joseph Pecoraro 2016-09-28 23:44:15 PDT
Comment on attachment 290164 [details]
patch

r=me! Seems like these test262 tests (there are generator ones that could also use the kind of patch)
- path: test262/test/language/expressions/object/method-definition/setter-use-strict-with-non-simple-param.js
- path: test262/test/language/expressions/object/method-definition/setter-use-strict-with-non-simple-param.js
Comment 6 Saam Barati 2016-09-29 01:20:56 PDT
Comment on attachment 290164 [details]
patch

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

>>> Source/JavaScriptCore/parser/Parser.cpp:2003
>>> +            semanticFailIfTrue(duplicateParameter, "Duplicate parameter '", duplicateParameter->impl(), "' not allowed in function with non-simple parameter list");
>> 
>> Failing these cases with the SyntaxError message "Duplicate parameter" is misleading.
>> 
>> That said, up above in ArrowFunction parsing we will want to handle Duplicate parameters: `(x, x) => 1` should produce a duplicate parameter warning. You may want to fix that separately though.
> 
> Err, I clearly misread this. The change here is setting the hasNonSimpleParameterList bit. Clearing r-

I'll handle arrow functions in a separate patch.

>> LayoutTests/js/parser-syntax-check-expected.txt:1052
>> +PASS Invalid: "({set foo(...x) {} })"
> 
> In all of these cases 'x' is not a duplicate. If that is the error message we get we should refine it.

Yeah here it will probably say unexpected "..." or something similar. I'll rewrite this test tomorrow to log the error messages so we can prevent regressions, be aware of when we change error messages, and know what the error messages are when we add new tests.

Also, specifically with respect to the rest parameter test, I just added it because it seemed worth adding and I'm not sure if we have parser tests elsewhere that test a rest parameter inside a setter
Comment 7 Saam Barati 2016-09-29 11:07:42 PDT
landed in:
https://trac.webkit.org/changeset/206590