test: Add tests for duplicate private methods (early-errors) #1769
Conversation
|
Yes, that last sample would be invalid as well. Is there anything about the specification that makes this ambiguous? |
|
On a second look, it is clear in the spec. My bad. I will add a test for that. Thanks for the answer! |
|
Added tests for that. |
|
I think in the contributing docs it says we should not PR the generated tests |
|
You are right. However, #1761 includes them. @leobalter @rwaldron: Should I remove generated tests? |
|
|
||
| //- elements | ||
| get #m() {} | ||
| set #m() {} |
There was a problem hiding this comment.
This is going to produce an unexpected SyntaxError. There is no grammar that allows writing a set accessor method definition without a formal parameter.
MethodDefinition :
set PropertyName ( PropertySetParameterList ) { FunctionBody }
PropertySetParameterList :
FormalParameter
FormalParameter :
BindingElement
BindingElement :
SingleNameBinding
SingleNameBinding :
BindingPattern Initializer opt
BindingIdentifier :
Identifier
Identifier :
IdentifierName but not ReservedWord
| // This code is governed by the BSD license found in the LICENSE file. | ||
|
|
||
| /*--- | ||
| desc: It's a SyntaxError if a class contains a private method and a private static field with the same name |
There was a problem hiding this comment.
I find this surprising and potentially unintended. I'm going to open an issue in the relevant proposals to determine if these semantics are intended.
There was a problem hiding this comment.
There was a problem hiding this comment.
Ok, I've verified that this is intentional here: tc39/proposal-private-methods#46
| // This code is governed by the BSD license found in the LICENSE file. | ||
|
|
||
| /*--- | ||
| desc: It's a SyntaxError if a class contains a private method and a private static method with the same name |
|
|
||
| //- elements | ||
| set #m() {} | ||
| set #m() {} |
There was a problem hiding this comment.
This is additionally invalid for an unintended reason, see previous comment regarding PropertySetParameterList
Yes, generate the tests after the PR is approved. |
2419054 to
090825b
Compare
|
@rwaldron thanks for the review. I removed generated tests and added missing parameters for the setters. |
|
Ok, now that review is complete and approved, please squash the first four commits (you can squash into as many commits as you'd like, but I think 2 makes sense here), then generate the tests and make a commit with just the generated tests. I will merge as soon as I can! |
090825b to
375ae59
Compare
|
@rwaldron done. Thanks again! |
|
Excellent, thanks! |
Adds tests for duplicate private methods according to the test plan posted in #1343.
grammar-privatemeth-duplicate-meth-methgrammar-privatemeth-duplicate-meth-getgrammar-privatemeth-duplicate-meth-setgrammar-privatemeth-duplicate-get-getgrammar-privatemeth-duplicate-set-setgrammar-privatemeth-duplicate-get-setgrammar-privatemeth-duplicate-meth-fieldgrammar-privatemeth-duplicate-get-fieldgrammar-privatemeth-duplicate-set-fieldgrammar-privatemeth-duplicate-meth-staticmethgrammar-privatemeth-duplicate-meth-staticfieldgrammar-privatemeth-duplicate-meth-nestedclassmeth@littledan Could you please have a look and see if I missed anything? One thing I wasn't clear is whether we can have a private getter/setter and a private field with the same name.
I can add a test for this if you tell me what the correct behavior should be.
Tracking Issue: #1343
cc: @jbhoosreddy