Treat ... in jsdoc type as creating a synthetic rest parameter -- not as an array type#19483
Treat ... in jsdoc type as creating a synthetic rest parameter -- not as an array type#194835 commits merged into
... in jsdoc type as creating a synthetic rest parameter -- not as an array type#19483Conversation
…ot as an array type
| !!! error TS8028: JSDoc '...' may only appear in the last parameter of a signature. | ||
| * @param {...number?[]!} k - (number[] | null)[] | ||
| ~~~~~~~~~ | ||
| !!! error TS8028: JSDoc '...' may only appear in the last parameter of a signature. |
There was a problem hiding this comment.
But this is the last parameter of the signature.
There was a problem hiding this comment.
Also, what is the error for repeated ellipses? It used to mean n-dimensional arrays, so it should definitely be an error now.
There was a problem hiding this comment.
I think the error is because this is parsed as (((...number)?)!)[]. I've added a commit that changes the parser so that ... should be at the top level.
| /** @param {...number} a */ | ||
| function f(a) { | ||
| // Ideally this would be a number. But currently checker.ts has only one `argumentsSymbol`. | ||
| arguments[0]; |
There was a problem hiding this comment.
Can you show off the type of a somehow, eg with assignment to another number variable?
| const syntheticArgsSymbol = createSymbol(SymbolFlags.Variable, "args" as __String); | ||
| syntheticArgsSymbol.type = lastParamIsDocumentedAsRest ? createArrayType(getTypeOfParameter(lastParam.symbol)) : anyArrayType; | ||
| syntheticArgsSymbol.isRestParameter = true; | ||
| parameters.push(syntheticArgsSymbol); |
There was a problem hiding this comment.
This will make f(a) have the signature f(a, ...Args)when the intent is just to have f(...Args). That is, the intent of jsdoc {...number} a is to replace a with ...args.
There was a problem hiding this comment.
After thinking about it, the naive fix of replacing the actual symbol with the synthetic one is likely to cause problems with, eg, renaming a. So this code might be good enough. Do you mind checking how the naive fix behaves?
There was a problem hiding this comment.
The real problem is, what will happen if the user tries to access a inside the body? Should we be giving them number | undefined because they declared it as a rest parameter?
d8221fa to
fc470ca
Compare
|
@sandersn Could you review again? |
sandersn
left a comment
There was a problem hiding this comment.
With the new changes, I think we have pretty reasonable behaviour.
I also had a couple of style comments.
| getUnionType([type, undefinedType, nullType]); | ||
| } | ||
|
|
||
| function getUndefinedableType(type: Type): Type { |
There was a problem hiding this comment.
'undefinedable' is usually called 'optional' in our terminology
| syntheticArgsSymbol.type = lastParamIsDocumentedAsRest ? createArrayType(getTypeOfParameter(lastParam.symbol)) : anyArrayType; | ||
| syntheticArgsSymbol.type = lastParamVariadicType ? createArrayType(getTypeFromTypeNode(lastParamVariadicType.type)) : anyArrayType; | ||
| syntheticArgsSymbol.isRestParameter = true; | ||
| if (lastParamVariadicType) { |
There was a problem hiding this comment.
Just thinking aloud: About half of this code has a separate code path for lastParamVariadicType. Would it read better if the normal and last-param-variadic parts were separate?
There was a problem hiding this comment.
Moved this whole part out to its own function.
Fixes #19444
Breaking change if people are using
...types not in a rest parameter position -- those will be errors now.