-
-
Notifications
You must be signed in to change notification settings - Fork 255
Modify grammar to support Private Fields proposal: #260
Changes from 4 commits
5d7b986
5d9688d
8161c4d
0064fa2
7624dbd
3845777
547cde0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ These are the core Babylon AST node types. | |
- [Node objects](#node-objects) | ||
- [Changes](#changes) | ||
- [Identifier](#identifier) | ||
- [PrivateName](#privatename) | ||
- [Literals](#literals) | ||
- [RegExpLiteral](#regexpliteral) | ||
- [NullLiteral](#nullliteral) | ||
|
@@ -90,6 +91,7 @@ These are the core Babylon AST node types. | |
- [ClassBody](#classbody) | ||
- [ClassMethod](#classmethod) | ||
- [ClassProperty](#classproperty) | ||
- [ClassPrivateProperty](#classprivateproperty) | ||
- [ClassDeclaration](#classdeclaration) | ||
- [ClassExpression](#classexpression) | ||
- [MetaProperty](#metaproperty) | ||
|
@@ -166,6 +168,18 @@ interface Identifier <: Expression, Pattern { | |
|
||
An identifier. Note that an identifier may be an expression or a destructuring pattern. | ||
|
||
|
||
# PrivateName | ||
|
||
```js | ||
interface PrivateName <: Expression, Pattern { | ||
type: "PrivateName"; | ||
name: string; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Still feeling like this should be I agree it's not as pretty nesting-wise, but it seems more consistent. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe semantics are different. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I could have been clearer, my suggestion is
I'm not suggesting a
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That I like better. @littledan What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
``` | ||
A Private Name Identifier. | ||
|
||
|
||
# Literals | ||
|
||
```js | ||
|
@@ -1015,7 +1029,7 @@ interface Class <: Node { | |
```js | ||
interface ClassBody <: Node { | ||
type: "ClassBody"; | ||
body: [ ClassMethod | ClassProperty ]; | ||
body: [ ClassMethod | ClassProperty | ClassPrivateProperty ]; | ||
} | ||
``` | ||
|
||
|
@@ -1043,6 +1057,16 @@ interface ClassProperty <: Node { | |
} | ||
``` | ||
|
||
## ClassPrivateProperty | ||
|
||
```js | ||
interface ClassPrivateProperty <: Node { | ||
type: "ClassPrivateProperty"; | ||
key: Identifier; | ||
value: Expression; | ||
} | ||
``` | ||
|
||
## ClassDeclaration | ||
|
||
```js | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -99,7 +99,6 @@ export default class ExpressionParser extends LValParser { | |
parseMaybeAssign(noIn?: ?boolean, refShorthandDefaultPos?: ?Pos, afterLeftParse?: Function, refNeedsArrowPos?: ?Pos): N.Expression { | ||
const startPos = this.state.start; | ||
const startLoc = this.state.startLoc; | ||
|
||
if (this.match(tt._yield) && this.state.inGenerator) { | ||
let left = this.parseYield(); | ||
if (afterLeftParse) left = afterLeftParse.call(this, left, startPos, startLoc); | ||
|
@@ -297,7 +296,7 @@ export default class ExpressionParser extends LValParser { | |
} else if (this.eat(tt.dot)) { | ||
const node = this.startNodeAt(startPos, startLoc); | ||
node.object = base; | ||
node.property = this.parseIdentifier(true); | ||
node.property = this.hasPlugin("classPrivateProperties") ? this.parsePrivateName() : this.parseIdentifier(true); | ||
node.computed = false; | ||
base = this.finishNode(node, "MemberExpression"); | ||
} else if (this.eat(tt.bracketL)) { | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this a leftover There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep will update the PR |
||
} else { | ||
this.unexpected(); | ||
} | ||
|
||
case tt._new: | ||
return this.parseNew(); | ||
|
||
|
@@ -540,6 +546,16 @@ export default class ExpressionParser extends LValParser { | |
} | ||
} | ||
|
||
parsePrivateName(): N.PrivateName | N.Identifier { | ||
const isPrivate = this.eat(tt.hash); | ||
const node = this.parseIdentifier(true); | ||
if (isPrivate) { | ||
// $FlowFixMe | ||
node.type = "PrivateName"; | ||
} | ||
return node; | ||
} | ||
|
||
parseFunctionExpression(): N.FunctionExpression | N.MetaProperty { | ||
const node = this.startNode(); | ||
const meta = this.parseIdentifier(true); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -407,6 +407,13 @@ export default class Tokenizer extends LocationParser { | |
switch (code) { | ||
// The interpretation of a dot depends on whether it is followed | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this comment should go after the new case for 35 ( |
||
// by a digit or another two dots. | ||
|
||
case 35: | ||
if (this.hasPlugin("classPrivateProperties") && this.state.inClass) { | ||
++this.state.pos; return this.finishToken(tt.hash); | ||
} else { | ||
this.raise(this.state.pos, `Unexpected character '${codePointToString(code)}'`); | ||
} | ||
case 46: // '.' | ||
return this.readToken_dot(); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -62,6 +62,13 @@ export type Identifier = PatternBase & { | |
__clone(): Identifier; | ||
}; | ||
|
||
export type PrivateName = PatternBase & { | ||
type: "PrivateName"; | ||
name: string; | ||
|
||
__clone(): Identifier; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This presumably clones to a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's only There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's correct |
||
}; | ||
|
||
// Literals | ||
|
||
export type Literal = RegExpLiteral | NullLiteral | StringLiteral | BooleanLiteral | NumericLiteral; | ||
|
@@ -334,7 +341,7 @@ export type ObjectExpression = NodeBase & { | |
properties: $ReadOnlyArray<ObjectProperty | ObjectMethod | SpreadElement>; | ||
}; | ||
|
||
export type ObjectOrClassMember = ClassMethod | ClassProperty | ObjectMember; | ||
export type ObjectOrClassMember = ClassMethod | ClassProperty | ClassPrivateProperty | ObjectMember; | ||
|
||
export type ObjectMember = ObjectProperty | ObjectMethod; | ||
|
||
|
@@ -552,7 +559,7 @@ export type ClassMemberBase = NodeBase & HasDecorators & { | |
|
||
export type Accessibility = "public" | "protected" | "private"; | ||
|
||
export type ClassMember = ClassMethod | ClassProperty; | ||
export type ClassMember = ClassMethod | ClassProperty | ClassPrivateProperty; | ||
|
||
export type MethodLike = ObjectMethod | FunctionExpression | ClassMethod; | ||
|
||
|
@@ -584,6 +591,18 @@ export type ClassProperty = ClassMemberBase & { | |
readonly?: true; | ||
}; | ||
|
||
export type ClassPrivateProperty = ClassMemberBase & { | ||
type: "ClassPrivateProperty"; | ||
key: Identifier; | ||
value: ?Expression; // TODO: Not in spec that this is nullable. | ||
|
||
typeAnnotation?: ?FlowTypeAnnotation; // TODO: Not in spec | ||
variance?: ?FlowVariance; // TODO: Not in spec | ||
|
||
// TypeScript only: (TODO: Not in spec) | ||
readonly?: true; | ||
}; | ||
|
||
export type OptClassDeclaration = ClassBase & DeclarationBase & HasDecorators & { | ||
type: "ClassDeclaration"; | ||
// TypeScript only | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
class Foo { | ||
#p = x | ||
[#m] () {} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
{ | ||
"throws": "Unexpected token, expected ; (3:10)", | ||
"plugins": [ | ||
"classProperties", | ||
"classPrivateProperties" | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
class Foo { | ||
#p = x | ||
*#m () {} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
{ | ||
"throws": "Unexpected token, expected ; (3:9)", | ||
"plugins": ["classProperties", "classPrivateProperties"] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
class Foo { | ||
#x #y | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
{ | ||
"throws": "Unexpected token, expected ; (2:5)", | ||
"plugins": ["classProperties", "classPrivateProperties"] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
class Foo { | ||
#x | ||
#y | ||
} |
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.
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.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.
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.
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.
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 +Stage3There 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.
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.
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.
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.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.
Ok sure, this make sense.