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

sajikix · 2022-10-03T10:11:01Z

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 · 2022-10-03T10:11:04Z

☁️ 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

Sent with 💌 from NxCloud.

typescript-eslint · 2022-10-03T10:11:05Z

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 · 2022-10-03T10:11:18Z

✅ 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!

packages/eslint-plugin/src/rules/no-duplicate-type-union-intersection-members.ts

packages/eslint-plugin/docs/rules/no-duplicate-type-union-intersection-members.md

packages/eslint-plugin/src/rules/no-duplicate-type-union-intersection-members.ts

…n-members

codecov · 2022-10-11T07:54:17Z

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%> (ø)`

…n-members

sajikix · 2022-10-16T05:46:01Z

Sorry for the wait since I got your review!

There have been two major changes since previous review.

change duplicate check to recursive ASTNode comparison instead of string comparison from sourceCode.getText.
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.

packages/eslint-plugin/src/rules/no-duplicate-type-constituents.ts

…n-members

sajikix · 2023-03-07T05:41:54Z

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?

report and provide fixer only the first duplicate detected

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

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            ^^^^^

…n-members

nx-cloud · 2023-03-22T01:04:39Z

☁️ 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

Sent with 💌 from NxCloud.

sajikix · 2023-03-23T05:10:55Z

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!

packages/eslint-plugin/docs/rules/no-duplicate-type-constituents.md

packages/eslint-plugin/src/rules/no-duplicate-type-constituents.ts

packages/eslint-plugin/tests/rules/no-duplicate-type-constituents.test.ts

…ount of calculation.

This reverts commit a6b2382.

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 · 2023-03-24T16:21:41Z

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 · 2023-03-24T16:41:25Z

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

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.56.0` -> `5.57.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2feslint-plugin/5.56.0/5.57.0) | | [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.56.0` -> `5.57.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2fparser/5.56.0/5.57.0) | --- ### Release Notes <details> <summary>typescript-eslint/typescript-eslint (@typescript-eslint/eslint-plugin)</summary> ### [`v5.57.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#5570-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5560v5570-2023-03-27) [Compare Source](typescript-eslint/typescript-eslint@v5.56.0...v5.57.0) ##### Bug Fixes - **eslint-plugin:** \[no-unnecessary-boolean-literal-compare] simplify fixer and add support for double negation ([#6620](typescript-eslint/typescript-eslint#6620)) ([81c8519](typescript-eslint/typescript-eslint@81c8519)) - **eslint-plugin:** correct crashes with getTypeArguments for ts < 3.7 ([#6767](typescript-eslint/typescript-eslint#6767)) ([59eab58](typescript-eslint/typescript-eslint@59eab58)) ##### Features - **eslint-plugin:** \[consistent-type-assertions] add suggestions for objectLiteralTypeAssertions ([#6642](typescript-eslint/typescript-eslint#6642)) ([720e811](typescript-eslint/typescript-eslint@720e811)) - **eslint-plugin:** \[consistent-type-assertions] autofix angle bracket assertions to as ([#6641](typescript-eslint/typescript-eslint#6641)) ([ad8ea64](typescript-eslint/typescript-eslint@ad8ea64)) - **eslint-plugin:** add `no-duplicate-type-constituents` rule ([#5728](typescript-eslint/typescript-eslint#5728)) ([bc31078](typescript-eslint/typescript-eslint@bc31078)) </details> <details> <summary>typescript-eslint/typescript-eslint (@typescript-eslint/parser)</summary> ### [`v5.57.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#5570-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5560v5570-2023-03-27) [Compare Source](typescript-eslint/typescript-eslint@v5.56.0...v5.57.0) **Note:** Version bump only for package [@typescript-eslint/parser](https://github.com/typescript-eslint/parser) </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. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1831 Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>

sajikix added 3 commits October 1, 2022 19:23

feat: add rule code

6ee8df7

test: add test for rule

90b5e96

docs: add docs of new rule

1a3a620

JoshuaKGoldberg requested changes Oct 3, 2022

View reviewed changes

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Oct 3, 2022

sajikix added 6 commits October 5, 2022 23:38

refactor: make method definitions more concise

29874a8

fix: change check option to ignore option

e03a7e8

refactor: rename to type-constituents

95afec8

refactor: use recursive type-node checker

1e7cbb1

Merge branch 'main' into add-rule-no-duplicate-type-union-intersectio…

3c67cef

…n-members

fix: rename doc filename and test title

39f18bc

bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Oct 15, 2022

sajikix added 4 commits October 15, 2022 19:10

refactor: use removeRage instead of replaceText

f430871

refactor: narrows node comparison function argument type

49e27be

fix: doc description

b368abb

Merge branch 'main' into add-rule-no-duplicate-type-union-intersectio…

4455b49

…n-members

