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.

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

Error on rest parameter with trailing comma #22262

Merged
3 commits merged into from
Mar 29, 2018
Merged

Conversation

ghost
Copy link

@ghost ghost commented Mar 1, 2018

Fixes babel/babel#7460 (comment)

This will likely be annoying for many users, so we should make sure this definitely isn't going to be allowed by any future ES spec.

(MDN ref: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Trailing_commas#Trailing_commas_in_functions)

@ghost ghost requested review from DanielRosenwasser and rbuckton March 1, 2018 15:33
@ajafff
Copy link
Contributor

ajafff commented Mar 1, 2018

TSLint's trailing-comma rule enforces trailing commas even after rest parameters. This used to work since TypeScript allowed it and removed it during transpiling (to a target other than ESNext).

There's an option to forbid trailing commas in that position, but it's not enabled by default. That means this change will break a lot of projects.

@RyanCavanaugh
Copy link
Member

Not to put too fine a point on it, but we're not responsible for projects enforcing that they have a syntax error, even if we mistakenly didn't flag that error

@loganfsmyth
Copy link

loganfsmyth commented Mar 1, 2018

It looks like the same issue is present for array rest and object rest.

var [arg, ...other,] = [1, 2, 3, 4];

var { arg2, ...rest, } = { arg2: 1};

also passes without error.

@ajafff
Copy link
Contributor

ajafff commented Mar 1, 2018

And not only on binding patterns, but also in destructuring assignments:

({...foo,} = {});
[...bar,] = [];

@@ -26093,6 +26093,9 @@ namespace ts {
if (i !== (parameterCount - 1)) {
return grammarErrorOnNode(parameter.dotDotDotToken, Diagnostics.A_rest_parameter_must_be_last_in_a_parameter_list);
}
if (parameters.hasTrailingComma) {
return grammarErrorOnNode(parameter.dotDotDotToken, Diagnostics.A_rest_parameter_may_not_have_a_trailing_comma);
Copy link
Member

@DanielRosenwasser DanielRosenwasser Mar 1, 2018

Choose a reason for hiding this comment

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

I would say that this is the wrong span to error on. Either error on the comma, or the parameter itself.

@ghost ghost force-pushed the rest_param_trailing_comma branch from 3f3b9f3 to f69d5b4 Compare March 1, 2018 18:00
@mhegazy mhegazy added the Breaking Change Would introduce errors in existing code label Mar 1, 2018
@mhegazy mhegazy added this to the TypeScript 2.8 milestone Mar 1, 2018
@ghost
Copy link
Author

ghost commented Mar 1, 2018

Noticed that tslint has a esSpecCompliant setting (palantir/tslint#3176). @adidahiya @ajafff I think this flag should be removed and made the only behavior, and published so that people can fix their code before this PR breaks them.

@@ -26067,11 +26069,9 @@ namespace ts {
return grammarErrorOnNode(asyncModifier, Diagnostics._0_modifier_cannot_be_used_here, "async");
}

function checkGrammarForDisallowedTrailingComma(list: NodeArray<Node>): boolean {
function checkGrammarForDisallowedTrailingComma(list: NodeArray<Node>, diag = Diagnostics.Trailing_comma_not_allowed): boolean {
Copy link
Member

Choose a reason for hiding this comment

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

Where is this optional parameter used?

Copy link
Author

Choose a reason for hiding this comment

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

See the four new calls added in this PR.

@mhegazy
Copy link
Contributor

mhegazy commented Mar 8, 2018

@andy-ms please add a blurb about this change in https://github.com/Microsoft/TypeScript/wiki/Breaking-Changes

@ghost
Copy link
Author

ghost commented Mar 15, 2018

@mhegazy Good to merge now that 2.8 is released?

@mhegazy
Copy link
Contributor

mhegazy commented Mar 29, 2018

👍

@huan
Copy link

huan commented May 6, 2018

@ajafff I run into this issue with TSLint's trailing-comma rule.

There's an option to forbid trailing commas in that position, but it's not enabled by default.

After some search, I did not find the option you mentioned, would be appreciate if you could post it here, thanks!

testerez added a commit to testerez/tslint-config-airbnb that referenced this pull request Jun 6, 2018
Since TS v2.9 trailing comma is not allowed after rest parameters or bindings.
See: microsoft/TypeScript#22262

This PR is just aligning with new TS requirement to avoid conflicts.
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Would introduce errors in existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants