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

Property variance type annotations for Flow plugin #161

Merged
merged 4 commits into from Oct 14, 2016

Conversation

Projects
None yet
3 participants
@samwgoldman
Copy link
Contributor

samwgoldman commented Oct 7, 2016

Non-method properties and indexers of object types, declare class, and
interfaces can be "positive" or "negative." Class fields, but again not
methods, can also have variance.

This PR generalizes the variance annotations for type parameters into a
new node type, and reuses that node for those properties.

The code for object types is reused for interfaces and declare classes.
The changes there are straightfoward.

The code for class fields is reused for object literals, which do not
support variance annotations (currently). This code is a bit sketchy,
because we always parse variance annotations in the parsePropertyName
extension, then error in a the subsequent parse phase for object
literals (parseObjPropValue) or class methods (parseClassMethod).

Property variance type annotations for Flow plugin
Non-method properties and indexers of object types, declare class, and
interfaces can be "positive" or "negative." Class fields, but again not
methods, can also have variance.

This PR generalizes the variance annotations for type parameters into a
new node type, and reuses that node for those properties.

The code for object types is reused for interfaces and declare classes.
The changes there are straightfoward.

The code for class fields is reused for object literals, which do not
support variance annotations (currently). This code is a bit sketchy,
because we always parse variance annotations in the `parsePropertyName`
extension, then error in a the subsequent parse phase for object
literals (`parseObjPropValue`) or class methods (`parseClassMethod`).

samwgoldman added a commit to samwgoldman/babel that referenced this pull request Oct 8, 2016

Add variance node type and generate property variance annotations
babel/babylon#161 adds parsing support for property variance
annotations. This PR adds the necessary node type for the new Variance
node and generate support for all the positions where variance can now
appear.
@samwgoldman

This comment has been minimized.

Copy link
Contributor Author

samwgoldman commented Oct 8, 2016

Tests fail due to missing babel support, which is added in the referenced PR to Babel.

Is it possible to bump these versions in lock step, or do I need to break up this change into multiple steps?

@danez
Copy link
Member

danez left a comment

Looks good, just two minor nitpicks

} else if (this.state.value === "-") {
variance.kind = "minus";
}
this.eat(tt.plusMin);

This comment has been minimized.

@danez

danez Oct 8, 2016

Member

We could simplify that here and do this.eat(tt.plusMin) already in the if () and remove this line then.

This comment has been minimized.

@samwgoldman

samwgoldman Oct 8, 2016

Author Contributor

That actually slightly changes the behavior here, because calling eat in the conditional expression advances the parser, so when I call startNode the start position is off by one.

I could also write this, but I think don't think it's clearly better:

let variance = this.startNode();
if (this.eat(tt.plusMin)) {
  // ...
} else {
  variance = null;
}
return variance;

This comment has been minimized.

@samwgoldman

samwgoldman Oct 8, 2016

Author Contributor

Will change the eat(plusMin) to next(), which will hopefully clarify the intent better. The tests do exercise the node positioning, so regression is unlikely here.

This comment has been minimized.

@danez

danez Oct 8, 2016

Member

Ah I see, sorry.

