Skip to content

A parameter not declared as a rest parameter is not one #18825

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

Merged
2 commits merged into from
Oct 6, 2017
Merged

A parameter not declared as a rest parameter is not one #18825

2 commits merged into from
Oct 6, 2017

Conversation

ghost
Copy link

@ghost ghost commented Sep 28, 2017

Was looking at this with @sandersn yesterday.
Currently we allow this code:

/** @param {...*} a */
function f(a) {
    return a;
}
f(1, 2).push(3);

We think a is a rest parameter because the JSDoc declares it that way, but that doesn't change the fact that it isn't really a rest parameter.

@ghost ghost requested a review from sandersn September 28, 2017 18:48
@ghost ghost merged commit e821c2b into master Oct 6, 2017
@ghost ghost deleted the isRest branch October 6, 2017 22:05
@mihailik
Copy link
Contributor

mihailik commented Oct 8, 2017

ES3/5 syntax does not allow for dot-dot-dot syntax.

Is there any way to declare rest parameter signature when using TS with ES3/5 JavaScript?

@ghost
Copy link
Author

ghost commented Oct 19, 2017

@mihailik I don't think there is, but we'll give you one for free anyway.

function f() {
    return arguments[1];
}

f(1, 2); // Works, because `arguments` appears in the function body.

@mihailik
Copy link
Contributor

@Andy-MS that looks like best of both worlds, neat!

However I've just checked with typescript@next the example from the above:

node ./node_modules/typescript/lib/tsc.js multi.js --out multiout.js --allowjs

/** @param {...*} a */
function f(a) {
    return a;
}
f(1, 2).push(3);

Compiles with no error. Either I misunderstood this PR, or something is not right?

To be specific: --version gives Version 2.7.0-dev.20171021, running on Mac.

@ghost
Copy link
Author

ghost commented Oct 23, 2017

@mihailik You need --checkJs too or you'll never get errors in .js files.

@mihailik
Copy link
Contributor

@Andy-MS

Thanks, I got the intended checking — no error on 2.5.2, error on typescript@next. But the magic arguments behaviour doesn't seem to work:

node ./node_modules/typescript/lib/tsc.js multi.js --out multiout.js --allowjs --checkjs

/** @param {...*} a */
function f(a) {
    return [arguments[arguments.length-1]];
}
f(1, 2).push(3);
  ~
multi.js(5,3): error TS2345: Argument of type '1' is not assignable to parameter of type 'any[]'.

To be specific: tsc --version returns:
Version 2.7.0-dev.20171024

@ghost
Copy link
Author

ghost commented Oct 24, 2017

That's because it's listening to the jsdoc which declares that a is an array. ...* is treated as the equivalent of any[]. a is the first parameter, not a rest parameter. So it expects the first parameter to be an array.

@mihailik
Copy link
Contributor

mihailik commented Oct 24, 2017

@Andy-MS your original point seems to be correct (same according to Stackoverflow referring to JSDocs spec and Google Closure):

We think a is a rest parameter because the JSDoc declares it that way

Which is opposite to what you're saying right here above:

That's because it's listening to the jsdoc which declares that a is an array

My concern is to make sure idiomatic ES5-style code exposing API with variable parameters is handled the right way. Thanks a lot!

@ghost
Copy link
Author

ghost commented Oct 24, 2017

Well, in the first example I'm describing a bug! If a really is a rest parameter, then the jsdoc should be declared with ... and it makes sense to give it an array type. You might argue that we should give an error if the jsdoc uses ... but the actual variable doesn't.

@mihailik
Copy link
Contributor

@Andy-MS looks like we have another bug introduced here. Here's a bit clearer example from above:

node ./node_modules/typescript/lib/tsc.js multi.js --out multiout.js --allowjs --checkjs

/** @param {...number} a */
function f(a) {
    console.log(arguments[0]);
}
f([1, 2]); // this should fail, but passes
f(1, 2); // this should pass, but fails
  ~
multi.js(6,3): error TS2345: Argument of type '1' is not assignable to parameter of type 'number[]'.

This is a valid idiomatic proper JavaScript: declare a function with variable number arguments. Compiler incorrectly assumes the argument is of type number[].

@ghost
Copy link
Author

ghost commented Oct 24, 2017

Well, like I said above, we could add a diagnostic for if you declare a rest parameter in JSDoc but the corresponding parameter (a) isn't a rest parameter.

@mihailik
Copy link
Contributor

Then it's a regression, unfortunately. That is a valid way to implement rest parameter in ES5.

@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants