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
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
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 site 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.
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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #10744 +/- ##==========================================- Coverage 90.82% 90.81% -0.01%========================================== Files 497 497 Lines 50204 50368 +164 Branches 8274 8306 +32 ==========================================+ Hits 45600 45744 +144- Misses 4589 4608 +19- Partials 15 16 +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 } | 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. |
Uh oh!
There was an error while loading.Please reload this page.
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.
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.
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.
[Testing] There are going to be quite a few variants of types that could break the rule:
- Conditional types
- Generic types & their type parameters
- Mapped types
- Optional properties
- Interfaces
- Interfaces extending other interfaces
keyofandtypeoftypesReturnType,Omit, etc. types- Symbols
Etc. - I haven't though super deeply beyond that. Could you think on what other test cases should happen and fill it all out?
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.
The current logic works well for union types, but contains errors when handling intersection types.
Some mapped type has 0 property
- Type is a generic type parameter that hasn't been specified yet
type U<Type> = {[Property in keyof Type]: 0;}; - An object type with potentially unlimited string keys,
Record<string, number>
| object type | 0 property | 0> property |
|---|---|---|
| 0 property | check assignable | non redundant |
| 0 > property | non redundant | check assignable |
optional property
In typescript type with only the required property a:1 is assignable to type with the same required property plus an optional property b?:1type T = { a : 1 } & { a : 1, b? : 1}
I've added logic that determines: when typeA is assignable to typeB andB contains no extra optional properties beyondA, thenB is considered redundant.
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.
Thank you for your review! |
Hello I have a question about applying rules to our code.
ClassDeclarationWithName is ovveridden by ClassDeclarationWithOptionalName in this union type
FunctionDeclarationWithOptionalName is ovveridden by FunctionDeclarationWithName in this union type In this case, should I remove them, use eslint-disable, or modify the rule itself? |
Ha, that's really interesting. Nice find. My instinct is that we should modify the |
prev: Record<string,number> & { a : 1} {a:1 } is rebundantnow :type T= Record<string,number> & { a : 1} is validThere 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.
This change was made to applying lint to code.
Let me know if you have any concerns about this adjustment
| | FunctionDeclarationWithName | ||
| | FunctionDeclarationWithOptionalName; | ||
| // eslint-disable-next-line @typescript-eslint/no-redundant-type-constituents | ||
| FunctionDeclarationWithName| FunctionDeclarationWithOptionalName; |
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.
Due to Merging Interfaces, it should not be considered as redundant types.
So I added a lint disable comment for the line.
typescript-eslint/packages/types/src/ts-estree.ts
Lines 92 to 96 infa265a9
| interfaceFunctionDeclarationWithName{ | |
| parent: | |
| |TSESTree.BlockStatement | |
| |TSESTree.ExportDefaultDeclaration | |
| |TSESTree.ExportNamedDeclaration |
| @@ -47,4 +43,5 @@ export type NamedExportDeclarations = | |||
| // TODO - breaking change remove this in the next major | |||
| export type ExportDeclaration = | |||
| | DefaultExportDeclarations | |||
| // eslint-disable-next-line @typescript-eslint/no-redundant-type-constituents | |||
| | NamedExportDeclarations; | |||
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.
As in the comment above, It is because of Merging InterfacesFunctionDeclarationWithName
| @@ -244,6 +244,7 @@ export interface ParserServicesBase { | |||
| } | |||
| export interface ParserServicesNodeMaps { | |||
| esTreeNodeToTSNodeMap: ParserWeakMapESTreeToTSNode; | |||
| // eslint-disable-next-line @typescript-eslint/no-redundant-type-constituents | |||
| tsNodeToESTreeNodeMap: ParserWeakMap<TSNode | TSToken, TSESTree.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.
InTSNode's union types, the code below is overridden. I think modifying it too complex, so I added a lint
disable code.
|ts.Modifier
| @@ -49,6 +49,7 @@ export default createRule({ | |||
| * @returns the name and attribute of the member or null if it's a member not relevant to the rule. | |||
| */ | |||
| function getMemberMethod( | |||
| // eslint-disable-next-line @typescript-eslint/no-redundant-type-constituents | |||
| member: Member | MemberDeclaration, | |||
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.
Member -> ProgramStatement -> Statement -> ClassDeclarationWithName
MemberDeclaration -> DefaultExportDeclarations -> ClassDeclarationWithOptionalNameClassDeclarationWithName is overriden byClassDeclarationWithOptionalName
I think modifying it too complex, so I added a lint disable code.
…com/mdm317/typescript-eslint into 7742-no-redundant-type-constituents
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.
I'd appreciate your feedback
| @@ -22,11 +22,11 @@ type IntersectionNever = string | never; | |||
| ~~~~~ 'never' is overridden by other types in this union type. | |||
| type IntersectionBooleanLiteral = boolean & false; | |||
| ~~~~~~~ boolean is overridden bythefalse in this intersection type. | |||
| ~~~~~~~ boolean is overridden by false in this intersection type. | |||
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.
Can we removethe?
| isFunctionOrFunctionType(node.parent.parent) | ||
| ); | ||
| function hasTargetOnlyOptionalProps( | ||
| sourceType: ts.ObjectType, |
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.
Checkint target type contains no extra optional properties beyond sourceType
| (sourceTypeProperties.length === 0) !== | ||
| (targetTypeProperties.length === 0) | ||
| ) { | ||
| return false; |
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.
target type is non redundant
| object type | 0 property | 0> property |
|---|---|---|
| 0 property | check assignable | non redundant |
| 0 > property | non redundant | check assignable |
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)