8000 Repo: Investigate better handling of unconstrained generics · Issue #10438 · typescript-eslint/typescript-eslint · GitHub
[go: up one dir, main page]
Skip to content

Repo: Investigate better handling of unconstrained generics #10438

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.

Sign up for GitHub

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

Jump to bottom
Closed
kirkwaiblinger opened this issue Dec 1, 2024 · 1 comment · Fixed by #10496
Closed

Repo: Investigate better handling of unconstrained generics #10438

kirkwaiblinger opened this issue Dec 1, 2024 · 1 comment · Fixed by #10496
Labels
accepting prs Go ahead, send a pull request that resolves this issue locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. repo maintenance things to do with maintenance of the repo, and not with code/docs

Comments

@kirkwaiblinger
Copy link
Member
kirkwaiblinger commented Dec 1, 2024 •
edited
Loading

Suggestion

Sadly, right now getConstrainedTypeOfLocation is bugged for unconstrained generics. The code just gives back the type parameter type if getBaseConstraintOfType(nodeType) is nullish, but that's not correct. Unconstrained generics should behave like unknown. Here's what it is right now:

typescript-eslint/packages/type-utils/src/getConstrainedTypeAtLocation.ts

Lines 7 to 20 in 62d5755

/**
* Resolves the given node's type. Will resolve to the type's generic constraint, if it has one.
*/
export function getConstrainedTypeAtLocation(
services: ParserServicesWithTypeInformation,
node: TSESTree.Node,
): ts.Type {
const nodeType = services.getTypeAtLocation(node);
const constrained = services.program
.getTypeChecker()
.getBaseConstraintOfType(nodeType);
return constrained ?? nodeType;
}

And here's what it "should" be:

/**
 * Resolves the given node's type. Will resolve to the type's generic constraint, if it has one.
 */
export function getConstrainedTypeAtLocation(
  services: ParserServicesWithTypeInformation,
  node: TSESTree.Node,
): ts.Type {
  const nodeType = services.getTypeAtLocation(node);
  if (tsutils.isTypeParameter(nodeType)) {
    const checker = services.program.getTypeChecker();

    return (
      checker.getBaseConstraintOfType(nodeType) ?? checker.getUnknownType()
    );
  }
  return nodeType;
}

Unfortunately, due to microsoft/TypeScript#60475, there is no checker.getUnknownType() yet or for a while.

It kind of sucks, but I propose that we instead modify the implementation to something more like

export function getConstrainedTypeAtLocation(
  services: ParserServicesWithTypeInformation,
  node: TSESTree.Node,
): ts.Type | undefined { // <-- note the `| undefined`
  const nodeType = services.getTypeAtLocation(node);
  if (tsutils.isTypeParameter(nodeType)) {
    const checker = services.program.getTypeChecker();
    return checker.getBaseConstraintOfType(nodeType) 
    /* in the future this can have ?? checker.getUnknownType() */;
  }
  return nodeType;
}

and force callers to handle the unconstrained case separately.

Note - I think we should also double-check (and add tests) for what happens when the constraint is any. I'm pretty sure function f<T extends any>(x: T) {} behaves as though T were unknown rather than any. So maybe we should really be doing

export function getConstrainedTypeAtLocation(
  services: ParserServicesWithTypeInformation,
  node: TSESTree.Node,
): ts.Type | undefined {
  const nodeType = services.getTypeAtLocation(node);
  if (tsutils.isTypeParameter(nodeType)) {
    const checker = services.program.getTypeChecker();
    const constraint = checker.getBaseConstraintOfType(nodeType);
    return constraint == null || isTypeAnyType(constraint)
      ? /* in the future this can be checker.getUnknownType() */ undefined
      : constraint;
  }
  return nodeType;
}

I haven't yet checked what checker.getBaseConstraintOfType() does with an any.

Additional Info

Related, caused #10311
Related, discussion at #10314 (comment)
Possibly related, #10415

cc @ronami
@kirkwaiblinger kirkwaiblinger added triage Waiting for team members to take a look repo maintenance things to do with maintenance of the repo, and not with code/docs labels Dec 1, 2024
@ronami
Copy link
Member
ronami commented Dec 2, 2024 •
edited
Loading

Linking possibly related #10442, #10443 too (still haven't been triaged though).

@kirkwaiblinger kirkwaiblinger mentioned this issue Dec 6, 2024
Merged
3 tasks
@JoshuaKGoldberg JoshuaKGoldberg added accepting prs Go ahead, send a pull request that resolves this issue and removed triage Waiting for team members to take a look labels Dec 7, 2024
@ronami ronami mentioned this issue Dec 7, 2024
Merged
3 tasks
@kirkwaiblinger kirkwaiblinger mentioned this issue Dec 13, 2024
Merged
3 tasks
@kirkwaiblinger kirkwaiblinger mentioned this issue Dec 30, 2024
Open
@github-actions github-actions bot added the locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. label Jan 7, 2025
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 7, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
accepting prs Go ahead, send a pull request that resolves this issue locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. repo maintenance things to do with maintenance of the repo, and not with code/docs
Projects
None yet
Milestone
No milestone
3 participants
@JoshuaKGoldberg @ronami @kirkwaiblinger
0