fix(eslint-plugin): add getConstraintInfo to handle generic constraints better #10496
Conversation
|
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. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
View your CI Pipeline Execution ↗ for commit 4c9da08.
☁️ Nx Cloud last updated this comment at |
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
| * | ||
| * Successor to {@link getConstrainedTypeAtLocation} due to https://github.com/typescript-eslint/typescript-eslint/issues/10438 | ||
| */ | ||
| export function getConstraintTypeInfoAtLocation( |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@kirkwaiblinger just confirming, are you planning on making changes? And/or, is this a question?
There was a problem hiding this comment.
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! 😁
There was a problem hiding this comment.
If you're on board with this plan, then this PR is up to date and ready to be merged 👍
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
I like this strategy. The union of possible constraint info points seems reasonable to me.
2e2731d
into
typescript-eslint:main
##### [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>
| 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.
PR Checklist
fixes (not anymore) Bug: [no-unnecessary-boolean-literal-compare] doesn't flag values of a type parameter with boolean type constraint #10443EDIT - this was already solved in the meantime, just refactoringOverview
Since removing or modifying
getConstrainedTypeAtLocationwould 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 placeI'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 existinggetConstrainedTypeAtLocation. 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
getConstrainedTypeAtLocationactually 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 #10443and in #10474TODO - 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 aliasgetConstrainedTypeAtLocationas the ironically not-deprecatedDEPRECATED_getConstrainedTypeAtLocationin order to allow existing usages not to fail CI.