fix(eslint-plugin): add getConstraintInfo to handle generic constraints better #10496

kirkwaiblinger · 2024-12-13T23:52:39Z

PR Checklist

Addresses an existing open issue
- fixes Repo: Investigate better handling of unconstrained generics #10438
- ~~fixes (not anymore) Bug: [no-unnecessary-boolean-literal-compare] doesn't flag values of a type parameter with boolean type constraint #10443~~ EDIT - this was already solved in the meantime, just refactoring
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

Since removing or modifying getConstrainedTypeAtLocation would be a breaking change (see https://typescript-eslint.io/users/versioning#parser-typescript-estree-scope-manager-types-type-utils-utils), ~~I've deprecated it and added a new utility in its place~~ I've left it in place for now, and just added a new utility. Since we may change our implementation in the future, I've made the new utility internal, and not deprecated the existing getConstrainedTypeAtLocation. Deprecating it and replacing internal usages can be handled in followup work. See #10496 (comment)

I've redone the testing so that the tests for getConstrainedTypeAtLocation actually test something useful, and tested several edge cases for both the old and new APIs.

I've used the new API for a few rules for good measure. We'll want to gradually replace the rest of our usages in followup work.

Retouched the fix in #10314 ~~and used the new strategy to solve #10443~~ and in #10474

TODO - I'll have to find a way to manage the deprecation linting failures 🙁 #9899 would be extremely clutch. Not really sure how to handle this otherwise, other than to alias the function with a non-deprecated version to replace the existing usages, with the intention of cleaning up the alias later. Not deprecating anything yet.

~~UPDATE - I chose to alias getConstrainedTypeAtLocation as the ironically not-deprecated DEPRECATED_getConstrainedTypeAtLocation in order to allow existing usages not to fail CI.~~

typescript-eslint · 2024-12-13T23:52:42Z

Thanks for the PR, @kirkwaiblinger!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

netlify · 2024-12-13T23:53:00Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`4c9da08`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/6770924ce4d8050008c05021
😎 Deploy Preview	https://deploy-preview-10496--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 75 (🟢 up 2 from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 98 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

nx-cloud · 2024-12-13T23:53:32Z

View your CI Pipeline Execution ↗ for commit 4c9da08.

Command	Status	Duration	Result
`nx run-many --target=build --exclude website --...`	✅ Succeeded	3s	View ↗
`nx run-many --target=clean`	✅ Succeeded	10s	View ↗

☁️ Nx Cloud last updated this comment at 2024-12-29 08:16:41 UTC

packages/type-utils/tests/getConstrainedTypeAtLocation.test.ts

codecov · 2024-12-14T07:33:39Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.87%. Comparing base (3ae4148) to head (4c9da08).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #10496   +/-   ##
=======================================
  Coverage   86.86%   86.87%           
=======================================
  Files         445      446    +1     
  Lines       15455    15465   +10     
  Branches     4507     4508    +1     
=======================================
+ Hits        13425    13435   +10     
  Misses       1675     1675           
  Partials      355      355

Flag	Coverage Δ
unittest	`86.87% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
packages/eslint-plugin/src/rules/await-thenable.ts	`100.00% <100.00%> (ø)`
...rc/rules/no-unnecessary-boolean-literal-compare.ts	`97.56% <100.00%> (+0.09%)`	⬆️
...slint-plugin/src/rules/no-unnecessary-condition.ts	`99.25% <100.00%> (-0.01%)`	⬇️
...ckages/eslint-plugin/src/util/getConstraintInfo.ts	`100.00% <100.00%> (ø)`
...ackages/eslint-plugin/src/util/needsToBeAwaited.ts	`100.00% <100.00%> (ø)`
...ges/type-utils/src/getConstrainedTypeAtLocation.ts	`100.00% <ø> (ø)`

JoshuaKGoldberg

LGTM, nice to see the cleanup & related fixes!

Just requesting a collection of touchups, nothing major.

packages/type-utils/src/getConstrainedTypeAtLocation.ts

packages/type-utils/package.json

packages/type-utils/src/getConstrainedTypeAtLocation.ts

Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>

JoshuaKGoldberg

I like this strategy 😄

kirkwaiblinger · 2024-12-17T22:44:55Z

packages/type-utils/src/getConstrainedTypeAtLocation.ts

+ *
+ * Successor to {@link getConstrainedTypeAtLocation} due to https://github.com/typescript-eslint/typescript-eslint/issues/10438
+ */
+export function getConstraintTypeInfoAtLocation(


@JoshuaKGoldberg I've just had the thought - I think this might be the wrong API to write. It might be better to just accept a type as input, like

export declare function getConstraintInfo(type: ts.Type): { isTypeParameter: boolean, constraint: ts.Type | undefined };

This maintains a separation between getting types and analyzing types.

@kirkwaiblinger just confirming, are you planning on making changes? And/or, is this a question?

Lol. In my head, I was asking you for your opinion, yeah, but I forgot to write that out 😆

Coming back to this, I currently lean towards: let's go with this PR in its current state, and if there's a need to extract just the constraint info part of this function that's quite easy to do in a followup when the need arises.

Feel free to offer your opinion! 😁

If you're on board with this plan, then this PR is up to date and ready to be merged 👍

Oh awesome! I don't have a strong preference here.

Proposal: whatever strategy you want to go with, how about marking the final creation as @internal so it's not part of the public API? That way we can play with it as we want.

Made lots of changes in this arena. New utility is just internal to eslint-plugin for now, no deprecation yet. Should be much easier to review!

I like this strategy. The union of possible constraint info points seems reasonable to me.

JoshuaKGoldberg

🎉 This is great!

@kirkwaiblinger

##### [v8.19.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8190-2024-12-30) ##### 🚀 Features - **eslint-plugin:** \[strict-boolean-expressions] check array predicate functions' return statements ([#10106](typescript-eslint/typescript-eslint#10106)) ##### 🩹 Fixes - **eslint-plugin:** \[member-ordering] ignore method overloading ([#10536](typescript-eslint/typescript-eslint#10536)) - **eslint-plugin:** \[consistent-indexed-object-style] don't report on indirect circular references ([#10537](typescript-eslint/typescript-eslint#10537)) - **eslint-plugin:** \[array-type] autofix with conditional types needs parentheses ([#10522](typescript-eslint/typescript-eslint#10522)) - **eslint-plugin:** add getConstraintInfo to handle generic constraints better ([#10496](typescript-eslint/typescript-eslint#10496)) ##### ❤️ Thank You - Karl Werner - Kirk Waiblinger [@kirkwaiblinger](https://github.com/kirkwaiblinger) - Ronen Amiel - YeonJuan [@yeonjuan](https://github.com/yeonjuan) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

…ts better (typescript-eslint#10496) * add getConstraintTypeInfoAtLocation * revisit typescript-eslint#10314 * handle typescript-eslint#10443 * tweak tests * Apply suggestions from code review Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com> * code review * deal with deprecated API lint failures * space * remove vfs, derp * typo * simplify * more simplify * jsdoc * more better * better --------- Co-authored-by: typescript-eslint[bot] <53356952+typescript-eslint[bot]@users.noreply.github.com> Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>

@kirkwaiblinger

| datasource | package | from | to | | ---------- | -------------------------------- | ------ | ------ | | npm | @typescript-eslint/eslint-plugin | 8.18.2 | 8.19.0 | | npm | @typescript-eslint/parser | 8.18.2 | 8.19.0 | ## [v8.19.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8190-2024-12-30) ##### 🚀 Features - **eslint-plugin:** \[strict-boolean-expressions] check array predicate functions' return statements ([#10106](typescript-eslint/typescript-eslint#10106)) ##### 🩹 Fixes - **eslint-plugin:** \[member-ordering] ignore method overloading ([#10536](typescript-eslint/typescript-eslint#10536)) - **eslint-plugin:** \[consistent-indexed-object-style] don't report on indirect circular references ([#10537](typescript-eslint/typescript-eslint#10537)) - **eslint-plugin:** \[array-type] autofix with conditional types needs parentheses ([#10522](typescript-eslint/typescript-eslint#10522)) - **eslint-plugin:** add getConstraintInfo to handle generic constraints better ([#10496](typescript-eslint/typescript-eslint#10496)) ##### ❤️ Thank You - Karl Werner - Kirk Waiblinger [@kirkwaiblinger](https://github.com/kirkwaiblinger) - Ronen Amiel - YeonJuan [@yeonjuan](https://github.com/yeonjuan) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

typescript-eslint bot and others added 3 commits December 13, 2024 16:31

add getConstraintTypeInfoAtLocation

d7bcdc6

revisit typescript-eslint#10314

cd97692

handle typescript-eslint#10443

3626dcb

kirkwaiblinger mentioned this pull request Dec 14, 2024

Enhancement: [no-deprecated] Add 'allow' option for specific types or values #9899

Closed

4 tasks

kirkwaiblinger marked this pull request as ready for review December 14, 2024 00:26

kirkwaiblinger commented Dec 14, 2024

View reviewed changes

packages/type-utils/tests/getConstrainedTypeAtLocation.test.ts Outdated Show resolved Hide resolved

tweak tests

bc12e04

JoshuaKGoldberg previously approved these changes Dec 14, 2024

View reviewed changes

JoshuaKGoldberg added the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Dec 14, 2024

Apply suggestions from code review

f0ebffa

Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>

kirkwaiblinger dismissed JoshuaKGoldberg’s stale review via f0ebffa December 14, 2024 18:00

kirkwaiblinger added 3 commits December 14, 2024 11:04

code review

2fa6426

Merge branch 'main' into get-constraint-type-info-at-location

2e0bd42

deal with deprecated API lint failures

e753c02

kirkwaiblinger requested a review from JoshuaKGoldberg December 14, 2024 18:58

space

11d3a0a

github-actions bot removed the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Dec 14, 2024

JoshuaKGoldberg previously approved these changes Dec 15, 2024

View reviewed changes

JoshuaKGoldberg added the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Dec 15, 2024

remove vfs, derp

5d7cab6

kirkwaiblinger dismissed JoshuaKGoldberg’s stale review via 5d7cab6 December 16, 2024 04:17

kirkwaiblinger requested a review from JoshuaKGoldberg December 16, 2024 04:18

github-actions bot removed the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Dec 16, 2024

typo

f383280

kirkwaiblinger commented Dec 17, 2024

View reviewed changes

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Dec 26, 2024

kirkwaiblinger added 6 commits December 26, 2024 18:01

Merge branch 'main' into get-constraint-type-info-at-location

c8c985c

simplify

2aecb98

more simplify

2a5c248

jsdoc

c646ffb

more better

943dc75

better

4c9da08

kirkwaiblinger changed the title ~~fix(type-utils): add getConstraintTypeInfoAtLocation and deprecate getConstrainedTypeAtLocation~~ fix(eslint-plugin): add getConstraintInfo to handle generic constraints better Dec 29, 2024

kirkwaiblinger removed the awaiting response Issues waiting for a reply from the OP or another party label Dec 29, 2024

JoshuaKGoldberg approved these changes Dec 29, 2024

View reviewed changes

JoshuaKGoldberg added the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Dec 29, 2024

JoshuaKGoldberg merged commit 2e2731d into typescript-eslint:main Dec 30, 2024
72 of 75 checks passed

kirkwaiblinger mentioned this pull request Dec 30, 2024

Repo: Adopt getConstraintInfo internally #10569

Open

github-actions bot locked as resolved and limited conversation to collaborators Jan 7, 2025

kirkwaiblinger deleted the get-constraint-type-info-at-location branch March 31, 2025 01:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(eslint-plugin): add getConstraintInfo to handle generic constraints better #10496

fix(eslint-plugin): add getConstraintInfo to handle generic constraints better #10496

kirkwaiblinger commented Dec 13, 2024 •

edited

Loading

typescript-eslint bot commented Dec 13, 2024

netlify bot commented Dec 13, 2024 •

edited

Loading

nx-cloud bot commented Dec 13, 2024 •

edited

Loading

codecov bot commented Dec 14, 2024 •

edited

Loading

JoshuaKGoldberg left a comment

JoshuaKGoldberg left a comment

kirkwaiblinger Dec 17, 2024

JoshuaKGoldberg Dec 26, 2024

kirkwaiblinger Dec 26, 2024

kirkwaiblinger Dec 27, 2024

JoshuaKGoldberg Dec 27, 2024

kirkwaiblinger Dec 29, 2024

JoshuaKGoldberg Dec 29, 2024

JoshuaKGoldberg left a comment

fix(eslint-plugin): add getConstraintInfo to handle generic constraints better #10496

fix(eslint-plugin): add getConstraintInfo to handle generic constraints better #10496

Conversation

kirkwaiblinger commented Dec 13, 2024 • edited Loading

PR Checklist

Overview

typescript-eslint bot commented Dec 13, 2024

netlify bot commented Dec 13, 2024 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

nx-cloud bot commented Dec 13, 2024 • edited Loading

codecov bot commented Dec 14, 2024 • edited Loading

Codecov Report

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

kirkwaiblinger Dec 17, 2024

Choose a reason for hiding this comment

JoshuaKGoldberg Dec 26, 2024

Choose a reason for hiding this comment

kirkwaiblinger Dec 26, 2024

Choose a reason for hiding this comment

kirkwaiblinger Dec 27, 2024

Choose a reason for hiding this comment

JoshuaKGoldberg Dec 27, 2024

Choose a reason for hiding this comment

kirkwaiblinger Dec 29, 2024

Choose a reason for hiding this comment

JoshuaKGoldberg Dec 29, 2024

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

kirkwaiblinger commented Dec 13, 2024 •

edited

Loading

netlify bot commented Dec 13, 2024 •

edited

Loading

nx-cloud bot commented Dec 13, 2024 •

edited

Loading

codecov bot commented Dec 14, 2024 •

edited

Loading