for example, we parse this, when we shouldn't: ``` let x = { set p(x = 20) { "use strict"; } } ```
Created attachment 290164 [details] patch
There might be some test262 tests that now pass. I'll check it out tonight (at airport now).
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 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 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 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
landed in: https://trac.webkit.org/changeset/206590