Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Conversation

@mdm317
Copy link
Contributor

@mdm317mdm317 commentedJan 29, 2025
edited
Loading

PR Checklist

Overview

even cases where they're deeply equal, but declared as different object types.

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

type ShallowEqual = number | number;type DeepEqual = { a: string } | { a: string };

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

type Foo = { a: 1 } | { a: 1, b: 1 }const foo: Foo = { a: 1 }const bar: Foo = { a: 1, b: 1 }type Foo2 = { a: 1 } const foo2: Foo2 = { a: 1 }const bar2: Foo2 = { a: 1, b: 1 } //error

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.
ForA | B, i checks ifA andB have the same keys. If they do, it then checks assignability.

Union Types with Intersection Types

  • Union with a Single Intersection:
    • If a union type contains an intersection, like A | (B & C & D), the intersection (B & C & D) is treated as a single object type.
  • Union within an Intersection:
    • If an intersection includes a union, like A | (B & (C | D)), TypeScript simplifies the intersection into a union of intersections. B & (C | D) is equivalent to B & C | B & D,
    • This means the assignability check becomes: Is A assignable to (B & C) | (B & D)

Intersection
Unlike union types, intersection types did not need excess property checks.
A & B in this case ifA is assignable toB B is rebundant.

  • If there is no union type within the intersection, I proceed the assignability check.A & B
  • If there is union type, I check if every type in the union is compatible with the intersection and follows the same rules.
  • A & ( B | C)
A >= B, A >= C  Then  A >= (B | C)B >= A, C >= A  Then   (B | C) >= A

@typescript-eslint
Copy link
Contributor

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.

@netlify
Copy link

netlifybot commentedJan 29, 2025
edited
Loading

Deploy Preview fortypescript-eslint ready!

NameLink
🔨 Latest commitacb39d8
🔍 Latest deploy loghttps://app.netlify.com/projects/typescript-eslint/deploys/690ec977b986270008806f99
😎 Deploy Previewhttps://deploy-preview-10744--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 99 (🟢 up 3 from production)
Accessibility: 97 (no change from production)
Best Practices: 100 (no change from production)
SEO: 92 (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 yourNetlify project configuration.

@nx-cloud
Copy link

nx-cloudbot commentedJan 29, 2025
edited
Loading

View yourCI Pipeline Execution ↗ for commitacb39d8

CommandStatusDurationResult
nx test eslint-plugin --coverage=false✅ Succeeded5m 17sView ↗
nx run-many -t lint✅ Succeeded3m 29sView ↗
nx run-many -t typecheck✅ Succeeded2m 14sView ↗
nx test eslint-plugin-internal --coverage=false✅ Succeeded4sView ↗
nx run types:build✅ Succeeded5sView ↗
nx run integration-tests:test✅ Succeeded6sView ↗
nx test typescript-estree --coverage=false✅ Succeeded2sView ↗
nx run generate-configs✅ Succeeded6sView ↗
Additional runs (29)✅ Succeeded...View ↗

☁️Nx Cloud last updated this comment at2025-11-08 04:52:01 UTC

@codecov
Copy link

codecovbot commentedJan 29, 2025
edited
Loading

Codecov Report

❌ Patch coverage is92.07746% with45 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.61%. Comparing base (ea2ee6b) to head (acb39d8).
⚠️ Report is 26 commits behind head on main.

Files with missing linesPatch %Lines
...plugin/src/rules/no-redundant-type-constituents.ts92.38%42 Missing and 1 partial⚠️
...escript-estree/src/semantic-or-syntactic-errors.ts0.00%2 Missing⚠️
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
FlagCoverage Δ
unittest90.61% <92.07%> (-0.05%)⬇️

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

Files with missing linesCoverage Δ
...ages/eslint-plugin/src/rules/no-unsafe-argument.ts93.24% <100.00%> (ø)
...nt-plugin/src/rules/related-getter-setter-pairs.ts100.00% <ø> (ø)
...escript-estree/src/semantic-or-syntactic-errors.ts2.32% <0.00%> (ø)
...plugin/src/rules/no-redundant-type-constituents.ts93.41% <92.38%> (-4.23%)⬇️

... and3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JoshuaKGoldberg
Copy link
Member

👋 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
Copy link
ContributorAuthor

Sorry for the delay. This issue was more challenging than I expected.
I'll try to finish it within two days. If it takes longer, I'll close the PR and reopen it once it's resolved.
Moving forward, if I anticipate a delay, I'll close it first and reopen it when it's ready.

JoshuaKGoldberg reacted with heart emoji

@mdm317mdm317 marked this pull request as ready for reviewMarch 5, 2025 19:03
Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg left a 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! 🙌

mdm317 reacted with heart emoji
@JoshuaKGoldbergJoshuaKGoldberg added the awaiting responseIssues waiting for a reply from the OP or another party labelMar 17, 2025
@mdm317
Copy link
ContributorAuthor

Thank you for your review!
I'll try implementing it according to your suggestion.

JoshuaKGoldberg reacted with rocket emoji

Copy link
ContributorAuthor

@mdm317mdm317 left a comment
edited
Loading

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.
  • 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 whenisTypeAssignableTo returns true.

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){
Copy link
ContributorAuthor

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;      };

Copy link
Member

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.

mdm317 reacted with thumbs up emoji
}
if(
declaration.kind!==ts.SyntaxKind.TypeLiteral&&
declaration.kind!==ts.SyntaxKind.InterfaceDeclaration&&
Copy link
ContributorAuthor

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

@mdm317mdm317 marked this pull request as ready for reviewNovember 9, 2025 08:51
@JoshuaKGoldbergJoshuaKGoldberg removed the awaiting responseIssues waiting for a reply from the OP or another party labelNov 10, 2025
Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg left a 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).

mdm317 reacted with heart emoji
@JoshuaKGoldbergJoshuaKGoldberg added the 1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge labelNov 18, 2025
checker:ts.TypeChecker,
depth=0,
):boolean{
if(depth>10){
Copy link
Member

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.

mdm317 reacted with thumbs up emoji
@bradzacherbradzacher merged commit2ffb168 intotypescript-eslint:mainNov 23, 2025
67 of 69 checks passed
renovatebot added a commit to andrei-picus-tink/auto-renovate that referenced this pull requestNov 24, 2025
| 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.
renovatebot added a commit to andrei-picus-tink/auto-renovate that referenced this pull requestNov 24, 2025
| 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.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@JoshuaKGoldbergJoshuaKGoldbergJoshuaKGoldberg approved these changes

@bradzacherbradzacherbradzacher approved these changes

Assignees

No one assigned

Labels

1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Enhancement: [no-redundant-type-constituents] Use assignability checking for redundancy checks

4 participants

@mdm317@JoshuaKGoldberg@bradzacher@gen-ip-1

[8]ページ先頭

©2009-2025 Movatter.jp