-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Jsdoc @template in scope as type parameter #16463
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
Conversation
The test to make sure that type parameters are in scope for instantiation previously ignored type parameters created by `@template`. Now it correctly says that they are in scope.
src/compiler/checker.ts
Outdated
@@ -8168,8 +8166,9 @@ namespace ts { | |||
case SyntaxKind.InterfaceDeclaration: | |||
case SyntaxKind.TypeAliasDeclaration: | |||
const declaration = node as DeclarationWithTypeParameters; | |||
if (declaration.typeParameters) { | |||
for (const d of declaration.typeParameters) { | |||
const typeParameters = declaration.typeParameters || getTypeParametersFromJSDocTemplate(declaration); |
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.
@rbuckton has been using a pattern to use of getEffectiveType*
to get the TS nodes, or the ones from JSDoc. we can follow the same here and use getEffectiveTypeParameters
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.
and use that in both places where you need the type parametes
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.
Thanks for pointing out that pattern. Done.
Create getEffectiveTypeParameterDeclarations in utilities.ts
src/compiler/utilities.ts
Outdated
* Gets the effective return type annotation of a signature. If the node was parsed in a | ||
* JavaScript file, gets the return type annotation from JSDoc. | ||
*/ | ||
export function getEffectiveTypeParameterDeclarations(node: DeclarationWithTypeParameters): TypeParameterDeclaration[] { |
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 you see any perf impact of having this function here vs in checker.ts?
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.
Is there normally a perf impact for moving functions to utilities? I didn't expect anything. But I can check to see whether anything shows up.
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.
these are emitted to ts.
instead of the direct function call.
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.
No, there is no perf impact. Monaco gets 0.05s slower, TFS gets 0.04s faster on average over 20 iterations. But neither number is statistically significant.
The code to make sure that type parameters are in scope for instantiation previously ignored type parameters created by
@template
. Now it correctly says that they are in scope.Fixes #15177, which showed that functions returned from a generic function were not instantiated. Turns out this was because the type parameters were not recognised as in scope, so the instantiation algorithm skipped them.