feat(eslint-plugin): add no-duplicate-type-constituents rule

[sajikix]

PR Checklist

• Addresses an existing open issue: fixes Rule proposal: disallow duplicate union and intersection types #2738
• That issue was marked as accepting prs
• Steps in CONTRIBUTING.md were taken

Overview

Add new rule : no-duplicate-type-union-intersection-members.

This rule disallows duplicate union or intersection type members. The autofixer will remove all duplicates, keeping the first one.

I implemented it so that only notational duplicates are checked.
It means that members with the same value but different names are not marked duplicates.

type A = { a: string; b: number };
type B = { a: string; b: number };

type AorB = A | B; // Type A and B are the same type, but are not considered duplicates.

In the case of object types, it judged as duplicates only when the order of key and value are exactly the same.

// ❌ berfore remove duplicates
type T =
  | { a: string; b: number }
  | { a: string; b: number } // considered duplicates with the first one. 
  | { b: number; a: string }; // not considered duplicates with the first one. 
// ↓ 
// ✅ after remove duplicates
type T =
  | { a: string; b: number }
  | { b: number; a: string };

What has considered a duplicate in object type is a matter of debate. I would love to hear the reviewers' opinions and would like to improve it with the reviewers' input.

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 2bae913. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

---

✅ Successfully ran 10 targets

• nx run-many --target=typecheck --parallel
• Node 14 - nx run-many --target=build --parallel --exclude website
• Node 14 - nx run-many --target=build --parallel --exclude website
• nx run-many --target=lint --parallel
• Node 14 - nx test ast-spec --coverage=false
• Node 14 - nx run-many --target=build --parallel --exclude website
• nx run-many --target=build --parallel --exclude website
• nx run-many --target=build --parallel --exclude website
• nx run-many --target=build --parallel --exclude website
• nx run-many --target=build --parallel --exclude website

---

Sent with 💌 from NxCloud.

[typescript-eslint]

Thanks for the PR, @sajikix!

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. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

[netlify]

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	abb92d8
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/641d34c527f83800089d8e9e
😎 Deploy Preview	https://deploy-preview-5728--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[JoshuaKGoldberg]

A great start, thanks for sending this in! 💓

Left some requests for changes - lmk if any of that doesn't make sense!

[codecov]

Codecov Report

Merging #5728 (abb92d8) into main (720e811) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5728      +/-   ##
==========================================
+ Coverage   87.24%   87.29%   +0.05%     
==========================================
  Files         383      384       +1     
  Lines       13080    13139      +59     
  Branches     3845     3859      +14     
==========================================
+ Hits        11411    11470      +59     
  Misses       1302     1302              
  Partials      367      367              

Flag	Coverage Δ
unittest	87.29% <100.00%> (+0.05%)	⬆️

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

Impacted Files	Coverage Δ
...plugin/src/rules/no-duplicate-type-constituents.ts	100.00% <100.00%> (ø)

[sajikix]

Sorry for the wait since I got your review!

There have been two major changes since previous review.

1. change duplicate check to recursive ASTNode comparison instead of string comparison from sourceCode.getText.
2. use removeRange instead of replaceText in the fix function.

1. use recursive ASTNode comparison

As you reviewed, the previous implementation compared pure code strings, so there was a bug that prevented comparison if there were comments, etc. in the union/intersction type.
To resolve this, I changed to compare ASTNode recursively. Specifically, this rule is looking for an exact match for each ASTNode excluding range, loc, and parent properties. (I first tried to write a comparison process for each AstTypeNode, but there are so many cases that I am not confident I could cover them... 😭 )
I believe this method can be used to compare union/intersection members, but do you have any concerns?

2. use removeRange()

As you reviewed, we are using removeRange instead of replaceText in the fix function. Specifically, I set the removeRange as follows.

type T = A | A | B;
//        ^   ^
//     start  end

// start : end loc of the member before the duplicated member
// end : end loc of the duplicated member

// ↓ Deleting this range will result in ....
type T = A | B;

