add class-method-parameter-decorators

[JLHwung]

View Rendered Text

This PR adds Decorators for Class Method and Constructor Parameters support, currently Stage 1.

Currently Babel has two models for the TS decorated parameter:

constructor(readonly @dec param) {}
// AST of the first param
TSParameterProperty {
  readonly: true,
  parameter: Identifier { name: "param" },
  decorators: [ Decorator ]
}

constructor(@dec param) {}
// AST of the first param
Identifier {
  name: "param",
  decorators: [ Decorator ]
}

The second approach, adding decorators to all Pattern AST, seems to be overkill for me because of the concern on memory and precision: the identifier nodes are virtually everywhere, but only a few of them, as function parameters, can be decorated. So I propose to add a new Parameter node as a wrapper around Pattern that also offers decorators storage.

If we agree on the proposed AST structure and the tc39 proposal were adopted without dramatic changes, then Babel has to migrate the current TypeScript AST from Identifier { decorators: [ Decorator ] to Parameter when it becomes a JS-thing.

@bradzacher Since the typescript-estree seems to follow Babel's design here for TSParameterProperty, I would like to know your opinion.

[bradzacher]

Since the typescript-estree seems to follow Babel's design here for TSParameterProperty

Slightly off-topic, but I think it was the other way around - typescript-eslint-parser existed before babel's support and I think babel drew inspiration from the ESTree AST it produced.

---

I'm not sure I follow why defining an optional property on Pattern would be a problem for memory usage?
That's what we do right now for typescript-eslint (Identifier, ArrayPattern, ObjectPattern, AssignmentPattern, RestElement) - the property+array is only defined on the node if and only if there are any decorators.

I agree that turning back time having a Parameter node would have been a great thing to encompass this and any other potential information stored on a function parameter - but sadly that ship has sailed and I don't think introducing one now just for the decorator case is great.

[nzakas]

I like the idea of Parameter. To me, this is an outstanding bug in ESTree that we can actually address. We already add new expected node types into the params array of functions: we started with just Identifier and then kept adding stuff as syntax for parameters changed. I think adding Parameter now also gives us some cover for an additional parameter types that might be introduced in the future.

[nzakas]

I'm not sure we need this restriction. Could we just say that Parameter is always valid in a Function#params array? Maybe we could say if decorators is not empty, then the parent must be a MethodDefinition?

[JLHwung]

Suggested change

	If `params` contains a `Parameter` node, its parent must be a `MethodDefinition` under a `ClassBody` node.
	If `params` contains a `Parameter` node, its parent must be a `MethodDefinition`.

The proposal explicitly disallows decorated parameter in general function expression. Previously I overlooked the spec: I thought MethodDefinition is also shared in ObjectExpression.

The intent of this PR is to use Parameter only when decorators are not empty, otherwise we will have two representations for non-decorated parameters. Though I lean towards using the Parameter for other elements as well.

[nzakas]

Gotcha. I suppose we can always revisit later.

[JLHwung]

Slightly off-topic, but I think it was the other way around - typescript-eslint-parser existed before babel's support and I think babel drew inspiration from the ESTree AST it produced.

Thanks for correcting me. I am not very informed about the history of the babylon parser.

the property+array is only defined on the node if and only if there are any decorators.

AFAIK the ESTree spec does not have optional property. Most properties can be initialized as null or an empty array. I believe it is to make the AST less polymorphic, so that the JS engine can generates less hidden class and yields better performance. Therefore we can't assign decorators to Identifier only when it is decorated, doing so will create a new hidden class and if we add more optional properties in the future, the Identifier will degrade to a Megamorphic object.

For reference here is a discussion thread on Babel about making the identifier node atomic: babel/babel#9545

[bradzacher]

You can definitely have nullable properties in the AST - for example. A return statement may or may not have an argument - so it's argument property is Expression | null.

Couldn't we have the same sort of spec for decorators? "it's null everywhere it's not allowed to have a decorator and it's an array anywhere its allowed"

---

I personally just don't like the idea of updating the AST to wrap the parameter if and only if it has a decorator. It's just creating an inconsistent state that downstream consumers have to handle.

For example - what's to stop a parser from always producing Parameter even if there's no decorator? Another parser might never produce a Parameter.

The state is just inconsistent and that means downstream consumers have to expliclty support both states.

I'd personally prefer just fully breaking change things and going all the way to "estree compatible parsers must always produce a Parameter node".

[nzakas]

I personally just don't like the idea of updating the AST to wrap the parameter if and only if it has a decorator. It's just creating an inconsistent state that downstream consumers have to handle.

I like Parameter a lot better than adding decorators to anything that might be a parameter. We were forced to add typeDefinition to things like Identifier because Facebook had already built that into their parser and refused to change it. I don't want to tread down that same path for decorators, otherwise Identifier and friends will end up with a ton of extra out-of-context information. (I do expect more such stuff in the future.)

I don't find Parameter be inconsistent. I went down this path when evaluating this proposal. If I came up with a solution on my own, it would be a DecoratedParameter node that specifies the decorators and the formal parameter. But that solution presumes that decorated parameters are the only parameter modifications that will ever exist, which I don't think is a good assumption. So, truncating to Parameter as the starting point for parameter modifications makes it more generic while solving the immediate use case.

And as I mentioned elsewhere, we have a long history of adding new node types in the params array, so I don't think this necessarily breaks anything any more than what we've already done. The upside is that once code is updated to respect Parameter, it basically doesn't matter if parsers use Parameter for non-decorated parameters or not. Once you have if (node.type === "Parameter"), that branch needs to handle all of the potential parameter types, so whether or not the decorators array has any elements doesn't really matter.

[nzakas]

LGTM

[JLHwung]

If I came up with a solution on my own, it would be a DecoratedParameter node that specifies the decorators and the formal parameter.

Off-topic: That is exactly my initial thought! Then I changed to Parameter per the same reason you have mentioned.