fix(eslint-plugin): [no-duplicate-type-constituents] shouldn't report on error types #9600

y-hsgw · 2024-07-20T04:22:15Z

PR Checklist

Addresses an existing open issue: fixes Bug: [no-duplicate-type-constituents] Shouldn't report on error types #9576
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

Fixed to not report errors.
Minor refactor.

typescript-eslint · 2024-07-20T04:22:19Z

Thanks for the PR, @y-hsgw!

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.

netlify · 2024-07-20T04:22:36Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`e26563d`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/66a729ae6ce4ab00092fcf29
😎 Deploy Preview	https://deploy-preview-9600--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 100 (🟢 up 1 from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 90 (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 your Netlify site configuration.

nx-cloud · 2024-07-20T04:23:28Z

☁️ Nx Cloud Report

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

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

codecov · 2024-07-20T07:47:48Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.45%. Comparing base (6b92aa5) to head (e26563d).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #9600   +/-   ##
=======================================
  Coverage   88.45%   88.45%           
=======================================
  Files         422      422           
  Lines       14695    14697    +2     
  Branches     4298     4299    +1     
=======================================
+ Hits        12998    13000    +2     
  Misses       1372     1372           
  Partials      325      325

Flag	Coverage Δ
unittest	`88.45% <100.00%> (+<0.01%)`	⬆️

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

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

auvred · 2024-07-28T13:00:59Z

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

-    if (
-      actualNodeKeys.some(
-        actualNodeKey =>
-          !isSameAstNode(
-            actualNode[actualNodeKey as keyof typeof actualNode],
-            expectedNode[actualNodeKey as keyof typeof expectedNode],
-          ),
-      )
-    ) {
-      return false;
-    }
-    return true;
+    return !actualNodeKeys.some(
+      actualNodeKey =>
+        !isSameAstNode(
+          actualNode[actualNodeKey as keyof typeof actualNode],
+          expectedNode[actualNodeKey as keyof typeof expectedNode],
+        ),
+    );


These changes don't seem to be related to the issue this PR solves?

I thought it was minor, so I went ahead and fixed it.
However, as you mentioned, it's unrelated to this PR, so I reverted it.
1560c83

auvred · 2024-07-28T13:04:36Z

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

+          const constituentNodeType = checker.getTypeAtLocation(
+            parserServices.esTreeNodeToTSNodeMap.get(constituentNode),
+          );


Suggested change

const constituentNodeType = checker.getTypeAtLocation(

parserServices.esTreeNodeToTSNodeMap.get(constituentNode),

);

const constituentNodeType = parserServices.getTypeAtLocation(constituentNode);

ParserServices has a nice utility function to do this :)

Nice! Thank you :) Fixed in fc5d5d8.

auvred · 2024-07-28T13:15:45Z

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

+          if (tsutils.isIntrinsicErrorType(constituentNodeType)) {
+            return [];
+          }


Suggested change

if (tsutils.isIntrinsicErrorType(constituentNodeType)) {

return [];

}

if (tsutils.isIntrinsicErrorType(constituentNodeType)) {

return uniqueConstituents;

}

As I understand this logic, uniqueConstituents is a kind of cache for finding duplicates using only AST (since it's faster than calling a checker to make type-level comparisons). See this comment #5728 (comment)

If we return an empty array, we discard all previously found unique constituents and fallback to type-level checks, this may result in some performance penalty.

Thank you for the guidance. Fixed in 6683e36.

auvred · 2024-07-28T13:17:17Z

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

-    {
-      code: 'type T = Class<string> | Class<string>;',
-      output: `type T = Class<string>  ;`,
-      errors: [
-        {
-          messageId: 'duplicate',
-          data: {
-            type: 'Union',
-            previous: 'Class<string>',
-          },
-        },
-      ],
-    },


[Testing] If this case is no longer reported as an error, let's just move it to the valid section instead of deleting it?

Indeed, that's correct. I moved the tests in 3cf71ad.

auvred · 2024-07-28T13:20:35Z

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

[Testing] It would be great if we had some test cases (both valid and invalid) that mix error types with normal types

I added the tests in 3cf71ad.

…to fix/no-duplicate-type-constituents

auvred

🔥

JoshuaKGoldberg

Woop, thanks!

y-hsgw added 2 commits July 20, 2024 13:17

fix: suppress error messages

a8e3dec

refactor: simplify conditional statements

7b23caf

y-hsgw changed the title ~~fix(eslint-plugin): [no-duplicate-type-constituents] Shouldn't report on error types~~ fix(eslint-plugin): [no-duplicate-type-constituents] shouldn't report on error types Jul 20, 2024

fix: no-duplicate-type-constituents doc

8ec1db0

auvred requested changes Jul 28, 2024

View reviewed changes

auvred added the awaiting response Issues waiting for a reply from the OP or another party label Jul 28, 2024

y-hsgw added 5 commits July 29, 2024 14:22

fix: revert refactor

1560c83

refactor: use parserServices

fc5d5d8

perf: use uniqueConstituents

6683e36

test: add test

3cf71ad

Merge branch 'main' of https://github.com/y-hsgw/typescript-eslint in…

e26563d

…to fix/no-duplicate-type-constituents

y-hsgw requested a review from auvred July 29, 2024 05:47

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Jul 29, 2024

auvred approved these changes Jul 29, 2024

View reviewed changes

auvred added the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Jul 29, 2024

JoshuaKGoldberg approved these changes Jul 29, 2024

View reviewed changes

JoshuaKGoldberg merged commit 3591b78 into typescript-eslint:main Jul 29, 2024
62 of 63 checks passed

y-hsgw deleted the fix/no-duplicate-type-constituents branch July 29, 2024 16:34

github-actions bot locked as resolved and limited conversation to collaborators Aug 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(eslint-plugin): [no-duplicate-type-constituents] shouldn't report on error types #9600

fix(eslint-plugin): [no-duplicate-type-constituents] shouldn't report on error types #9600

y-hsgw commented Jul 20, 2024 •

edited

Loading

typescript-eslint bot commented Jul 20, 2024

netlify bot commented Jul 20, 2024 •

edited

Loading

nx-cloud bot commented Jul 20, 2024 •

edited

Loading

codecov bot commented Jul 20, 2024 •

edited

Loading

auvred Jul 28, 2024

y-hsgw Jul 29, 2024

auvred Jul 28, 2024

y-hsgw Jul 29, 2024

auvred Jul 28, 2024

y-hsgw Jul 29, 2024

auvred Jul 28, 2024

y-hsgw Jul 29, 2024

auvred Jul 28, 2024

y-hsgw Jul 29, 2024

auvred left a comment

JoshuaKGoldberg left a comment

fix(eslint-plugin): [no-duplicate-type-constituents] shouldn't report on error types #9600

fix(eslint-plugin): [no-duplicate-type-constituents] shouldn't report on error types #9600

Conversation

y-hsgw commented Jul 20, 2024 • edited Loading

PR Checklist

Overview

typescript-eslint bot commented Jul 20, 2024

netlify bot commented Jul 20, 2024 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

nx-cloud bot commented Jul 20, 2024 • edited Loading

☁️ Nx Cloud Report

codecov bot commented Jul 20, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

auvred left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

y-hsgw commented Jul 20, 2024 •

edited

Loading

netlify bot commented Jul 20, 2024 •

edited

Loading

nx-cloud bot commented Jul 20, 2024 •

edited

Loading

codecov bot commented Jul 20, 2024 •

edited

Loading