Skip to content

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

Merged

Conversation

Andarist
Copy link
Contributor

fixes #56474

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Nov 26, 2023
@@ -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) {
Copy link
Contributor Author

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) {
Copy link
Contributor Author

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

&& isInTypeParameterDefault(contextToken)
&& !isInferTypeNode(closestSymbolDeclaration.parent)
) {
const typeParameters = closestSymbolDeclaration.parent.typeParameters;
Copy link
Contributor Author

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

Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@Andarist
Copy link
Contributor Author

@iisaduan would it be possible for you to take a quick look at this?

Copy link
Member

@iisaduan iisaduan left a 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.

Copy link
Member

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

Copy link
Contributor Author

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? :)

Copy link
Member

@iisaduan iisaduan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@iisaduan iisaduan merged commit 53039d3 into microsoft:main Sep 9, 2024
32 checks passed
@sandersn sandersn removed this from PR Backlog Apr 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Latter type parameters are not suggested in constraints of the preceding ones
4 participants