Skip to content
This repository has been archived by the owner on May 19, 2018. It is now read-only.

Modify grammar to support Private Fields proposal: #260

Merged
merged 7 commits into from
May 22, 2017
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions ast/spec.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 {
Copy link
Member

@loganfsmyth loganfsmyth Dec 18, 2016

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.

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.

Copy link
Contributor Author

@diervo diervo Dec 18, 2016

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 +Stage3

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

type: "PrivateName";
name: string;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

The 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
Expand Down Expand Up @@ -1043,6 +1057,16 @@ interface ClassProperty <: Node {
}
```

## ClassPrivateProperty

```js
interface ClassPrivateProperty <: Node {
type: "ClassPrivateProperty";
key: Identifier;
value: Expression;
}
```

## ClassDeclaration

```js
Expand Down
20 changes: 18 additions & 2 deletions src/parser/expression.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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)) {
Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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();

Expand All @@ -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);
Expand Down
2 changes: 2 additions & 0 deletions src/parser/lval.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export default class LValParser extends NodeUtils {
if (node) {
switch (node.type) {
case "Identifier":
case "PrivateName":
case "ObjectPattern":
case "ArrayPattern":
case "AssignmentPattern":
Expand Down Expand Up @@ -227,6 +228,7 @@ export default class LValParser extends NodeUtils {
checkClashes: ?{ [key: string]: boolean },
contextDescription: string): void {
switch (expr.type) {
case "PrivateName":
case "Identifier":
this.checkReservedWord(expr.name, expr.start, false, true);

Expand Down
25 changes: 25 additions & 0 deletions src/parser/statement.js
Original file line number Diff line number Diff line change
Expand Up @@ -650,6 +650,7 @@ export default class StatementParser extends ExpressionParser {
// class bodies are implicitly strict
const oldStrict = this.state.strict;
this.state.strict = true;
this.state.inClass = true;

let hadConstructor = false;
let decorators = [];
Expand Down Expand Up @@ -680,6 +681,13 @@ export default class StatementParser extends ExpressionParser {
decorators = [];
}

if (this.hasPlugin("classPrivateProperties") && this.match(tt.hash)) { // Private property
this.next();
this.parsePropertyName(method);
classBody.body.push(this.parsePrivateClassProperty(method));
continue;
}

method.static = false;
if (this.match(tt.name) && this.state.value === "static") {
const key = this.parseIdentifier(true); // eats 'static'
Expand Down Expand Up @@ -774,9 +782,26 @@ export default class StatementParser extends ExpressionParser {

node.body = this.finishNode(classBody, "ClassBody");

this.state.inClass = false;
this.state.strict = oldStrict;

}

parsePrivateClassProperty(node: N.ClassPrivateProperty): N.ClassPrivateProperty {
this.state.inClassProperty = true;

if (this.match(tt.eq)) {
this.next();
node.value = this.parseMaybeAssign();
} else {
node.value = null;
}
this.semicolon();
this.state.inClassProperty = false;
return this.finishNode(node, "ClassPrivateProperty");
}


parseClassProperty(node: N.ClassProperty): N.ClassProperty {
const hasPlugin = this.hasPlugin("classProperties");
const noPluginMsg = "You can only use Class Properties when the 'classProperties' plugin is enabled.";
Expand Down
7 changes: 7 additions & 0 deletions src/tokenizer/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,13 @@ export default class Tokenizer extends LocationParser {
switch (code) {
// The interpretation of a dot depends on whether it is followed
Copy link
Member

Choose a reason for hiding this comment

The 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();

Expand Down
2 changes: 2 additions & 0 deletions src/tokenizer/state.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export default class State {
this.inAsync =
this.inPropertyName =
this.inType =
this.inClass =
this.inClassProperty =
this.noAnonFunctionType =
false;
Expand Down Expand Up @@ -81,6 +82,7 @@ export default class State {
noAnonFunctionType: boolean;
inPropertyName: boolean;
inClassProperty: boolean;
inClass: boolean;

// Labels in scope.
labels: Array<{ kind: ?("loop" | "switch"), statementStart?: number }>;
Expand Down
1 change: 1 addition & 0 deletions src/tokenizer/types.js
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ export const types: { [name: string]: TokenType } = {
backQuote: new TokenType("`", { startsExpr }),
dollarBraceL: new TokenType("${", { beforeExpr, startsExpr }),
at: new TokenType("@"),
hash: new TokenType("#"),

// Operators. These carry several kinds of properties to help the
// parser use them properly (the presence of these properties is
Expand Down
23 changes: 21 additions & 2 deletions src/types.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,13 @@ export type Identifier = PatternBase & {
__clone(): Identifier;
};

export type PrivateName = PatternBase & {
type: "PrivateName";
name: string;

__clone(): Identifier;
Copy link

@ghost ghost May 22, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's correct

};

// Literals

export type Literal = RegExpLiteral | NullLiteral | StringLiteral | BooleanLiteral | NumericLiteral;
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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
Expand Down
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
}
Loading