fix(eslint-plugin): [no-unnecessary-type-parameters] check mapped alias type arguments #9741
Conversation
…as type arguments
|
Thanks for the PR, @JoshuaKGoldberg! 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. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
☁️ Nx Cloud ReportCI is running/has finished running commands for commit cc6563c. 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 targetsSent with 💌 from NxCloud. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9741 +/- ##
==========================================
+ Coverage 87.93% 88.02% +0.09%
==========================================
Files 403 406 +3
Lines 13796 13869 +73
Branches 4022 4052 +30
==========================================
+ Hits 12131 12208 +77
+ Misses 1358 1352 -6
- Partials 307 309 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
I don't think this is the right way to handle this case.
I pulled this change down locally and set a breakpoint on line 292 to see which tests hit the new code path. This was the first one:
> type
"Extract<AccessorPropertyComputedName, { type: NodeType; }>"
> type.aliasSymbol.declarations[0].getText()
'type Extract<T, U> = T extends U ? T : never;'
> > type.aliasTypeArguments.map(t => type.checker.typeToString(t))
[ 'AccessorPropertyComputedName', '{ type: NodeType; }' ]
> checkType
AccessorPropertyComputedName
> extendsType
{ type: NodeType; }
It's pretty weird that TS "inlines" type aliases that are conditional types like this. But since the same types appear in both type.aliasTypeArguments as type.checkType and type.extendsType, this change means we'll start double-counting them.
#9721 shows that we need the recursive call via aliasTypeArguments. So in the case that a type is both a type alias instantiation and a conditional type, I think it's safer to only treat it as a type alias instantiation and ignore the extra information.
In practice, that just means moving the last "catch-all" higher up. This lets us remove some duplicated logic. Here's a commit that does that, all tests still pass:
danvk@bacd28d
|
Oh awesome! @danvk do you have bandwidth to send a PR? I can add that better change to this PR if not. |
Sure, here's a PR to merge that commit into your PR branch JoshuaKGoldberg#323 |
handle type aliases higher up
94f7c99
into
typescript-eslint:main
PR Checklist
Overview
I'm not 100% on my understanding here. But what I think is happening is that TypeScript is inlining the resolved parts of the conditional type when possible.
type.aliasTypeArgumentsis the piece that still has a reference to the type arguments<T, S>.💖