Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Support auto accessors with TypeScript annotations #15209

Merged
merged 8 commits into from Dec 22, 2022
Merged

fix: Support auto accessors with TypeScript annotations #15209

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0a40615
fix
liuxingbaoyu Nov 18, 2022
5f1d823
add tests
liuxingbaoyu Nov 18, 2022
69726d9
review
liuxingbaoyu Nov 18, 2022
f3fe423
review
liuxingbaoyu Nov 18, 2022
d9764c0
fix babel8 test
liuxingbaoyu Nov 18, 2022
4845f74
review
liuxingbaoyu Nov 22, 2022
1080dd1
review
liuxingbaoyu Dec 16, 2022
68312ff
update tests
liuxingbaoyu Dec 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 15 additions & 0 deletions packages/babel-parser/test/fixtures/typescript/class/accessor-invalid/input.ts
View file
Open in desktop
@@ -0,0 +1,15 @@
abstract class Foo {
declare accessor prop7: number;
JLHwung marked this conversation as resolved.
Show resolved Hide resolved
private accessor #p: any;

accessor a!;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

abstract accessor #s;
accessor #d?;
abstract accessor f = 1;
readonly accessor g;
}

declare class C {
accessor x = 1;
#y = 1;
}
6 changes: 6 additions & 0 deletions packages/babel-parser/test/fixtures/typescript/class/accessor-invalid/options.json
View file
Open in desktop
@@ -0,0 +1,6 @@
{
"plugins": [
"typescript",
"decoratorAutoAccessors"
]
}
220 changes: 220 additions & 0 deletions packages/babel-parser/test/fixtures/typescript/class/accessor-invalid/output.json
View file
Open in desktop
@@ -0,0 +1,220 @@
{
"type": "File",
"start":0,"end":239,"loc":{"start":{"line":1,"column":0,"index":0},"end":{"line":15,"column":1,"index":239}},
"errors": [
"SyntaxError: An 'accessor' property cannot be declared optional. (7:2)"
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that other errors are not reported.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@JLHwung JLHwung Nov 22, 2022 •
edited

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

],
"program": {
"type": "Program",
"start":0,"end":239,"loc":{"start":{"line":1,"column":0,"index":0},"end":{"line":15,"column":1,"index":239}},
"sourceType": "module",
"interpreter": null,
"body": [
{
"type": "ClassDeclaration",
"start":0,"end":190,"loc":{"start":{"line":1,"column":0,"index":0},"end":{"line":10,"column":1,"index":190}},
"abstract": true,
"id": {
"type": "Identifier",
"start":15,"end":18,"loc":{"start":{"line":1,"column":15,"index":15},"end":{"line":1,"column":18,"index":18},"identifierName":"Foo"},
"name": "Foo"
},
"superClass": null,
"body": {
"type": "ClassBody",
"start":19,"end":190,"loc":{"start":{"line":1,"column":19,"index":19},"end":{"line":10,"column":1,"index":190}},
"body": [
{
"type": "ClassAccessorProperty",
"start":23,"end":54,"loc":{"start":{"line":2,"column":2,"index":23},"end":{"line":2,"column":33,"index":54}},
"declare": true,
"static": false,
"key": {
"type": "Identifier",
"start":40,"end":45,"loc":{"start":{"line":2,"column":19,"index":40},"end":{"line":2,"column":24,"index":45},"identifierName":"prop7"},
"name": "prop7"
},
"computed": false,
"typeAnnotation": {
"type": "TSTypeAnnotation",
"start":45,"end":53,"loc":{"start":{"line":2,"column":24,"index":45},"end":{"line":2,"column":32,"index":53}},
"typeAnnotation": {
"type": "TSNumberKeyword",
"start":47,"end":53,"loc":{"start":{"line":2,"column":26,"index":47},"end":{"line":2,"column":32,"index":53}}
}
},
"value": null
},
{
"type": "ClassAccessorProperty",
"start":57,"end":82,"loc":{"start":{"line":3,"column":2,"index":57},"end":{"line":3,"column":27,"index":82}},
"accessibility": "private",
"static": false,
"key": {
"type": "PrivateName",
"start":74,"end":76,"loc":{"start":{"line":3,"column":19,"index":74},"end":{"line":3,"column":21,"index":76}},
"id": {
"type": "Identifier",
"start":75,"end":76,"loc":{"start":{"line":3,"column":20,"index":75},"end":{"line":3,"column":21,"index":76},"identifierName":"p"},
"name": "p"
}
},
"computed": false,
"typeAnnotation": {
"type": "TSTypeAnnotation",
"start":76,"end":81,"loc":{"start":{"line":3,"column":21,"index":76},"end":{"line":3,"column":26,"index":81}},
"typeAnnotation": {
"type": "TSAnyKeyword",
"start":78,"end":81,"loc":{"start":{"line":3,"column":23,"index":78},"end":{"line":3,"column":26,"index":81}}
}
},
"value": null
},
{
"type": "ClassAccessorProperty",
"start":86,"end":98,"loc":{"start":{"line":5,"column":2,"index":86},"end":{"line":5,"column":14,"index":98}},
"static": false,
"key": {
"type": "Identifier",
"start":95,"end":96,"loc":{"start":{"line":5,"column":11,"index":95},"end":{"line":5,"column":12,"index":96},"identifierName":"a"},
"name": "a"
},
"computed": false,
"definite": true,
"value": null
},
{
"type": "ClassAccessorProperty",
"start":101,"end":122,"loc":{"start":{"line":6,"column":2,"index":101},"end":{"line":6,"column":23,"index":122}},
"abstract": true,
"static": false,
"key": {
"type": "PrivateName",
"start":119,"end":121,"loc":{"start":{"line":6,"column":20,"index":119},"end":{"line":6,"column":22,"index":121}},
"id": {
"type": "Identifier",
"start":120,"end":121,"loc":{"start":{"line":6,"column":21,"index":120},"end":{"line":6,"column":22,"index":121},"identifierName":"s"},
"name": "s"
}
},
"computed": false,
"value": null
},
{
"type": "ClassAccessorProperty",
"start":125,"end":138,"loc":{"start":{"line":7,"column":2,"index":125},"end":{"line":7,"column":15,"index":138}},
"static": false,
"key": {
"type": "PrivateName",
"start":134,"end":136,"loc":{"start":{"line":7,"column":11,"index":134},"end":{"line":7,"column":13,"index":136}},
"id": {
"type": "Identifier",
"start":135,"end":136,"loc":{"start":{"line":7,"column":12,"index":135},"end":{"line":7,"column":13,"index":136},"identifierName":"d"},
"name": "d"
}
},
"computed": false,
"optional": true,
"value": null
},
{
"type": "ClassAccessorProperty",
"start":141,"end":165,"loc":{"start":{"line":8,"column":2,"index":141},"end":{"line":8,"column":26,"index":165}},
"abstract": true,
"static": false,
"key": {
"type": "Identifier",
"start":159,"end":160,"loc":{"start":{"line":8,"column":20,"index":159},"end":{"line":8,"column":21,"index":160},"identifierName":"f"},
"name": "f"
},
"computed": false,
"value": {
"type": "NumericLiteral",
"start":163,"end":164,"loc":{"start":{"line":8,"column":24,"index":163},"end":{"line":8,"column":25,"index":164}},
"extra": {
"rawValue": 1,
"raw": "1"
},
"value": 1
}
},
{
"type": "ClassAccessorProperty",
"start":168,"end":188,"loc":{"start":{"line":9,"column":2,"index":168},"end":{"line":9,"column":22,"index":188}},
"readonly": true,
"static": false,
"key": {
"type": "Identifier",
"start":186,"end":187,"loc":{"start":{"line":9,"column":20,"index":186},"end":{"line":9,"column":21,"index":187},"identifierName":"g"},
"name": "g"
},
"computed": false,
"value": null
}
]
}
},
{
"type": "ClassDeclaration",
"start":192,"end":239,"loc":{"start":{"line":12,"column":0,"index":192},"end":{"line":15,"column":1,"index":239}},
"declare": true,
"id": {
"type": "Identifier",
"start":206,"end":207,"loc":{"start":{"line":12,"column":14,"index":206},"end":{"line":12,"column":15,"index":207},"identifierName":"C"},
"name": "C"
},
"superClass": null,
"body": {
"type": "ClassBody",
"start":208,"end":239,"loc":{"start":{"line":12,"column":16,"index":208},"end":{"line":15,"column":1,"index":239}},
"body": [
{
"type": "ClassAccessorProperty",
"start":212,"end":227,"loc":{"start":{"line":13,"column":2,"index":212},"end":{"line":13,"column":17,"index":227}},
"static": false,
"key": {
"type": "Identifier",
"start":221,"end":222,"loc":{"start":{"line":13,"column":11,"index":221},"end":{"line":13,"column":12,"index":222},"identifierName":"x"},
"name": "x"
},
"computed": false,
"value": {
"type": "NumericLiteral",
"start":225,"end":226,"loc":{"start":{"line":13,"column":15,"index":225},"end":{"line":13,"column":16,"index":226}},
"extra": {
"rawValue": 1,
"raw": "1"
},
"value": 1
}
},
{
"type": "ClassPrivateProperty",
"start":230,"end":237,"loc":{"start":{"line":14,"column":2,"index":230},"end":{"line":14,"column":9,"index":237}},
"static": false,
"key": {
"type": "PrivateName",
"start":230,"end":232,"loc":{"start":{"line":14,"column":2,"index":230},"end":{"line":14,"column":4,"index":232}},
"id": {
"type": "Identifier",
"start":231,"end":232,"loc":{"start":{"line":14,"column":3,"index":231},"end":{"line":14,"column":4,"index":232},"identifierName":"y"},
"name": "y"
}
},
"value": {
"type": "NumericLiteral",
"start":235,"end":236,"loc":{"start":{"line":14,"column":7,"index":235},"end":{"line":14,"column":8,"index":236}},
"extra": {
"rawValue": 1,
"raw": "1"
},
"value": 1
}
}
]
}
}
],
"directives": []
}
}
13 changes: 0 additions & 13 deletions packages/babel-parser/test/fixtures/typescript/class/accessor/input.ts
View file
Open in desktop
Expand Up @@ -5,17 +5,4 @@ abstract class Foo {
accessor [prop4]: number = 1;
private accessor prop5: number = 1;
abstract accessor prop6: number;
declare accessor prop7: number;
private accessor #p: any;

accessor a!;
abstract accessor #s;
accessor #d?;
abstract accessor f = 1;
readonly accessor g;
}

declare class C {
accessor x = 1;
#y = 1;
}