New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[parser] Disallow duplicate and undeclared private names #10456
[parser] Disallow duplicate and undeclared private names #10456
Conversation
cec80d4
to
cee6b25
Compare
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/11701/ |
As long as we make it syntax error finally, do we need check duplicated private name in the transformer? babel/packages/babel-helper-create-class-features-plugin/src/index.js Lines 80 to 85 in 45a484f
|
I'm ok with removing it since we don't check other early errors in the transform plugins, but:
|
If we want to report errors on duplicated private names after parsing, ideally it should be done in babel-traverse instead of the plugin since we could not make any assumptions of plugin ordering.
We could bump its peerDependency of |
Well, we can be sure that this plugin will be the last one run on private names, since then they are removed.
This is a breaking change, since npm would suddently start to warn about valid dependencies. I'm not going to die on this hill since I am just slightly in favor of the duplicated error 😛 |
What would happen if a plugin inserts private names after they are removed? Would it still be transpiled? If it is a breaking change to bump peerDependencies, we could target to v8 anyway. Not a big deal. |
It makes sense to move it to I also agree that v8 should be targeted for the change. |
73b3231
to
13dcae6
Compare
I noticed that private names are much more complex to handle since undeclared private names are an early error. I'm mostly rewriting this PR, but all the test changes and the logic to handle accessors will remain the same.
13dcae6
to
8e836dd
Compare
language/expressions/object/method-definition/private-name-early-error-async-gen-inside-class.js(default) | ||
language/expressions/object/method-definition/private-name-early-error-async-gen-inside-class.js(strict mode) | ||
language/expressions/object/method-definition/private-name-early-error-async-gen.js(default) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test (correctly) threw but for the wrong reason. I will open a [good first issue] for it after that this PR is merged.
8818dfd
to
3b8e42c
Compare
f2af0fd
to
013d7fa
Compare
013d7fa
to
1a86603
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍for the code.
WDYT moving classScope to an extra scope handler like what we did in prodParam
? It could get scopeHandler
more focused then, especially private name is only allowed in class body so this.classScope.declarePrivateName
looks straightforward to me.
// they have the same placement (static or not). | ||
redefined = | ||
!(differences & CLASS_ELEMENT_KIND_ACCESSOR) || | ||
differences & CLASS_ELEMENT_FLAG_STATIC; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes my head hurt. Why not split into multiple if-statements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or, just compare them in two separate steps:
// staticBit will be zero if both are static or both are instance.
const staticBit = (accessor & CLASS_ELEMENT_FLAG_STATIC) ^ (elementType & CLASS_ELEMENT_FLAG_STATIC);
// accessorBit will be zero if they are opposite types.
const accessorBit = (accessor & CLASS_ELEMENT_KIND_ACCESSOR) & (elementType & CLASS_ELEMENT_KIND_ACCESSOR);
redefined = !!(staticBit | accessorBit);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-reading it made my head hurt too 😛
I'm rewriting it to this code:
const oldStatic = accessor & CLASS_ELEMENT_FLAG_STATIC;
const newStatic = elementType & CLASS_ELEMENT_FLAG_STATIC;
const oldKind = accessor & CLASS_ELEMENT_KIND_ACCESSOR;
const newKind = elementType & CLASS_ELEMENT_KIND_ACCESSOR;
// The private name can be duplicated only if it is used by
// two accessors with different kind (get and set), and if
// they have the same placement (static or not).
redefined = oldKind === newKind || oldStatic !== newStatic;
- Disallow duplicate private names - Disallow undeclared private names
Note: the tests inside
class-private-names-duplicated
are automatically generated. They are (instance|static
*field|method|get|set
)^2This PR reduces the Test262 whitelist from 194 entries to 18 🎉 We are now 99.97% compliant.