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(ts): allow trailing comma after rest parameter in TSDeclareFunction #13101

Merged
merged 2 commits into from Apr 6, 2021
Merged

fix(ts): allow trailing comma after rest parameter in TSDeclareFunction #13101

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
264bc6a
fix(ts): allow trailing comma after rest parameter in `TSDeclareFunct…
fedeci Apr 5, 2021
fd7dbcc
Apply code review changes
fedeci Apr 6, 2021
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
14 changes: 14 additions & 0 deletions packages/babel-parser/src/plugins/typescript/index.js
View file
Open in desktop
Expand Up @@ -2779,6 +2779,20 @@ export default (superClass: Class<Parser>): Class<Parser> =>
return super.parseMaybeDecoratorArguments(expr);
}

checkCommaAfterRest(close) {
if (this.match(tt.comma)) {
if (this.lookaheadCharCode() === close) {
if (!this.state.isDeclareContext) {
Copy link
Contributor

@thorn0 thorn0 Apr 5, 2021 •
edited

Choose a reason for hiding this comment

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

Is this.state.isDeclareContext true in d.ts files?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is true if there is the declare keyword before the function declaration.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case this PR doesn't fully fix #13100

Copy link
Member

Choose a reason for hiding this comment

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

We don't have a separate parsing goal for .d.ts files I think, since any code that is valid in .d.ts files was valid in normal .ts files (before we learned about this).

Copy link
Member

Choose a reason for hiding this comment

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

(Q: Do you know if I can use the .d.ts parse goal on the TS playground?)

Copy link
Contributor

Choose a reason for hiding this comment

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

any code that is valid in .d.ts files was valid in normal .ts

class Foo {
    foo(): any;
}

is invalid in .ts unless it's nested in declare namespace.

As for function foo(...args: any[], ) without the body, it can be used for two different things:

  1. for overloads
  2. for type-only declarations (inside declare namespace, declare global, declare module "name" { ... }, and probably something else)

In the first case, the rest parameter trailing comma isn't allowed. In the latter, it's allowed as it's an "ambient context".

Copy link
Contributor

@thorn0 thorn0 Apr 5, 2021 •
edited

Choose a reason for hiding this comment

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

Do you know if I can use the .d.ts parse goal on the TS playground

I believe if you wrap you code e.g. in declare module 'foo' { ... }, it'll be the same as if it were a d.ts file.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can start by fixing the "easy" case (which is what this PR does), but we'll eventually need to add a { dts: boolean } option to the typescript parser plugin.

Copy link
Contributor

Choose a reason for hiding this comment

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

From Prettier's perspective, making the error recoverable would suffice.

Copy link
Member

Choose a reason for hiding this comment

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

I've created a new issue so we don't forget #13108

this.raiseTrailingCommaAfterRest(this.state.start);
return;
fedeci marked this conversation as resolved.
Show resolved Hide resolved
}
this.eat(tt.comma);
} else {
this.raiseRestNotLast(this.state.start);
Copy link
Member

Choose a reason for hiding this comment

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

It is need to add tests to check raising this 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.

This method is inherited from here adding some adjustments:

babel/packages/babel-parser/src/parser/lval.js

Lines 526 to 535 in 7bc72bb

checkCommaAfterRest(close: $Values<typeof charCodes>): void {
if (this.match(tt.comma)) {
if (this.lookaheadCharCode() === close) {
this.raiseTrailingCommaAfterRest(this.state.start);
} else {
this.raiseRestNotLast(this.state.start);
}
}
}

That branch is already tested in "ES" parser, I don't know if it is necessary to repeat those.

Copy link
Member

Choose a reason for hiding this comment

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

Then I'd prefer to do something like this rather than copy-pasting the code:

checkCommaAfterRest(close) {
  if (
    this.state.isDeclareContext &&
    this.match(tt.comma) &&
    this.lookaheadCharCode() === close
  ) {
    this.next(); // tt.comma
  } else {
    super.checkCommaAfterRest(close);
  }
}

by doing so, if we'll make any change to checkCommaAfterRest we don't risk forgetting to also update this second implementation.

Copy link
Member Author

@fedeci fedeci Apr 6, 2021 •
edited

Choose a reason for hiding this comment

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

Don't we risk to call twice lookaheadCharCode by doing so?
edit: I confused it with lookahead()

Copy link
Member

Choose a reason for hiding this comment

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

Just thinking, maybe we can add some parser contributing.md section (or somewhere) mentioning that for plugins (TS/Flow) we would prefer overwritten methods to use super? Not sure if that's worth making a lint thing either though

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if that's worth making a lint thing either though

It's hard to do so, because we need to know which methods shadow methods on the superclass. Maybe it's doable with tslint, but not with eslint.

Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah I was thinking of a simple heuristic of methods that don't start with ts since it seems we do that? Not a big deal

}
}
}

// === === === === === === === === === === === === === === === ===
// Note: All below methods are duplicates of something in flow.js.
// Not sure what the best way to combine these is.
Expand Down
1 change: 1 addition & 0 deletions packages/babel-parser/test/fixtures/typescript/declare/function-rest-trailing-comma/input.ts
View file
Open in desktop
@@ -0,0 +1 @@
declare function foo(...args: any[], )
48 changes: 48 additions & 0 deletions ...es/babel-parser/test/fixtures/typescript/declare/function-rest-trailing-comma/output.json
View file
Open in desktop
@@ -0,0 +1,48 @@
{
"type": "File",
"start":0,"end":38,"loc":{"start":{"line":1,"column":0},"end":{"line":1,"column":38}},
"program": {
"type": "Program",
"start":0,"end":38,"loc":{"start":{"line":1,"column":0},"end":{"line":1,"column":38}},
"sourceType": "module",
"interpreter": null,
"body": [
{
"type": "TSDeclareFunction",
"start":0,"end":38,"loc":{"start":{"line":1,"column":0},"end":{"line":1,"column":38}},
"declare": true,
"id": {
"type": "Identifier",
"start":17,"end":20,"loc":{"start":{"line":1,"column":17},"end":{"line":1,"column":20},"identifierName":"foo"},
"name": "foo"
},
"generator": false,
"async": false,
"params": [
{
"type": "RestElement",
"start":21,"end":35,"loc":{"start":{"line":1,"column":21},"end":{"line":1,"column":35}},
"argument": {
"type": "Identifier",
"start":24,"end":28,"loc":{"start":{"line":1,"column":24},"end":{"line":1,"column":28},"identifierName":"args"},
"name": "args"
},
"typeAnnotation": {
"type": "TSTypeAnnotation",
"start":28,"end":35,"loc":{"start":{"line":1,"column":28},"end":{"line":1,"column":35}},
"typeAnnotation": {
"type": "TSArrayType",
"start":30,"end":35,"loc":{"start":{"line":1,"column":30},"end":{"line":1,"column":35}},
"elementType": {
"type": "TSAnyKeyword",
"start":30,"end":33,"loc":{"start":{"line":1,"column":30},"end":{"line":1,"column":33}}
}
}
}
}
]
}
],
"directives": []
}
}