Skip to content
This repository has been archived by the owner. It is now read-only.

Add delete check and fix nested class parsing for private fields #589

Merged
merged 3 commits into from Jun 27, 2017

Conversation

@Qantas94Heavy
Copy link
Member

@Qantas94Heavy Qantas94Heavy commented Jun 23, 2017

Q A
Bug fix? yes
Breaking change? no
New feature? no
Deprecations? no
Spec compliancy? yes
Tests added/pass? yes
Fixed tickets N/A
License MIT

This PR adds a check to make sure the delete operator is not applied to a private field (spec).

This also fixes an issue where private fields fail to be parsed when a method within the class contains a nested class.

@Qantas94Heavy Qantas94Heavy force-pushed the fix-private-fields branch from 8c54227 to 9d5b9db Jun 23, 2017
if (arg.type === "Identifier") {
this.raise(node.start, "Deleting local variable in strict mode");
} else if (this.hasPlugin("classPrivateProperties")) {
if (arg.type === "PrivateName" || arg.property.type === "PrivateName") {
Copy link
Member

@Jessidhia Jessidhia Jun 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the argument always guaranteed to be Identifier, PrivateName or MemberExpression?

If it happens to be neither PrivateName or MemberExpression in this branch it could throw TypeError.

Copy link
Member Author

@Qantas94Heavy Qantas94Heavy Jun 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right -- I'll add a check for MemberExpression.

Given that private fields can only be used within classes, any code
using them must be in a strict mode context. As private fields cannot
be deleted, throw an early SyntaxError.
The parsing of private fields checks whether or not it is within a
class to determine if it is valid or not. However, the state.inClass
property is incorrect as it marks it as outside a class when the inner
class is closed.

This commit fixes this problem by replacing the state.inClass property
with a class nesting counter.
#y = +y;


this.foo = class {
Copy link
Member

@hzoo hzoo Jun 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm already like why would you ever do this 🙂 but ok.

hzoo
hzoo approved these changes Jun 23, 2017
Copy link
Member

@existentialism existentialism left a comment

Nice work!

@hzoo hzoo merged commit 994cde6 into babel:master Jun 27, 2017
3 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants