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

Add support for class private methods #703

Merged
merged 6 commits into from Sep 6, 2017

Conversation

@Qantas94Heavy
Copy link
Member

Qantas94Heavy commented Aug 29, 2017

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

This adds support for private methods, as described in https://github.com/littledan/proposal-private-methods.

I haven't made an issue for this proposal yet, but if we want it to be separate from #540, I can do so.

  • An update to the #plugins part of the readme. Add a new entry to that list for the new plugin flag (and link to the proposal)
  • If any new nodes or modifications need to be added to the AST, update ast/spec.md
  • Make sure you use the this.hasPlugin("plugin-name-here") check so that your new plugin code only runs when that flag is turned on (not default behavior)
  • Add failing/passing tests according to spec behavior

I'll also need to submit changes to babel-types to match up with this PR.

NOTE: There's still a little bit I'm not exactly sure about (tick means handled):

  • There's some points where this code "eats" the # then throws the error, while in other parts it throws the error without "eating" the #.
  • This might need a few more tests.
  • One of the existing ASI tests seems to fail (fixtures/experimental/class-private-properties/asi-failure-inline) because the error message doesn't include expected ;, but the error is still thrown in the same position in the test file. (edit: changed test to match new message)
@Qantas94Heavy Qantas94Heavy changed the title WIP: Add support for class private methods Add support for class private methods Aug 29, 2017
@hzoo hzoo requested review from littledan and bakkot Aug 29, 2017
Copy link
Member

jridgewell left a comment

Woo!

ast/spec.md Outdated
type: "ClassPrivateMethod";
key: Identifier;
kind: "method" | "get" | "set";
computed: boolean;

This comment has been minimized.

Copy link
@jridgewell

jridgewell Aug 30, 2017

Member

Can't be computed.

ast/spec.md Outdated
kind: "method" | "get" | "set";
computed: boolean;
static: boolean;
decorators: [ Decorator ];

This comment has been minimized.

Copy link
@jridgewell

jridgewell Aug 30, 2017

Member

Also need async and generator flags.

This comment has been minimized.

Copy link
@Qantas94Heavy

Qantas94Heavy Aug 30, 2017

Author Member

Would that be needed for the normal ClassMethod as well?

This comment has been minimized.

