Bug 160483 – We don't properly propagate non-simple-parameter-list when parsing a setter

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

for example, we parse this, when we shouldn't:
```
let x = {
   set p(x = 20) { "use strict"; }
}
```

[Saam Barati]

Created attachment 290164 [details]
patch

[Saam Barati]

There might be some test262 tests that now pass. I'll check it out tonight (at airport now).

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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-

[Joseph Pecoraro]

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

[Saam Barati]

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

[Saam Barati]

landed in:
https://trac.webkit.org/changeset/206590