Add the decoratorsAutoAccessors parser plugin

[pzuraq]

Q	A
Fixed Issues?	N/A
Patch: Bug Fix?	No
Major: Breaking Change?	No
Minor: New Feature?	Yes
Tests Added + Pass?	Yes
Documentation PR Link	N/A
Any Dependency Changes?	No
License	MIT

This is the first part of the implementation of the latest version of
the decorators proposal: https://github.com/tc39/proposal-decorators

The keyword is outlined here in the README: https://github.com/tc39/proposal-decorators#class-auto-accessors

It adds a simpleAutoAccessors plugin which can be used to parse the
accessor keyword into ClassAccessorProperty and ClassPrivateAccessorProperty
node types.

[babel-bot]

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/48994/

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 7cdc5a0:

Sandbox	Source
babel-repl-custom-plugin	Configuration
babel-plugin-multi-config	Configuration

[fedeci]

Spec compliance or New feature?

[nicolo-ribaudo]

New feature, it's the first step to implement the new proposal.

[JLHwung]

On the AST changes, can you open a PR to the ESTree project? See also https://github.com/babel/babel/blob/main/CONTRIBUTING.md#creating-a-new-plugin-spec-new for the workflow of creating a new plugin.

[nicolo-ribaudo]

We don't follow ESTree for class fields anyway, right?

[nicolo-ribaudo]

Q: Is decoratorsBeforeExport still an undecided point in the new proposal?

[pzuraq]

We haven't discussed this in a long time, but I believe the status quo is that decorators will come after exports. There are some objectors to this, notably the Angular team, but most other objectors have withdrawn at this point.

[nicolo-ribaudo]

I'm reading tc39/proposal-decorators#426, https://github.com/tc39/proposal-decorators#class-auto-accessors and https://github.com/tc39/proposal-grouped-and-auto-accessors, and I find it slightly annoying that we have to implement this feature under the "decorators" plugin.

It feels like it should go under the plugin for https://github.com/tc39/proposal-grouped-and-auto-accessors. I 100% understand that this feature is needed for the decorators MVP, but it feels like it needs to be there just because the decorators proposal happens to be Stage 2 while https://github.com/tc39/proposal-grouped-and-auto-accessors is Stage 1.

There are a few options, considering a feature where we'll implement https://github.com/tc39/proposal-grouped-and-auto-accessors:

1. accessor x = 1 is enabled both by the decorators and the autoAccessors plugin
2. accessor x = 1 is only enabled by autoAccessors. We now add an incomplete autoAccessors plugin just for what's needed by the decorators proposal, and we'll complete it in the future.
3. accessor x = 1 is enabled by autoAccessors and by a temporary simpleAutoAccessors plugin (which is the intersection between the two proposals). When we'll release autoAccessors, we will documentation-deprecate simpleAutoAccessors.

I slightly prefer 3, but I'd like to wait for the opinion of other team members. The same question applies to the Babel transform plugin.

Also, regardless of what we choose, we should design the AST for accessor x = 1 to be forward-compatible with the other accessors proposal. I think I initially said that the .accessor: boolean property was ok, but now that I'm more aware about the grouped accessors proposal I fear that it won't be compatible with things like accessor x { get } = 1. We might need a new node type:

interface ClassAccessorProperty {
  type: "ClassAccessorProperty",
  key: Expression | PrivateName;
  computed: boolean;
  init?: Expression;

  // These will be added when implementing the other proposal
  get?: Identifier | PrivateName | ClassMethod | ClassPrivateMethod;
  set?: Identifier | PrivateName | ClassMethod | ClassPrivateMethod;
}

[nicolo-ribaudo]

Or maybe it's ok to use .accessor: true, and we'll introduce ClassAccessorProperty only when the accessor has explicitly { ... }.

[pzuraq]

@nicolo-ribaudo that's what I was thinking, since grouped and auto-accessors is currently at stage 1 and there's no guarantee that it will advance, there are a lot of concerns with certain aspects of the proposal. It is a bit annoying though, I would be open to adding a new type of node for this if you prefer to keep a cleaner separation.

I think option 3 makes the most sense as well, I can update the implementation accordingly.

[JLHwung]

We don't follow ESTree for class fields anyway, right?

