Use better context scope for class constructor implementation signatures in JS files #61013
Conversation
typescript-bot
added
the
For Uncommitted Bug
|
@typescript-bot test it |
|
@jakebailey Here are the results of running the user tests with tsc comparing Something interesting changed - please have a look. Details
|
|
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
| @@ -36138,8 +36141,8 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| if (candidate.typeParameters) { | |||
| // If we are *inside the body of candidate*, we need to create a clone of `candidate` with differing type parameter identities, | |||
| // so our inference results for this call doesn't pollute expression types referencing the outer type parameter! | |||
| const paramLocation = candidate.typeParameters[0].symbol.declarations?.[0]?.parent; | |||
| const candidateParameterContext = paramLocation || (candidate.declaration && isConstructorDeclaration(candidate.declaration) ? candidate.declaration.parent : candidate.declaration); | |||
| const paramHost = getEffectiveTypeParameterHost(candidate.typeParameters[0]); | |||
There was a problem hiding this comment.
Do we actually use the term "host" like this elsewhere?
There was a problem hiding this comment.
I guess "host" might only be appropriate in the JSDoc context but I don't have a better name for this that would cover both TS and JSDoc-based types
There was a problem hiding this comment.
I mean "context" is what the PR title and the code below uses; could technically just inline it too. But, the existing JSDoc func used the term, so it's probably okay.
|
Regarding this reported change. I don't quite know on the spot why this has changed but it looks like a correct change. At the very least the new behavior matches existing TS behavior. See those 2: JS playground and TS playground. It also looks like the original intention of this cast was a non-null-like assertion but accidentally EDIT: this error was reported in TS 5.4 (TS playground) so that just assures me it's the "new" behavior is the correct one :p |
| const tp = getDeclarationOfKind<TypeParameterDeclaration>(typeParameter.symbol, SyntaxKind.TypeParameter)!; | ||
| const host = isJSDocTemplateTag(tp.parent) ? getEffectiveContainerForJSDocTemplateTag(tp.parent) : tp.parent; | ||
| return isJSDocTemplateTag(tp.parent) ? getEffectiveJSDocHost(tp.parent) : tp.parent; |
There was a problem hiding this comment.
This sort of makes me wonder when we really should be using getEffectiveJSDocHost where we are currently using getEffectiveContainerForJSDocTemplateTag...
There was a problem hiding this comment.
Right, but to be fair - I had to use getEffectiveJSDocHost to fix this issue at hand and it didn't look to me like this older function would have to stick to the other one. There is a chance that it should not use this - but there is also a chance that this change fixed some other issue ;p
In the ideal world, I'd go through all of the callers to both and assess their needs but I just don't have time to do that right now 😢
The thing I'm most confused about is that it says |
It fails to infer it completely so it uses the default for an unconstrained type parameter. It happens that in JS files it's |
fixes #60988 , it fixes a regression from #57403 that was not addressed by #58168 , cc @weswigham