Copy link
@jridgewell
ast/spec.md Outdated
@@ -1049,6 +1050,19 @@ interface ClassMethod <: Function {
}
```

## ClassMethod

This comment has been minimized.

Copy link
@jridgewell

jridgewell Aug 30, 2017

Member

Rename to ClassPrivateMethod

parseClassPrivateName(
prop: N.ClassPrivateProperty | N.ClassPrivateMethod,
): N.Expression {
prop.computed = false;

This comment has been minimized.

Copy link
@jridgewell

jridgewell Aug 30, 2017

Member

Let's delete this.

// eaten the #, but in other cases we haven't. How to solve?
this.expectPlugin("classPrivateMethods");
// private "normal" method
method.kind = "method";

This comment has been minimized.

Copy link
@jridgewell

jridgewell Aug 30, 2017

Member

Can we hoist this to the beginning of the this.isClassMethod() if statement?

This comment has been minimized.

Copy link
@Qantas94Heavy

Qantas94Heavy Aug 30, 2017

Author Member

Confused about where exactly you want this part to be moved to.

This comment has been minimized.

Copy link
@jridgewell

jridgewell Aug 30, 2017

Member

Before the if (isPrivate).

const isPrivate = this.eat(tt.hash);

if (isPrivate) {
this.expectOnePlugin(["classPrivateProperties", "classPrivateMethods"]);

This comment has been minimized.

Copy link
@jridgewell

jridgewell Aug 30, 2017

Member

I think we need to do this before eating the hash.

This comment has been minimized.

Copy link
@Qantas94Heavy

Qantas94Heavy Aug 30, 2017

Author Member

Done.

if (this.match(tt.hash)) {
// private getter/setter
this.expectPlugin("classPrivateMethods");
// The private # wouldn't have been eaten as the "async" was in front of it.

This comment has been minimized.

Copy link
@jridgewell

jridgewell Aug 30, 2017

Member

Comment is incorrect.

This comment has been minimized.

Copy link
@Qantas94Heavy

Qantas94Heavy Aug 30, 2017

Author Member

Huh? Maybe my wording was confusing.

What I'm trying to say is that it's now being eaten, but it wasn't detected as being "private" in the code above (let isPrivate = ...). Is that wrong?

This comment has been minimized.

Copy link
@jridgewell

jridgewell Aug 30, 2017

Member

The "async" part is incorrect. We're in the "get"/"set" path here.

this.expectPlugin("classPrivateMethods");
// The private # wouldn't have been eaten as the "async" was in front of it.
this.next();
// The so-called parsed name would have been "async": get the real name.

This comment has been minimized.

Copy link
@jridgewell

jridgewell Aug 30, 2017

Member

Comment again.

false,
/* isConstructor */ false,
);
this.checkGetterSetterParamCount(method);

This comment has been minimized.

Copy link
@jridgewell

jridgewell Aug 30, 2017

Member

I think this should apply to private accessors, too.

ClassMethodOrDeclareMethodCommon & {
type: "ClassPrivateMethod",
key: Identifier,
static: false,

This comment has been minimized.

Copy link
@jridgewell

jridgewell Aug 30, 2017

Member

Should be boolean, right?

This comment has been minimized.

Copy link
@Qantas94Heavy

Qantas94Heavy Aug 30, 2017

Author Member

Does this need static in the first place? If not, how would I handle that in terms of types?

This comment has been minimized.

Copy link
@jridgewell

jridgewell Aug 30, 2017

Member

Oh, it's already in ClassMethodOrDeclareMethodCommon, so we can just drop this line.

} else {
node.value = null;
}
if (node.computed !== undefined) node.computed = undefined;

This comment has been minimized.

Copy link
@jridgewell

jridgewell Aug 30, 2017

Member

What's this for?

This comment has been minimized.

Copy link
@Qantas94Heavy

Qantas94Heavy Aug 31, 2017

Author Member

Yeah, that was a dodgy hack for some test failure because I had set computed elsewhere by accident. I've fixed this now.

@Qantas94Heavy Qantas94Heavy force-pushed the Qantas94Heavy:private-methods branch from c367d80 to d21cbfb Aug 30, 2017
@Qantas94Heavy Qantas94Heavy force-pushed the Qantas94Heavy:private-methods branch from d21cbfb to 6e7175d Aug 30, 2017
Copy link

littledan left a comment

This seems like a correct implementation of the grammar to me!

One surprising thing to me here, which I unfortunately missed in the earlier Private Fields PR, is that there's no single AST node for a private name. Instead, an Identifier is used inside of various different node types--previously, ClassPrivateProperty and PrivateName, now also ClassPrivateMethod. I would've expected that ClassPrivateProperty and ClassPrivateMethod would contain a PrivateName which contains an Identifier, instead of directly containing the Identifier.

I assume that the private methods plugin will be responsible for throwing early errors (such as duplicate method definitions); is that right?

I appreciate the tests here. Although I can think of some more tests for syntax edge cases, maybe those are best developed in test262.


if (isPrivate) {
// TODO: seems a bit inconsistent error throwing: in some cases we've already
// eaten the #, but in other cases we haven't. How to solve?

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Aug 30, 2017

Member

You could make expectPlugin accept an optional pos parameter (like unexpected does) and pass node.start to it.

@@ -1009,21 +1019,43 @@ export default class StatementParser extends ExpressionParser {
return;
}

const isPrivate = this.match(tt.hash);
if (isPrivate) {
this.expectOnePlugin(["classPrivateProperties", "classPrivateMethods"]);

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Aug 30, 2017

Member

Instead of throwing here, you can wait until it is known if the node is a property or a method (to give a more specific error message).

method: N.ClassMethod,
isGenerator: boolean,
isAsync: boolean,
isConstructor: boolean,

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Aug 30, 2017

Member

Can this ever be true?

This comment has been minimized.

Copy link
@bakkot

bakkot Aug 31, 2017

Contributor

No. Not only that, it should be a syntax error; see below.

This comment has been minimized.

Copy link
@Qantas94Heavy

Qantas94Heavy Sep 3, 2017

Author Member

Removed (not sure why it's not saying it's outdated).

Copy link
Contributor

bakkot left a comment

If I'm reading it right, this allows methods and fields named #constructor. (That appears to have been behavior inherited from #609, which was right at the time but changed in tc39/proposal-class-fields@e7d009b.) It shouldn't, for fields or methods. Could you have parseClassPrivateName throw in that case, and add a test or two?

Otherwise looks good to me.

// TODO: if no plugin and is PrivateName, should error be handled here or elsewhere?
if (prop.key.type !== "PrivateName") {
// ClassPrivateProperty is never computed, so we don't assign in that case.
prop.computed = false;

This comment has been minimized.

Copy link
@jridgewell

jridgewell Sep 1, 2017

Member

prop here is a MemberExpression? In that case, it should always be set to false.

This comment has been minimized.

Copy link
@Qantas94Heavy

Qantas94Heavy Sep 2, 2017

Author Member

I think the method signature says it's a Class[Private]Property or Class[Private]Method.

if (isPrivate) {
this.expectPlugin("classPrivateProperties", key.start);
// Private property
classBody.body.push(this.parseClassPrivateProperty(prop));

This comment has been minimized.

Copy link
@jridgewell

jridgewell Sep 1, 2017

Member

Can we make a #pushClassPrivateProperty?

This comment has been minimized.

Copy link
@Qantas94Heavy

Qantas94Heavy Sep 2, 2017

Author Member

Done -- should the two be merged or remain separate?

method,
false,
false,
isConstructor,

This comment has been minimized.

Copy link
@jridgewell

jridgewell Sep 1, 2017

Member

Can't be true.

@Qantas94Heavy Qantas94Heavy force-pushed the Qantas94Heavy:private-methods branch from ce87518 to 5769ae5 Sep 2, 2017
@Qantas94Heavy

This comment has been minimized.

Copy link
Member Author

Qantas94Heavy commented Sep 2, 2017

I've done some updates to the PR -- will squash later once review done.

While I'm at it, I'm just wondering whether the one test failure is important or not.

This roughly equivalent non-private code just throws Unexpected token (2:4):

class A {
  a b;
}

Do we need it to check whether a semicolon should be there, and if so, how would I go about doing that?

@Qantas94Heavy Qantas94Heavy force-pushed the Qantas94Heavy:private-methods branch from 339b4fb to 8645c96 Sep 2, 2017
@bakkot

This comment has been minimized.

Copy link
Contributor

bakkot commented Sep 2, 2017

@Qantas94Heavy, I don't think it's important that the failure says Unexpected token, expected ; vs just Unexpected token, especially given that the latter is what happens with regular properties. I'd just fix the test.

@Qantas94Heavy Qantas94Heavy force-pushed the Qantas94Heavy:private-methods branch 3 times, most recently from d5e0c38 to 8b5c2d5 Sep 4, 2017
@bakkot
bakkot approved these changes Sep 4, 2017
const isSimple = key.type === "Identifier";

// TODO: should these be merged into pushClassProperty?
const handleClassProperty = () => {

This comment has been minimized.

Copy link
@jridgewell

jridgewell Sep 6, 2017

Member

This could just be inlined into the callsites.

Copy link
Member

jridgewell left a comment

Oh, sorry, we need to update https://github.com/babel/babylon/pull/703/files#diff-aaeb808425fc8034ffaa66c89d53cf0aR1082. That's a PrivateName now, right?

@Qantas94Heavy Qantas94Heavy force-pushed the Qantas94Heavy:private-methods branch from 8b5c2d5 to 09c3f50 Sep 6, 2017
This commit adds parser support for the TC39 Stage 2 Private Methods
proposal.

This commit also changes "key" in ClassPrivateProperty from an
Identifier to a PrivateName, as well as disallowing #constructor as a
valid private field name.
These should be treated as regular methods and not special get/set/async
behaviour.
This also removes a test from the Test262 whitelist that failed before
the changes for private methods support and now passes.
@Qantas94Heavy Qantas94Heavy force-pushed the Qantas94Heavy:private-methods branch from 09c3f50 to 62233fb Sep 6, 2017
@Qantas94Heavy

This comment has been minimized.

Copy link
Member Author

Qantas94Heavy commented Sep 6, 2017

@jridgewell right, updated.

@jridgewell jridgewell merged commit 65bea96 into babel:master Sep 6, 2017
2 of 3 checks passed
2 of 3 checks passed
codecov/patch 83.56% of diff hit (target 95.34%)
Details
codecov/project 95.76% (+0.41%) compared to 8cc89e6
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
7 participants
You can’t perform that action at this time.