JoshuaKGoldberg changed the title ~~feat(eslint-plugin): add no-duplicate-type-union-intersection-members rule~~ feat(eslint-plugin): add no-duplicate-type-constituents rule Oct 17, 2022

JoshuaKGoldberg requested changes Oct 17, 2022

View reviewed changes

JoshuaKGoldberg mentioned this pull request Oct 17, 2022

Rename sort-type-union-intersection-members to sort-type-constituents #5841

Closed

JoshuaKGoldberg reviewed Oct 17, 2022

View reviewed changes

packages/eslint-plugin/src/rules/no-duplicate-type-constituents.ts Outdated Show resolved Hide resolved

bradzacher added enhancement: new plugin rule New rule request for eslint-plugin awaiting response Issues waiting for a reply from the OP or another party labels Oct 24, 2022

sajikix added 3 commits November 6, 2022 23:38

Merge branch 'main' into add-rule-no-duplicate-type-union-intersectio…

98a008c

…n-members

refactor: update hasComments logic

f1f507d

fix: remove cases that never occur

9e75e07

sajikix added 2 commits March 13, 2023 09:54

Merge branch 'main' into add-rule-no-duplicate-type-union-intersectio…

2bae913

…n-members

Merge branch 'main' into add-rule-no-duplicate-type-union-intersectio…

9e46016

…n-members

refactor: separate report function and solve fixer range problem

73b66a7

bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Mar 23, 2023

armano2 previously approved these changes Mar 23, 2023

View reviewed changes

JoshuaKGoldberg requested changes Mar 23, 2023

View reviewed changes

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Mar 23, 2023

sajikix added 6 commits March 24, 2023 10:34

refactor: improved documentation.

f161c61

fix: make additionalProperties false

255cc8c

fix: delete printing message {{duplicated}}

e8d1aa1

fix: do not abbreviate "unique"

47c2b4c

refactor: reverse the key and value in cachedTypeMap to reduce the am…

9113663

…ount of calculation.

fix: reportLocation start

a55e36f

sajikix dismissed armano2’s stale review via a55e36f March 24, 2023 02:31

sajikix added 4 commits March 24, 2023 14:03

refactor: stop test generation and write tests naively.

7a85718

refactor: Narrowing the type of options

a6b2382

Revert "refactor: Narrowing the type of options"

9de38e4

This reverts commit a6b2382.

refactor: use Set instead of array

abb92d8

sajikix requested a review from JoshuaKGoldberg March 24, 2023 06:52

armano2 mentioned this pull request Mar 24, 2023

Website: [Playground] add ability to inspect/preview detailed fixer information's #6753

Open

2 tasks

JoshuaKGoldberg approved these changes Mar 24, 2023

View reviewed changes

JoshuaKGoldberg mentioned this pull request Mar 24, 2023

Configs: Apply changes to config presets for v6 #6759

Closed

2 tasks

JoshuaKGoldberg merged commit bc31078 into typescript-eslint:main Mar 24, 2023

github-actions bot locked as resolved and limited conversation to collaborators Apr 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sajikix commented Oct 3, 2022

nx-cloud bot commented Oct 3, 2022 •

edited

Loading

typescript-eslint bot commented Oct 3, 2022

netlify bot commented Oct 3, 2022 •

edited

Loading

JoshuaKGoldberg left a comment

codecov bot commented Oct 11, 2022 •

edited

Loading

sajikix commented Oct 16, 2022

JoshuaKGoldberg left a comment

sajikix commented Mar 7, 2023

nx-cloud bot commented Mar 22, 2023 •

edited

Loading

sajikix commented Mar 23, 2023

armano2 left a comment

JoshuaKGoldberg left a comment

JoshuaKGoldberg left a comment

sajikix commented Mar 24, 2023

JoshuaKGoldberg commented Mar 24, 2023

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

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

Conversation

sajikix commented Oct 3, 2022

PR Checklist

Overview

nx-cloud bot commented Oct 3, 2022 • edited Loading

☁️ Nx Cloud Report

typescript-eslint bot commented Oct 3, 2022

netlify bot commented Oct 3, 2022 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 11, 2022 • edited Loading

Codecov Report

sajikix commented Oct 16, 2022

1. use recursive ASTNode comparison

2. use removeRange()

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

sajikix commented Mar 7, 2023

nx-cloud bot commented Mar 22, 2023 • edited Loading

☁️ Nx Cloud Report

sajikix commented Mar 23, 2023

armano2 left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

sajikix commented Mar 24, 2023

JoshuaKGoldberg commented Mar 24, 2023

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

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

nx-cloud bot commented Oct 3, 2022 •

edited

Loading

netlify bot commented Oct 3, 2022 •

edited

Loading

codecov bot commented Oct 11, 2022 •

edited

Loading

nx-cloud bot commented Mar 22, 2023 •

edited

Loading