Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
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 ourterms of service andprivacy 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 onhttps://opencollective.com/typescript-eslint. |
netlifybot commentedJan 29, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
✅ Deploy Preview fortypescript-eslint ready!
To edit notification comments on pull requests, go to yourNetlify project configuration. |
nx-cloudbot commentedJan 29, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
View yourCI Pipeline Execution ↗ for commitacb39d8
☁️Nx Cloud last updated this comment at |
codecovbot commentedJan 29, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 typetostringtype B = {a : 2};type T = { a: 1 } | BJoshuaKGoldberg commentedMar 3, 2025
👋 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. |
mdm317 commentedMar 3, 2025
Sorry for the delay. This issue was more challenging than I expected. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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 OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
packages/eslint-plugin/src/rules/no-redundant-type-constituents.ts OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
packages/eslint-plugin/src/rules/no-redundant-type-constituents.ts OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
mdm317 commentedMar 20, 2025
Thank you for your review! |
mdm317 left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Sorry for late. 😔
This code was quite old even for me.
It took some time to review it again and fix a few issues
fixes:
- Handle union property types correctly.
- I used isTypeAssignableTo when the object’s property was a union type, but the result wasn’t redundant, so I fixed it.
type T = { a: 3 | { a: 1, b: 1 } } | { a: 3 | { a: 1 } } ^~~~~~~~~~~~ should not report- Added a check to skip self-referencing types (depth > 10) to prevent a maximum call stack error.
- Because of this feature, files in ast-spec can’t be checked (most of them use recursive types.)
So I reverted the changes made to ast-spec.
- Because of this feature, files in ast-spec can’t be checked (most of them use recursive types.)
- Changed it to only check Object types that are arrays, plain objects, or mapped types.
- Don't split union types when dealing with reference types.
- Skip mapped types with no properties.
- Not sure if it's redundant when
isTypeAssignableToreturns true.
- Not sure if it's redundant when
limitation
Currently, the logic checks redundant types based on their properties when the type is ants.ObjectType.
As a result, function types — which have no properties — weren’t properly handled.
Therefore, the check is now limited to object, array, and mapped types only.
| 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.
Skip check if depth > 10 (assume self-reference)
type Node = { value: string; next?: Node; };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.
| } | ||
| if( | ||
| declaration.kind!==ts.SyntaxKind.TypeLiteral&& | ||
| declaration.kind!==ts.SyntaxKind.InterfaceDeclaration&& |
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.
Skip check if type isfunction orclass
Uh oh!
There was an error while loading.Please reload this page.
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.
2ffb168 intotypescript-eslint:mainUh oh!
There was an error while loading.Please reload this page.
| 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.

Uh oh!
There was an error while loading.Please reload this page.
PR Checklist
Overview
This feature seems to overlap with this ruleno-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'sExcess 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)