fix(eslint-plugin): [no-unnecessary-type-parameters] should parenthesize type in suggestion fixer if necessary

[y-hsgw]

PR Checklist

• Addresses an existing open issue: fixes Bug: [no-unnecessary-type-parameters] should parenthesize type in suggestion fixer if necessary #10894
• That issue was marked as accepting prs
• Steps in Contributing were taken

Overview

Fixed the bug described in the title.

Please note that in addition to the error examples in the #10894, I have also addressed several related cases.

🐛

[typescript-eslint]

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]

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	8086924
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/67dbc51561c4810008fd4248
😎 Deploy Preview	https://deploy-preview-10907--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 93 (🔴 down 6 from production) Accessibility: 100 (no change from production) Best Practices: 100 (no change from production) SEO: 98 (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]

View your CI Pipeline Execution ↗ for commit 8086924.

Command	Status	Duration	Result
nx test eslint-plugin	✅ Succeeded	7m 41s	View ↗
nx run eslint-plugin:test -- --coverage	✅ Succeeded	6m 10s	View ↗
nx test eslint-plugin --coverage=false	✅ Succeeded	6m 26s	View ↗
nx test scope-manager	✅ Succeeded	<1s	View ↗
nx run types:build	✅ Succeeded	<1s	View ↗
nx run-many --target=build --exclude website --...	✅ Succeeded	14s	View ↗
nx test rule-tester	✅ Succeeded	<1s	View ↗
nx test utils	✅ Succeeded	<1s	View ↗
Additional runs (25)	✅ Succeeded	...	View ↗

---

☁️ Nx Cloud last updated this comment at 2025-03-20 07:48:40 UTC

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.59%. Comparing base (3c7b8ff) to head (4cb399a).
Report is 33 commits behind head on main.

❗ Current head 4cb399a differs from pull request most recent head 8086924

Please upload reports for the commit 8086924 to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10907      +/-   ##
==========================================
+ Coverage   87.43%   87.59%   +0.15%     
==========================================
  Files         468      470       +2     
  Lines       16040    16101      +61     
  Branches     4649     4671      +22     
==========================================
+ Hits        14025    14103      +78     
+ Misses       1658     1642      -16     
+ Partials      357      356       -1     

Flag	Coverage Δ
unittest	87.59% <100.00%> (+0.15%)	⬆️

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

Files with missing lines	Coverage Δ
...plugin/src/rules/no-unnecessary-type-parameters.ts	92.44% <100.00%> (+0.27%)	⬆️

... and 16 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]

Thanks! 👍

Wait, was too optimistic - taking a closer look..

[JoshuaKGoldberg]

Good stuff! I think just a bit of extra testing & code flow trimming are needed now. 🔥

[JoshuaKGoldberg]

🚀 Looks great to me, thanks!

[kirkwaiblinger]

I think this logic isn't quite right, in that it's not general enough.... We really want (as close as we can) to answer the question - "is the destination context going to be higher precedence than the source?". This logic only identifies the special case of intersection/union being lower precedence than TSArrayType/TSIndexedAccessType, which is true, but there is much more nuance.

For example, TSIntersectionType is higher precedence than TSUnionType, so the following test case currently doesn't work correctly:

type A = string;
type B = string;
type C = string;

declare function f<T extends A | B>(): T & C;

// fixes to 
declare function f(): A | B & C;
// (which is equivalent to)
declare function f(): A | (B & C);

// should fix to 
declare function f(): (A | B) & C;

Another (quite perverse) test case that isn't currently handled correctly is

type A = string;
type B = string;
type C = string;
type D = string;

declare function f<T extends A extends B ? C : D>(): T | null;
// fixes to 
declare function f(): A extends B ? C : D | null;
// (which is equivalent to)
declare function f(): A extends B ? C : (D | null);

// should fix to 
declare function f(): (A extends B ? C : D) | null;

Note that it's not necessarily practical to get this 100% correct in all cases, so some extra "unnecessary" parens are totally acceptable here if it the implementation benefits. FWIW - prettier actually puts "unnecessary" parens anyway for the mixed union/intersection case, since it's just hard to read for us humans otherwise 🙃

Have you had a look at the implementations of getWrappingFixer and friends? I'd think they have some of this logic already implemented more generally?

[y-hsgw]

Thank you. I've added the missing cases and modified it to use getWrappingFixer.