This is a relatively large change, but I would appreciate your review. 🙏

[JoshuaKGoldberg]

Progress! I think this is getting closer - but nailing down edge cases (i.e. switching to === type equality) is going to be key.

[sajikix]

Thank you so much for your detailed explanation!
And I finally realized the problem with my implementation. 🙏

The problem with the current behavior is that there are multiple fixers (number of detected duplicates) that remove the exact same duplicate (the one at the end), right?
And this kind of fixer behavior is not very good for eslint's autofix behavior: .... 😭

Now that I am aware of the problem, I would like to implement it according to the second solution you suggested (= report / fixer only the first duplicates found in constituents).
In addition, I will include a change to correctly specify the node and loc options in the report function. This will make it easier to see which parts are duplicates and which parts will be removed. (You suggested this too!).

However, I am wondering if the above change (pointing out the first detected duplicate) should be applied to the report function as well, or only to the fixer.
In other words, which of the following is the preferred behavior?

1. report and provide fixer only the first duplicate detected

type F = (A | B) | (A | B) | ((C | D) & (A | B)) | (A | B);
// reported         ^^^^^
// fixer            ^^^^^

2. provide a fixer only for the first duplicate detected (report all duplicated)

type F = (A | B) | (A | B) | (C | D) & (A | B) | (A | B);
// reported         ^^^^^     ^^^^^^^^^^^^^^^     ^^^^^
// fixer            ^^^^^

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit abb92d8. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

---

✅ Successfully ran 25 targets

• Node 14 - nx test eslint-plugin --coverage=false
• Node 18 - nx test eslint-plugin
• Node 18 - nx test utils
• Node 18 - nx run-many --target=build --parallel --exclude website
• Node 18 - nx test visitor-keys
• nx run-many --target=typecheck --parallel
• Node 18 - nx test type-utils
• nx run-many --target=lint --parallel
• Node 14 - nx run-many --target=build --parallel --exclude website
• Node 14 - nx test ast-spec --coverage=false
• nx run-many --target=build --parallel --exclude website
• Node 18 - nx test typescript-estree
• Node 18 - nx test ast-spec
• Node 18 - nx test parser
• Node 14 - nx test type-utils --coverage=false
• Node 18 - nx test scope-manager
• Node 14 - nx test typescript-estree --coverage=false
• Node 14 - nx test parser --coverage=false
• Node 14 - nx test utils --coverage=false
• Node 14 - nx test eslint-plugin-internal --coverage=false
• Node 18 - nx test eslint-plugin-tslint
• Node 18 - nx test eslint-plugin-internal
• Node 14 - nx test eslint-plugin-tslint --coverage=false
• Node 14 - nx test visitor-keys --coverage=false
• Node 14 - nx test scope-manager --coverage=false

---

Sent with 💌 from NxCloud.

[sajikix]

I have addressed the issues regarding the range of the fixer function and the clarity of the error reporting scope!

You can see these improvements in action by checking out the playground. (It should be fixed properly 🙏 )

Talking about implementation, I have cut the report function and changed the implementation to call it each time a duplicate is found.

[armano2]

this look great, thank you for your hard work

[JoshuaKGoldberg]

This is looking really great, nice work @sajikix! 👏

Requesting changes on performance tuning, non-generated tests, and some small touchups. Awesome! Looking forward this getting shipped!

[JoshuaKGoldberg]

🔥 this is wonderful, thanks a million @sajikix! Really excited to get this out in front of users. Really appreciate you working with us on it!

I'll also modify #6014 to indicate we'll add this to the recommended-type-checked ruleset. Because IMO it should be a recommended rule. 😁

[sajikix]

Thank you again and again for your kind review! 🙏
And we are very happy that you will be adding it to the ruleset for v6! 😁

Do I need to do any additional work on this PR? (Is it OK if I leave the rest to you?)

[JoshuaKGoldberg]

none needed, was just waiting for the build to pass then got distracted 😄