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

[fedeci]

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

If in declare context we can allow a comma after a function rest parameter.

[babel-bot]

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

[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 fd7dbcc:

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

[thorn0]

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

[fedeci]

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

[thorn0]

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

[nicolo-ribaudo]

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).

[nicolo-ribaudo]

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

[thorn0]

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".

[thorn0]

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.

[nicolo-ribaudo]

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.

[thorn0]

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

[sosukesuzuki]

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

[sosukesuzuki]

It is need to add tests to check raising this error.

[fedeci]

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.

[nicolo-ribaudo]

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.

[fedeci]

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

[hzoo]

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

[nicolo-ribaudo]

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.

[hzoo]

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