- Notifications
You must be signed in to change notification settings - Fork 13.2k
Use proper type parameter hosts in JS files#61013
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
base:main
Are you sure you want to change the base?
Use proper type parameter hosts in JS files #61013
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Andarist commented Jan 22, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
jakebailey commented Jan 22, 2025
@typescript-bot test it |
typescript-bot commented Jan 22, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
typescript-bot commented Jan 22, 2025
@jakebailey Here are the results of running the user tests with tsc comparing Something interesting changed - please have a look. Details
|
typescript-bot commented Jan 22, 2025
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
typescript-bot commented Jan 22, 2025
@jakebailey Here they are:tscComparison Report - baseline..pr
System info unknown Hosts
Scenarios
Developer Information: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
typescript-bot commented Jan 22, 2025
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually use the term "host" like this elsewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Andarist commented Jan 22, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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 |
src/compiler/checker.ts Outdated
| function getEffectiveTypeParameterHost(typeParameter: TypeParameter){ | ||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😢
jakebailey commented Jan 23, 2025
The thing I'm most confused about is that it says |
Andarist commented Jan 23, 2025
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
fixes#61090