Yes, the class fields unfortunately differ between Babel and ESTree, but we could coordinate early, for example to add accessor: boolean to both Babel's ClassProperty and ESTree's equivalent PropertyDefinition so we don't end up having more differences, let's say, hasAccessor: boolean in PropertyDefinition but not the other.

The idea is to port early proposals to the ESTree so in the future we don't introduce more discrepancy between two AST. E.g. StaticBlock is identical in Babel and ESTree.

[pzuraq]

Alright, this PR has been updated to add the simpleAutoAccessors plugin that @nicolo-ribaudo outlined. I also added a number of test cases to cover the different combinations of modifiers and options to accessor properties.

@JLHwung I'll make a separate PR to add the types to ESTree as soon as I have time, thanks for pointing that out!

[nicolo-ribaudo]

Some missing tests with newlines:

• class A {
    accessor
    x
  }

• class A {
    static accessor
    x
  }

• class A {
    accessor
    [x]
  }

And a test for invalid swapped static/accessor:

• class A {
    accessor static x
  }

• class A {
    accessor static
    x
  }

[fedeci]

CI errors are also related.

[JLHwung]

@pzuraq I have open an AST PR for ESTree: estree/estree#255 feedbacks are welcome.

[nicolo-ribaudo]

@JLHwung We diverge from ESTree when representing private fields and methods. I think we should also diverge when representing private accessors, using ClassPrivateAccessor, for internal consistency.

[JLHwung]

I tend to merge PrivateClassAccessor into ClassAccessor because unlike class methods/properties, a class accessor can have a public getter and a private setter:

class C { accessor a { get; #set } }

Should it be a PrivateClassAccessor or a PublicClassAccessor? I think people may have different opinions here but we don't have to classify them if we cover them with ClassAccessor.

[nicolo-ribaudo]

Ok, fair. I was thinking about the accessor #x = y case, but it doesn't map well to the extended proposal.

[pzuraq]

Ok, this PR has been updated with the extra test cases you gave @nicolo-ribaudo, and I've fixed the failing tests. Couple notes:

• In keeping with the ESTree spec proposal, I changed init back to value for the accessor nodes, so they can essentially be extensions of class property/class private property. If we still want to have it be init instead, I can change it back.
• For some reason, computed shows up on ClassPrivateAccessorProperty. I can't figure out how to remove it, and it isn't really necessary. Happy to remove if anyone can figure out how to do that.

Other than those issues, I think this PR is good to go!

[JLHwung]

For some reason, computed shows up on ClassPrivateAccessorProperty

Good catch! The computed: false is assigned by parsePropertyName when we parse accessor as a property name. Since then we decide to reinterpret it as an accessor keyword, the computed property gets no chance to be deleted.

Currently we have similar issues on private accessors / async private methods. The AST will contain computed: false even though computed is not in the ClassPrivateMethod interface.

Note that deleting a property from AST node is a perf killer. To fix this issue we can move computed assignment out of parsePropertyName and assign it when we are sure the consumed token is a class modifier / key identifier. This can be in a separate PR so it can get merged in patch releases.

[JLHwung]

Since it is a new AST type, we can use the Babel 8 behaviour directly.

[pzuraq]

How do we do that exactly? Not familiar with the breaking behavior, I copied this from the other definitions

[JLHwung]

Here you can assume process.env.BABEL_8_BREAKING is always true and reduce the conditional expression to its true branch.

[nicolo-ribaudo]

We (@pzuraq, @JLHwung and me) just had a call to discuss about this PR.

• The plugin name should be decoratorsAutoAccessors, to make it clear that it's only meant to be used for the intersection between the "class accessors" and "decorators" proposals.
• Chris will try following the ESTree AST shape, but if it ends up being too complex we will fallback to a simple .accessor: boolean on ClassProperty and ClassPrivateProperty. Regardless of what path we choose, when implementing the full class accessors plugin we can change the AST in a minor since it would be opt-in.

[nicolo-ribaudo]

Thanks!

[JLHwung]

Awesome work. The unresolved comments are nits / edge cases.

[nicolo-ribaudo]

Thanks! Merged to https://github.com/babel/babel/tree/feat-7.16.0/decorators