Fixed an out-of-order quick info issue with contextual rest parameter #57580
Conversation
| @@ -85,7 +85,7 @@ | |||
| // return curry(getStylingByKeys, 2)(mergedStyling, ...args); | |||
| // ^^^^ | |||
| // | ---------------------------------------------------------------------- | |||
| // | (parameter) args: any | |||
| // | (parameter) args: [] | |||
There was a problem hiding this comment.
this is the correct display that we can see when hovering at this location in the playground (after the playgrounds gets typechecked first): TS playground. If we make an edit and quickly hover over this location we can sometimes "catch" this any being displayed since then the quick info request happens before computing semantic diagnostics and this is exactly what this test is doing.
src/compiler/checker.ts
Outdated
| @@ -49585,7 +49590,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| return grammarErrorOnNode(parameter.name, Diagnostics.A_rest_parameter_cannot_have_an_initializer); | |||
| } | |||
| } | |||
| else if (isOptionalParameter(parameter)) { | |||
| else if (hasEffectiveQuestionToken(parameter)) { | |||
There was a problem hiding this comment.
isOptionalParameter calls getMinArgumentCount for parameters with initializers and that in turn calls getTypeOfSymbol on rest parameters. This led to anyType being returned by reportCircularityError since the circularity was caused by this getTypeOfSymbol called again for something that was just being computed through out-of-order request
I spent some time on this and to the best of my knowledge - this particular function (checkGrammarParameterList) doesn't even have to consult that type. A parameter with an initializer is always treated as required (unless it's last) - and the main purpose of checking this here is to set seenOptionalParameter to potentially error on a required parameter that follows an optional one. This still happens but this error is mainly based on the syntactic~ optionality so it should be OK to just call hasEffectiveQuestionToken here.
I think that avoiding this extra type resolution from a grammar check function should be preferred - but I didn't recheck with all of the other existing grammar checks to see if this is a hard rule today or not.
There was a problem hiding this comment.
checkGrammar* should in general not get types. So I think this is an improvement.
A parameter with an initializer is always treated as required
Shouldn't it be treated as optional by default?
There was a problem hiding this comment.
Shouldn't it be treated as optional by default?
I worded this terribly here. I think I've meant that it's always treated as required when followed by a required parameter:
function fn1(a: string, b = '', c = 0) {} // b is optional
function fn2(a: string, b = '', ...args: any[]) {} // b is optional
function fn3(a: string, b = '', c: number) {} // b is required
| @@ -10518,7 +10517,7 @@ export function canHaveExportModifier(node: Node): node is Extract<HasModifiers, | |||
| } | |||
|
|
|||
| /** @internal */ | |||
| export function isOptionalJSDocPropertyLikeTag(node: Node): node is JSDocPropertyLikeTag { | |||
| export function isOptionalJSDocPropertyLikeTag(node: Node): boolean { | |||
There was a problem hiding this comment.
If I'm not mistaken this function wasn't proving that the input is JSDocPropertyLikeTag - some extra checks on a variable with JSDocPropertyLikeTag type were done here and it could still return false for it. So this wasn't a correct return type annotation
There was a problem hiding this comment.
It's certainly not the first type guard we've had which has lied for convenience's sake. But it doesn't seem like it really helped here.
src/compiler/checker.ts
Outdated
| @@ -49585,7 +49590,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| return grammarErrorOnNode(parameter.name, Diagnostics.A_rest_parameter_cannot_have_an_initializer); | |||
| } | |||
| } | |||
| else if (isOptionalParameter(parameter)) { | |||
| else if (hasEffectiveQuestionToken(parameter)) { | |||
There was a problem hiding this comment.
checkGrammar* should in general not get types. So I think this is an improvement.
A parameter with an initializer is always treated as required
Shouldn't it be treated as optional by default?
|
@typescript-bot perf test this |
|
Starting jobs; this comment will be updated as builds start and complete.
|
sandersn
added
the
For Uncommitted Bug
|
I broke the perf pipeline for PRs accidentally, rerunning @typescript-bot perf test this |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
No description provided.