@@ -380,6 +379,10 @@ pp.flowParseObjectType = function (allowStatic, allowExact) {
}
if (this.isRelational("<") || this.match(tt.parenL)) {
// This is a method property
if (variance) {
this.unexpected(variance.start);
variance = null;

This comment has been minimized.

@danez

danez Oct 8, 2016

Member

This doesn't makes sense as this.unexpected() throws anyway. Maybe we want to assignment after the diff, although not sure if necessary.

This comment has been minimized.

@samwgoldman

samwgoldman Oct 8, 2016

Author Contributor

Good catch! These two assignments are totally bogus :P

@danez

danez approved these changes Oct 8, 2016

@samwgoldman

This comment has been minimized.

Copy link
Contributor Author

samwgoldman commented Oct 10, 2016

Heads up, I'm going to change this to roll back the addition of a new variance node. Instead, I will continue to use the "plus" | "minus" | null format that currently exists for type parameters.

Don't use a new node type for variance annotations
Adding a new node type, specifically changing the TypeParameter node's
variance property to be node-valued, is a breaking change. We might
choose to make this breaking change in a later version.
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 11, 2016

Current coverage is 94.55% (diff: 97.29%)

No coverage report found for master at 2697bfd.

Powered by Codecov. Last update 2697bfd...a4c9e8f

@samwgoldman

This comment has been minimized.

Copy link
Contributor Author

samwgoldman commented Oct 11, 2016

OK, I removed the new node type, so this is no longer a breaking change. In order to preserve the existing error messages, I needed to pass around the position of the variance annotation out-of-band, which is a bit hacky, because we need to be careful to clean up after ourselves with delete.

In the future, when Babel approaches a major version and we can make the breaking change, I'd like to go back to having a separate node type for variance annotations.

@danez
Copy link
Member

danez left a comment

One minor thing I'm unsure but else looks good now. After we merge this, I can create a new PR with the breaking change for the next major bump.

@@ -1052,9 +1055,10 @@ export default function (instance) {
instance.extend("parseObjPropValue", function (inner) {
return function (prop) {
if (prop.variance) {
this.unexpected(prop.variance.start);
this.unexpected(prop.start);

This comment has been minimized.

@danez

danez Oct 12, 2016

Member

Should this be prop.variancePos?

This comment has been minimized.

@samwgoldman

samwgoldman Oct 12, 2016

Author Contributor

👍 Yup. In this case they are one-in-the-same, but this should definitely be variancePos for clarity. We wouldn't need variancePos except for static properties on classes, which share the same AST building code.

@danez

danez approved these changes Oct 14, 2016

@danez danez merged commit 26809e8 into babel:master Oct 14, 2016

3 checks passed

codecov/patch 97.29% of diff hit (target 94.55%)
Details
codecov/project 94.55% (+0.05%) compared to 4fc0a38
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

hzoo added a commit to babel/babel that referenced this pull request Oct 21, 2016

Add variance node type and generate property variance annotations (#4697
)

* Add variance node type and generate property variance annotations

babel/babylon#161 adds parsing support for property variance
annotations. This PR adds the necessary node type for the new Variance
node and generate support for all the positions where variance can now
appear.

* Variance is no longer a separate node type

This diff also adds tests to class properties and to the
flow-strip-types transform.

* Add test + fix for edge case with variance and class proeprties

kristofdegrave pushed a commit to kristofdegrave/babylon that referenced this pull request Oct 27, 2016

Property variance type annotations for Flow plugin (babel#161)
* Property variance type annotations for Flow plugin

Non-method properties and indexers of object types, declare class, and
interfaces can be "positive" or "negative." Class fields, but again not
methods, can also have variance.

This PR generalizes the variance annotations for type parameters into a
new node type, and reuses that node for those properties.

The code for object types is reused for interfaces and declare classes.
The changes there are straightfoward.

The code for class fields is reused for object literals, which do not
support variance annotations (currently). This code is a bit sketchy,
because we always parse variance annotations in the `parsePropertyName`
extension, then error in a the subsequent parse phase for object
literals (`parseObjPropValue`) or class methods (`parseClassMethod`).

* Remove bogus unreachable code, clarify variance parsing conditional

* Don't use a new node type for variance annotations

Adding a new node type, specifically changing the TypeParameter node's
variance property to be node-valued, is a breaking change. We might
choose to make this breaking change in a later version.

* s/start/variancePos

panagosg7 added a commit to panagosg7/babel that referenced this pull request Jan 17, 2017

Add variance node type and generate property variance annotations (ba…
…bel#4697)

* Add variance node type and generate property variance annotations

babel/babylon#161 adds parsing support for property variance
annotations. This PR adds the necessary node type for the new Variance
node and generate support for all the positions where variance can now
appear.

* Variance is no longer a separate node type

This diff also adds tests to class properties and to the
flow-strip-types transform.

* Add test + fix for edge case with variance and class proeprties
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.