fix: Support auto accessors with TypeScript annotations

[liuxingbaoyu]

Q	A
Fixed Issues?	Fixes #15205
Patch: Bug Fix?	√
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	√
Documentation PR Link
Any Dependency Changes?
License	MIT

[babel-bot]

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

[JLHwung]

Can you add more test cases? So we can cover cases like static, private, computed, etc.

We also need to check if the auto accessor should work with other TypeScript modifiers, such as abstract, declare, private, etc.

[JLHwung]

Apart from the parser bits, we should also extend the classMemberVisitors

babel/packages/babel-plugin-transform-typescript/src/index.ts

Line 120 in 871fd5e

const classMemberVisitors = {

for ClassAccessorProperty.

[JLHwung]

Good job. Can you add the following test cases?

For transform

class C {
  accessor a;
}

With { allowDeclareFields: false }, we should not remove the accessor field.

class C {
    accessor a!;
}

With { allowDeclareFields: false }, we should not remove the definite accessor field.

For parser

class C {
    private accessor #p: any;
}

Accessibility modifier should not be used with private names.

class C {
    accessor a!;
}

We should support the definite postfix (!) in accessor key.

abstract class C {
    abstract accessor #a;
}

A private accessor should not have abstract modifier.

declare class C {
  accessor x = 1;
}

An ambient accessor cannot have initializer.

abstract class C {
  abstract accessor x = 1;
}

An abstract accessor cannot have initializer.

declare class C {
  #x = 1;
}

An ambient private property cannot have initializer. (This issue is not related to this PR but would be great to included here, too)

class C {
  accessor #x?;
}

An accessor can not be optional. We are currently throwing unexpected token at ?. We can provide a better error here.

class C {
  readonly accessor x;
}

An accessor can not be readonly.

[liuxingbaoyu]

Is accesor a misspelling of accessor?
I've noticed that many error messages currently use this.😕

[JLHwung]

accesor

Yeah, definitely a typo of accessor.

[liuxingbaoyu]

This is kind of annoying, delaying to babel8 means we have to copy and paste a lot of tests, and modify it immediately I'm not sure if it will affect compatibility.

[JLHwung]

We can do it in a separate PR.

We don't have to defer to Babel 8: The error message improvement can be shipped in patch releases, since it is not part of our API protocol. This approach is also similar to how Node.js treats its error message.

[JLHwung]

This is not allowed in an abstract context. Can you update the test case?

[liuxingbaoyu]

Are you saying abstract accessor #s;?

[JLHwung]

Oh actually accessor #d? is not allowed in both abstract class and non-abstract class. It seems to me the test case class/accessor covers only valid cases, can we make it a separate invalid test case?

[JLHwung]

TS will throw Declarations with definite assignment assertions must also have type annotations., we can address that in a separate PR.

[JLHwung]

It seems that other errors are not reported.

[liuxingbaoyu]

Yes, I only reported that unrecoverable error.
Actually initially I also implemented another one, but then I thought they didn't matter.

[JLHwung]

Doesn't the output.json in parser tests only report recoverable errors unless one set errorRecovery: false? The unrecoverable error will be reported at options.throw.

[liuxingbaoyu]

I mean currently the only errors reported are from unrecoverable errors. And the rest are illegal but normal parsing, I don't think it matters, so they haven't been implemented.

[JLHwung]

I mean currently the only errors reported are from unrecoverable errors. And the rest are illegal but normal parsing, I don't think it matters, so they haven't been implemented.

Why is accessor x? an unrecoverable error? Can you elaborate?

[liuxingbaoyu]

An accessor can not be optional. We are currently throwing unexpected token at ?. We can provide a better error here.

https://babeljs.io/repl#?browsers=&build=&builtIns=false&corejs=3.6&spec=false&loose=false&code_lz=MYGwhgzhAEDC0G8BQ1pmMAplA9gJ2gGIAPAfgG4kBfIA&debug=false&forceAllTransforms=false&shippedProposals=false&circleciRepo=&evaluate=false&fileSize=false&timeTravel=false&sourceType=module&lineWrap=true&presets=stage-2&prettier=false&targets=Node-0&version=7.20.4&externalPlugins=&assumptions=%7B%7D

[JLHwung]

And the rest are illegal but normal parsing

Although tsc parser is a loose parser, it does not necessarily mean it should not report errors. The errorRecovery option enables Babel the same tsc functionality: report errors without crashing the parser.

Generally the Babel parser does not report typing errors, such as "the call signature does not match the function interface": they are considered as "runtime errors" because these errors can only be reported from a typing system.

But for those errors that can be detected solely from AST, they are most static errors and thus they should be reported by the parser. We have reported similar errors, e.g. Babel throws "Private elements cannot have an accessibility modifier" for

class C {
  private #Q;
}

Therefore I am not convinced the idea that "the rest are illegal but normal parsing". If TS would ever have a spec, they should be early errors and a parser should report them.

[JLHwung]

An accessor can not be optional. We are currently throwing unexpected token at ?. We can provide a better error here.

My bad, by a better error message, I do mean that we can recover from this error and throw a recoverable error.

[liuxingbaoyu]

I personally prefer to make unrecoverable errors recoverable, and normally parsed ts errors are not actively reported. (at least when no user requests these)
Because I believe all users will use tsc at the same time, and ts makes too many errors recoverable.

My bad, by a better error message, I do mean that we can recover from this error and throw a recoverable error.

I've done this and the only error is from what you said.

[liuxingbaoyu]

Let's release this in the next patch release?

[nicolo-ribaudo]

As @JLHwung mentioned, we generally report errors for TypeScript errors which are more close to syntax errors than to type errors.

However, this PR is good enough to be merged, except that you need to update the error codes/messages.

[nicolo-ribaudo]

We can (and should) fix the error messages now (i.e. BadSetterArity/BetGetterArity), but we should wait until Babel 8 to fix typos in error codes.

[liuxingbaoyu]

Do error codes refer to the property names? These seem to be only visible from the inside.

[nicolo-ribaudo]

Yes they are the property names; and they are public. For example, prettier relies on them: https://github.com/prettier/prettier/blob/b50dfd1974d33abb980e9b4df949913932eeb478/src/language-js/parse/babel.js#L183

[nicolo-ribaudo]

This should be included in the next patch if possible, since other tools are waiting on us.