-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
feat(eslint-plugin): [no-redundant-type-constituents] use assignability checking for redundancy checks #10744
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
feat(eslint-plugin): [no-redundant-type-constituents] use assignability checking for redundancy checks #10744
Conversation
|
Thanks for the PR, @mdm317! 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 project configuration. |
|
View your CI Pipeline Execution ↗ for commit acb39d8
☁️ Nx Cloud last updated this comment at |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #10744 +/- ##
==========================================
- Coverage 90.66% 90.61% -0.05%
==========================================
Files 518 518
Lines 52435 52733 +298
Branches 8686 8771 +85
==========================================
+ Hits 47541 47785 +244
- Misses 4880 4933 +53
- Partials 14 15 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
we can not know typename {a : 2 } using typetostring
type B = {a : 2};
type T = { a: 1 } | B
|
👋 Ping @mdm317, are you planning on un-draft ing this soon? We'd prefer not to keep drafts around for too long in case someone else would want to send one too. |
|
Sorry for the delay. This issue was more challenging than I expected. |
JoshuaKGoldberg
left a comment
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! Sorry for the delay in reviewing. This is a really tricky one 😅.
I think the feature request is a lot trickier than the test cases here currently capture. It'll have to handle a lot of cases, including optional properties, interfaces extending each other, and so on. I suspect the core implementation will have to adjust a bunch - so left a few suggestions on how to reduce the amount of work done. Hope it's helpful, and let me know if you have questions! 🙌
packages/eslint-plugin/src/rules/no-redundant-type-constituents.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/tests/rules/no-redundant-type-constituents.test.ts
Show resolved
Hide resolved
packages/eslint-plugin/src/rules/no-redundant-type-constituents.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/src/rules/no-redundant-type-constituents.ts
Outdated
Show resolved
Hide resolved
|
Thank you for your review! |
JoshuaKGoldberg
left a comment
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.
🥳 Whoohoo! Thanks so much for bearing with me on reviews and making so many refactors. This is a lot of code by necessity, but it's roughly as clean as I think it can get. Nice job!
Just to be safe I'd like to get another review from someone on @typescript-eslint/triage-team if possible (though we are swamped, and I don't want to wait beyond the end of this month).
| checker: ts.TypeChecker, | ||
| depth = 0, | ||
| ): boolean { | ||
| if (depth > 10) { |
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.
another way to do this is to keep track of "seen" types.
If the type is already in the set then it's recursive.
| datasource | package | from | to | | ---------- | -------------------------------- | ------ | ------ | | npm | @typescript-eslint/eslint-plugin | 8.47.0 | 8.48.0 | | npm | @typescript-eslint/parser | 8.47.0 | 8.48.0 | ## [v8.48.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8480-2025-11-24) ##### 🚀 Features - **eslint-plugin:** \[no-redundant-type-constituents] use assignability checking for redundancy checks ([#10744](typescript-eslint/typescript-eslint#10744)) ##### 🩹 Fixes - **typescript-estree:** disallow binding patterns in parameter properties ([#11760](typescript-eslint/typescript-eslint#11760)) - **eslint-plugin:** \[consistent-generic-constructors] ignore when constructor is typed array ([#10477](typescript-eslint/typescript-eslint#10477)) ##### ❤️ Thank You - Dima Barabash [@dbarabashh](https://github.com/dbarabashh) - JamesHenry [@JamesHenry](https://github.com/JamesHenry) - Josh Goldberg - mdm317 [@gen-ip-1](https://github.com/gen-ip-1) You can read about our [versioning strategy](https://typescript-eslint.io/users/versioning) and [releases](https://typescript-eslint.io/users/releases) on our website.
| datasource | package | from | to | | ---------- | -------------------------------- | ------ | ------ | | npm | @typescript-eslint/eslint-plugin | 8.47.0 | 8.48.0 | | npm | @typescript-eslint/parser | 8.47.0 | 8.48.0 | ## [v8.48.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8480-2025-11-24) ##### 🚀 Features - **eslint-plugin:** \[no-redundant-type-constituents] use assignability checking for redundancy checks ([#10744](typescript-eslint/typescript-eslint#10744)) ##### 🩹 Fixes - **typescript-estree:** disallow binding patterns in parameter properties ([#11760](typescript-eslint/typescript-eslint#11760)) - **eslint-plugin:** \[consistent-generic-constructors] ignore when constructor is typed array ([#10477](typescript-eslint/typescript-eslint#10477)) ##### ❤️ Thank You - Dima Barabash [@dbarabashh](https://github.com/dbarabashh) - JamesHenry [@JamesHenry](https://github.com/JamesHenry) - Josh Goldberg - mdm317 [@gen-ip-1](https://github.com/gen-ip-1) You can read about our [versioning strategy](https://typescript-eslint.io/users/versioning) and [releases](https://typescript-eslint.io/users/releases) on our website.
…; re-enable the bugged lint rule: eslint-plugin: revert: [no-redundant-type-constituents] use assignability checking for redundancy checks (typescript-eslint/typescript-eslint#10744) (typescript-eslint/typescript-eslint#11812)
…; re-enable the bugged lint rule: - eslint-plugin: revert: [no-redundant-type-constituents] use assignability checking for redundancy checks (typescript-eslint/typescript-eslint#10744) (typescript-eslint/typescript-eslint#11812) - chore: update pkg
This PR contains the following updates: | Package | Change | Age | Confidence | |---|---|---|---| | [@typescript-eslint/eslint-plugin](https://typescript-eslint.io/packages/eslint-plugin) ([source](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/eslint-plugin)) | [`8.47.0` -> `8.48.1`](https://renovatebot.com/diffs/npm/@typescript-eslint%2feslint-plugin/8.47.0/8.48.1) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | | [@typescript-eslint/parser](https://typescript-eslint.io/packages/parser) ([source](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/parser)) | [`8.47.0` -> `8.48.1`](https://renovatebot.com/diffs/npm/@typescript-eslint%2fparser/8.47.0/8.48.1) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>typescript-eslint/typescript-eslint (@​typescript-eslint/eslint-plugin)</summary> ### [`v8.48.1`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8481-2025-12-02) [Compare Source](typescript-eslint/typescript-eslint@v8.48.0...v8.48.1) ##### 🩹 Fixes - **eslint-plugin:** \[restrict-template-expressions] check base types in allow list ([#​11764](typescript-eslint/typescript-eslint#11764), [#​11759](typescript-eslint/typescript-eslint#11759)) - **eslint-plugin:** honor ignored base types on generic classes ([#​11767](typescript-eslint/typescript-eslint#11767)) - **eslint-plugin:** \[consistent-type-exports] check value flag before resolving alias ([#​11769](typescript-eslint/typescript-eslint#11769)) ##### ❤️ Thank You - Josh Goldberg - OleksandraKordonets - SangheeSon [@​Higangssh](https://github.com/Higangssh) - tao You can read about our [versioning strategy](https://typescript-eslint.io/users/versioning) and [releases](https://typescript-eslint.io/users/releases) on our website. ### [`v8.48.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8480-2025-11-24) [Compare Source](typescript-eslint/typescript-eslint@v8.47.0...v8.48.0) ##### 🚀 Features - **eslint-plugin:** \[no-redundant-type-constituents] use assignability checking for redundancy checks ([#​10744](typescript-eslint/typescript-eslint#10744)) ##### 🩹 Fixes - **typescript-estree:** disallow binding patterns in parameter properties ([#​11760](typescript-eslint/typescript-eslint#11760)) - **eslint-plugin:** \[consistent-generic-constructors] ignore when constructor is typed array ([#​10477](typescript-eslint/typescript-eslint#10477)) ##### ❤️ Thank You - Dima Barabash [@​dbarabashh](https://github.com/dbarabashh) - JamesHenry [@​JamesHenry](https://github.com/JamesHenry) - Josh Goldberg - mdm317 [@​gen-ip-1](https://github.com/gen-ip-1) You can read about our [versioning strategy](https://typescript-eslint.io/users/versioning) and [releases](https://typescript-eslint.io/users/releases) on our website. </details> <details> <summary>typescript-eslint/typescript-eslint (@​typescript-eslint/parser)</summary> ### [`v8.48.1`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#8481-2025-12-02) [Compare Source](typescript-eslint/typescript-eslint@v8.48.0...v8.48.1) This was a version bump only for parser to align it with other projects, there were no code changes. You can read about our [versioning strategy](https://typescript-eslint.io/users/versioning) and [releases](https://typescript-eslint.io/users/releases) on our website. ### [`v8.48.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#8480-2025-11-24) [Compare Source](typescript-eslint/typescript-eslint@v8.47.0...v8.48.0) This was a version bump only for parser to align it with other projects, there were no code changes. You can read about our [versioning strategy](https://typescript-eslint.io/users/versioning) and [releases](https://typescript-eslint.io/users/releases) on our website. </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0Mi4xNC4yIiwidXBkYXRlZEluVmVyIjoiNDIuMTQuMiIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOltdfQ==--> Reviewed-on: https://git.csmpro.ru/csmpro/mapban/pulls/57 Co-authored-by: Renovate Bot <renovate@csmpro.ru> Co-committed-by: Renovate Bot <renovate@csmpro.ru>

PR Checklist
Overview
This feature seems to overlap with this rule no-duplicate-type-constituents so I'm not sure whether to include it.
For now, I'll remove it and proceed.
deeply equal
Union
If x is assignable to y, then in the union x∣y, x is redundant and can be removed.
However, due to TypeScript's Excess Property Checks feature, this logic did not work as expected.
For example, consider the union type
{ a: 1 } | { a: 1, b: 1 }. Since{ a: 1, b: 1 }is assignable to{ a: 1 }So
{ a: 1, b: 1 }becomes redundant and can be removed.However, if
{ a: 1, b: 1 }is removed, the remaining type{ a: 1 }exhibits different behavior when declaring values.For example,
{ a: 1 }alone can only be assigned to variables of type{ a: 1 }, whereas{ a: 1 } | { a: 1, b: 1 }allows the declaration of both{ a: 1 }and{ a: 1, b: 1 }Example code
So, before checking whether one type is assignable to another, I first verified that both objects have the same set of keys. Only then did I proceed with the assignability check.
For
A | B, i checks ifAandBhave the same keys. If they do, it then checks assignability.Union Types with Intersection Types
B & (C | D) is equivalent to B & C | B & D,Intersection
Unlike union types, intersection types did not need excess property checks.
A & Bin this case ifAis assignable toBB is rebundant.A & BA & ( B | C)