Modify grammar to support Private Fields proposal: #260

diervo · 2016-12-18T05:06:55Z

First step to support Private Fields
Implemented grammar changes according to the spec:

Adding a new (optional) plugin classPrivateProperties
Adding PrivateName type identifier
Adding ClassPrivateProperty to ClassBody
Allow PrivateName in MemberExpression
Allow PrivateName as a references

Bonus: Minor renaming and small code tweaks towards a future refactor within ClassBody parsing method.

littledan · 2016-12-18T05:34:16Z

src/parser/expression.js

@@ -1014,6 +1022,15 @@ pp.parseIdentifier = function (liberal) {
  return this.finishNode(node, "Identifier");
 };

+pp.parsePrivateName = function (liberal) {
+  const isPrivate = this.eat(tt.hash);
+  const node = this.parseIdentifier(liberal);


Should this be parsePropertyName?

Im following the naming convention for publicProps (fields => props), but I think for this one you are right.
As a side note, we agreed that we will reconcile the naming for a mayor version.

I'm not talking about naming convention, I'm talking about grammar. The grammar for Identifier is more restrictive than IdentifierName/property names, in that it doesn't allow keywords. The grammar for private state specifically allows keywords as the name (just like ordinary properties).

I believe this is covered by passing liberal as true through to parseIdentifier (although it probably should be hard-coded to true here, rather than an arg to parsePrivateName.

parseIdentifier only restricts using keywords when liberal is falsy

littledan · 2016-12-18T05:35:06Z

src/parser/statement.js

+        if (!this.hasPlugin("classPrivateProperties")) {
+          return this.raise(this.state.pos, `Unexpected character '${tt.hash.label}'`);
+        }
+        this.next();


Will this ignore whitespace? In the current spec, whitespace between the # and the IdentifierName is not allowed (though Waldemar proposed that it be allowed).

I will test it and probably I should write a test for it.

loganfsmyth · 2016-12-18T05:39:30Z

ast/spec.md

+# PrivateName
+
+```js
+interface PrivateName <: Expression, Pattern {


I'm curious to hear other's thoughts, but I'd probably have expected this to be a normal Identifier, rather than having a specific subtype for the private node.

The grammar of PrivateNames is pretty different from Identifiers, in terms of where they can be used. It's been suggested that the grammar in the specification be refactored to not call it PrivateName but more like PrivateReference, consisting of the two individual tokens. To me, it makes sense that they are treated differently from identifiers.

Apart from dan's comments about grammar, @loganfsmyth, I though it was cleaner (at least while in stage2) to make additive changes when possible rather than mutating existing nodes, like adding a private attribute for this case. We can recondile in the next mayor version, once public fields, private fields et al, are in +Stage3

Identifier

It is an identifier with a specific type.
It would be confusing for developers because they will be forced to check the type of the identifier.

Most of them will omnit it and will cause some bugs.

PrivateName

More explicit.

I don't have much context here, but I guess most transformation within a class will be with both PrivateNames and Identifier.

My main motivation is consistency. You already always need to check what an identifier is. It could be a local variable, it could be a class property, it could be an object property, it could be an import, it could be imported binding. Identifier is essentially a catch-all for non-keyword values in the original source.

My expectation would be that the ClassPrivateProperty would be the clarifying context for it.

Ok sure, this make sense.

diervo · 2016-12-18T06:24:16Z

Someone needs to help me figure out what to do with the one test failure.

DrewML · 2016-12-18T19:54:19Z

The test failure looks like it is catching something valid-ish.

✖ esprima/invalid syntax/migrated_0065 Error: /home/travis/build/babel/babylon/test/fixtures/esprima/invalid-syntax/migrated_0065/actual.js: Expected error message: Unexpected character '#' (1:2). Got error message: Unexpected token, expected ; (1:2)

For that particular test-case, it seems like the error we're spitting out now is much less descriptive of the problem.

diervo · 2017-01-13T21:32:21Z

I have fixed the test issue due to an less descriptive message in the test that runs id #= 1, but in order to do that I added a check on parseMaybeAssign ( see code changes here beef2ef).

Although that does work, it prevents for example to use the decorators since it will throw because parseMaybeAssign is used inside decorators as well. The best solution I found was to add in parseMaybeAssign another flag to detect when to raise the error, but I don't like it.

I'm not very happy with this change.
Thoughts?

- Adding optional plugin `classPrivateProperties` - Adding PrivateName type identifier - Adding ClassPrivateProperty to ClassBody - Allow PrivateName in MemberExpression - Allow PrivateName as a reference - Adding tests

diervo · 2017-05-15T08:18:56Z

Alright this PR has been dead for way too long, and I promised @littledan I will try to land it before the next TC39 meeting :)

I fixed the couple outstanding test issues and added a bunch of specific tests for private fields.

There are two small details based on the feedback here and the conversation on estree #154:

First is about internal naming convention for ClassProperty and ClassPrivateProperty. I just named the new Node based on the previous naming convention for now. Given than the committee was strongly in favor of unifying both proposals going forward, we can normalize the names when both proposals reach Stage3 (I volunteer to do the work).

The second issue is about wether PrivateName contains in the name field a string or an Identifier (more specifically a namedIdentifier). Again, this is mostly implementation details (although this one matters towards the integration with babel and the transforms). I will defer to @littledan to weight on this one.

IMHO any of the previous points shouldn't be blocking this PR, otherwise we will continue to be stacked without being able to get feedback from developers and push this hopefully forward.

@loganfsmyth @hzoo Can we do another pass at this PR, and unless there are other concerns, merge it?

littledan · 2017-05-15T08:28:38Z

I'm fine with the current code. My only nitpick would be, maybe there doesn't need to be a 'liberal' parameter, as private names are always liberal.

codecov · 2017-05-16T04:06:13Z

Codecov Report

Merging #260 into master will decrease coverage by 0.09%.
The diff coverage is 97.05%.

@@            Coverage Diff            @@
##           master     #260     +/-   ##
=========================================
- Coverage   98.27%   98.18%   -0.1%     
=========================================
  Files          22       22             
  Lines        3530     3572     +42     
  Branches      979      992     +13     
=========================================
+ Hits         3469     3507     +38     
- Misses         22       24      +2     
- Partials       39       41      +2

Flag	Coverage Δ
#babel	`80.34% <26.47%> (-0.71%)`	⬇️
#babylon	`96.94% <97.05%> (-0.17%)`	⬇️

Impacted Files	Coverage Δ
src/tokenizer/types.js	`100% <ø> (ø)`	⬆️
src/tokenizer/state.js	`100% <ø> (ø)`	⬆️
src/parser/lval.js	`98.03% <100%> (+0.02%)`	⬆️
src/tokenizer/index.js	`98.37% <100%> (+0.01%)`	⬆️
src/parser/statement.js	`99.1% <100%> (+0.01%)`	⬆️
src/parser/expression.js	`96.84% <91.66%> (-0.55%)`	⬇️
src/plugins/flow.js	`98.39% <0%> (+0.01%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4ef964e...547cde0. Read the comment docs.

diervo · 2017-05-16T04:18:00Z

@littledan suggestion done!
@hzoo This code coverage thing, doesn't make too much sense to me...
What is remaining to merge this PR?

hzoo · 2017-05-16T04:41:53Z

I think not much if we are ok with the naming conventions - I think we should be ok with making the breaking changes if necessary. You can ignore the coverage issue if it's not making sense

However (I guess this is my responsibility not yours 😄 ) wanted to be clearer about how we do proposals moving forward (meta issue). I thought that by making a "plugin" like flow/jsx do we could have independent versions of proposals.

So if a breaking change is necessary (and it will probably be if we change the AST node names), how can we update the parser without requiring a major version bump? The proposal was to just make another plugin like "classPrivateProperties2" or something etc and then it would be a minor version bump in babylon itself while the transform could do a major version. #275

diervo · 2017-05-16T06:27:41Z

@hzoo Ready to be versioned in the future :)

diervo · 2017-05-17T00:40:25Z

@hzoo Who else I need to bribe to get this in? :) I will keep poking!

hzoo

LGTM other than we aren't sure about the AST node types. I want to get someone else's feedback like @loganfsmyth @danez but I believe they are both busy atm

loganfsmyth · 2017-05-17T22:36:34Z

ast/spec.md

+```js
+interface PrivateName <: Expression, Pattern {
+  type: "PrivateName";
+  name: string;


Still feeling like this should be Identifier, otherwise we're diverging from the semantics that are used throughout the rest of the AST, where literal text blocks without quotes are Identifier nodes.

I agree it's not as pretty nesting-wise, but it seems more consistent.

I believe semantics are different.
It doesn't make sense to have an Identifier that has its private attribute to true, first because its only available in the scope of a ClassBody, seconds because its meaning of it is different on the spec.

I could have been clearer, my suggestion is

interface PrivateName <: Expression, Pattern { type: "PrivateName"; name: Identifier; }

I'm not suggesting a private: boolean flag on Identifier.

Identifier nodes in our spec are currently every case of non-keyword, non-string literals. The same way it's not name: string on FunctionDeclaration names or MemberExpression properties.

That I like better. @littledan What do you think?

Well, thinking of unquoted property names or private field names as Identifiers isn't all that intuitive for me--they are more like strings with a prettier syntax. But if this is just an AST and the rest of the system works this way, it seems fine. I agree that these are somewhat analogous to property names, as long as they are marked specially.

Qantas94Heavy · 2017-05-18T00:21:38Z

src/tokenizer/index.js

@@ -407,6 +407,13 @@ export default class Tokenizer extends LocationParser {
    switch (code) {
      // The interpretation of a dot depends on whether it is followed


I think this comment should go after the new case for 35 ("#").

Qantas94Heavy · 2017-05-18T01:12:01Z

src/parser/expression.js

@@ -518,6 +517,13 @@ export default class ExpressionParser extends LValParser {
        this.takeDecorators(node);
        return this.parseClass(node, false);

+      case tt.hash:
+        if (this.hasPlugin("classPrivateProperties")) {
+          return this.parsePrivateName(true);


Is this a leftover true from the removal of the "liberal" parameter?

Yep will update the PR

diervo · 2017-05-22T00:30:46Z

@loganfsmyth with this we should be good to go! :)

ghost · 2017-05-22T15:59:40Z

src/types.js

+  type: "PrivateName";
+  name: string;
+
+  __clone(): Identifier;


This presumably clones to a PrivateName and not an Identifier.
I also don't think it should be a PatternBase unless it's allowed to appear as a parameter.

I think it's only this.#a, #a and #a = 1 in the class body? At least from the examples.

That's correct

littledan reviewed Dec 18, 2016

View reviewed changes

loganfsmyth reviewed Dec 18, 2016

View reviewed changes

loganfsmyth mentioned this pull request Feb 13, 2017

Private Class Field representation estree/estree#154

Closed

Modify grammar to support Private Fields proposal:

5d7b986

- Adding optional plugin `classPrivateProperties` - Adding PrivateName type identifier - Adding ClassPrivateProperty to ClassBody - Allow PrivateName in MemberExpression - Allow PrivateName as a reference - Adding tests

diervo force-pushed the grammarPrivateFields branch from beef2ef to 5d7b986 Compare May 15, 2017 07:55

hzoo added the spec-new label May 15, 2017

Remove unnecesary liberal parameter

5d9688d

Guarding for plugin dependecy for future versioning

8161c4d

hzoo approved these changes May 17, 2017

View reviewed changes

update spec.md [skip ci]

0064fa2

loganfsmyth reviewed May 17, 2017

View reviewed changes

Qantas94Heavy reviewed May 18, 2017

View reviewed changes

hzoo and others added 3 commits May 19, 2017 16:23

move comment [skip ci]

7624dbd

remove unused param [skip ci]

3845777

Refactor PrivateName to contain Identifier in name property

547cde0

existentialism approved these changes May 22, 2017

View reviewed changes

hzoo merged commit 01da622 into babel:master May 22, 2017

ghost reviewed May 22, 2017

View reviewed changes

ghost mentioned this pull request May 22, 2017

Simplify PrivateName and ClassPrivateProperty types #532

Merged

hzoo mentioned this pull request May 26, 2017

NumberLiteralSeparator: Stage 1 feature plugin. Closes gh-538 #541

Merged

JLHwung mentioned this pull request Jun 5, 2020

add class features estree/estree#222

Merged

		@@ -407,6 +407,13 @@ export default class Tokenizer extends LocationParser {
		switch (code) {
		// The interpretation of a dot depends on whether it is followed

Modify grammar to support Private Fields proposal: #260

Modify grammar to support Private Fields proposal: #260

Conversation

diervo commented Dec 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loganfsmyth Dec 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diervo Dec 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Identifier

PrivateName

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diervo commented Dec 18, 2016

DrewML commented Dec 18, 2016

diervo commented Jan 13, 2017

diervo commented May 15, 2017 • edited Loading

littledan commented May 15, 2017

codecov bot commented May 16, 2017 • edited Loading

Codecov Report

diervo commented May 16, 2017

hzoo commented May 16, 2017 • edited Loading

diervo commented May 16, 2017

diervo commented May 17, 2017 • edited Loading

hzoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diervo commented May 22, 2017

ghost May 22, 2017 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diervo commented Dec 18, 2016 •

edited

Loading

loganfsmyth Dec 18, 2016 •

edited

Loading

diervo Dec 18, 2016 •

edited

Loading

diervo commented May 15, 2017 •

edited

Loading

codecov bot commented May 16, 2017 •

edited

Loading

hzoo commented May 16, 2017 •

edited

Loading

diervo commented May 17, 2017 •

edited

Loading

ghost May 22, 2017 •

edited by ghost

Loading