-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Include all type parameters in completions within type parameters' constraints #56543
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
Include all type parameters in completions within type parameters' constraints #56543
Conversation
@@ -5767,20 +5771,39 @@ function isModuleSpecifierMissingOrEmpty(specifier: ModuleReference | Expression | |||
return !tryCast(isExternalModuleReference(specifier) ? specifier.expression : specifier, isStringLiteralLike)?.text; | |||
} | |||
|
|||
function getVariableOrParameterDeclaration(contextToken: Node | undefined, location: Node) { | |||
function getClosestSymbolDeclaration(contextToken: Node | undefined, location: Node) { |
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.
since Parameter
has a specific meaning and commonly doesn't include TypeParameter
I decided to rename this (as this function might return both of those types)
variableOrParameterDeclaration.parent.typeParameters; | ||
if (symbolDeclarationPos >= variableOrParameterDeclaration.pos && parameters && symbolDeclarationPos < parameters.end) { | ||
return false; | ||
if (closestSymbolDeclaration && symbolDeclaration) { |
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.
The only real change here is the addition of isInTypeParameterDefault
check. I decided to refactor this slightly though as I find handling the parameters
and typeParameters
cases through those separate (although similar) code paths cleaner/more readable
src/services/completions.ts
Outdated
&& isInTypeParameterDefault(contextToken) | ||
&& !isInferTypeNode(closestSymbolDeclaration.parent) | ||
) { | ||
const typeParameters = closestSymbolDeclaration.parent.typeParameters; |
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 believe that those are always available in this codepath - but I kept this as nullable to stay within the safety bounds just in case
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.
👍
@iisaduan would it be possible for you to take a quick look at this? |
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 the reminder! Change generally looks good, but the self-completion is invalid, so we should continue filtering it out.
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.
function test<First extends First, Second>(a: First, b: Second) {}
and type A1<K extends K, L> = K
are both circular definitions and are invalid code, so we should filter the current parameter out. I can't think of a situation where First extends First
is the first part of some valid parameter and not offered by the completion for the valid case
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.
Ok, I addressed this - could you take another look? :)
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.
Looks good, thanks!
fixes #56474