feat(eslint-plugin): [no-unnecessary-type-parameters] initial implementation #8173
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for the PR, @danvk! 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. |
danvk
changed the title
danvk
changed the title
Hmm, interesting. Might be a caching issue with Nx? If you can get a reliable repro for it I'd love to see an issue. I've had the occasional issue myself but haven't been able to pinpoint anything I can report... |
|
I am getting this error consistently. Here's my shell session: and here's the contents of the |
|
(A fresh clone worked, so there's definitely some sort of bad state here.) |
|
@danvk please can you see if running yarn nx reset and then rerunning the install resolves the issue? |
@JamesHenry Running |
|
I was able to build this and run it on some of my code! Here's the one false positive I found: type AllValues<T extends Record<PropertyKey, PropertyKey>> = {
[P in keyof T]: {key: P; value: T[P]};
}[keyof T];
type InvertResult<T extends Record<PropertyKey, PropertyKey>> = {} & {
[P in AllValues<T>['value']]: Extract<AllValues<T>, {value: P}>['key'];
};
declare function invert<T extends Record<PropertyKey, PropertyKey>>(obj: T): InvertResult<T>;This rule reports that |
Result of merging a few PRs into main.
JoshuaKGoldberg
removed
the
awaiting response
JoshuaKGoldberg
force-pushed
the
no-unnecessary-type-parameters
branch
from
78d6db9
to
a8f5c2e
Compare
JoshuaKGoldberg
force-pushed
the
no-unnecessary-type-parameters
branch
from
7ed4c77
to
c2384b3
Compare
danvk
commented
Apr 30, 2024
There was a problem hiding this comment.
Your tweet about carving out dedicated time to finish this inspired me to carve out some time to understand how it works. It's not what I expected! But I think it's fine for now. See my comments / questions. Some more clarifying comments would be helpful.
| if ( | ||
| reference.identifier.parent.type === AST_NODE_TYPES.TSTypeReference && | ||
| reference.identifier.parent.parent.type === | ||
| AST_NODE_TYPES.TSTypeParameterInstantiation && |
There was a problem hiding this comment.
This is a little more strict than necessary. It misses cases where the type parameter is used in a type expression that's passed as the type argument, for example Promise<T | null>. The slow path will still handle this correctly, but might be a nice optimization in the future.
| visitType(type, false); | ||
| } | ||
|
|
||
| function visitType(type: ts.Type | undefined, asRepeatedType: boolean): void { |
There was a problem hiding this comment.
Continuing the discussion from #8173
I spent some time stepping through this in the debugger trying to understand why asRepeatedType was necessary. The good news is that I get it now! The interesting news is that this rule works totally differently than how I expected. I think the way it's implemented is fine, but I think some clarifying comments and name changes could make it much clearer.
My main point of confusion was around what collectTypeParameterUsageCounts does, specifically what "recursively descending through type references" meant. I thought it meant that, when this function saw Set<T>, it would instantiate the type declaration for Set and see how many times T appeared in that. I don't think it does that, though! If it hits Fn<Expr of T>, then it will descend into Expr of T but not Fn. So asRepeatedType is a shortcut to make sure that Fn<T> always counts as a repeated use of T.
This isn't really correct since you could write code like this:
type Identity<T> = T;
declare function takeIdentity<T>(x: Identity<T>): void;This is not a good use of generics, but the lint rule will allow it because of asRepeatedType.
I think this is an OK concession to expediency. But I can imagine real world examples of this getting thrown off by general helper types, e.g.:
type NonNullish<T> = T & {};
declare function foo<T>(x: NonNullish<T>): void;So, a few comments / suggestions:
- How hard would it be to recurse into the type aliases, not just the their type arguments? If this is impossible, then that's OK, but it would be good to at least note that (and add my two examples as tests!).
- Clarify what is "recursively descended" into in the comment. Or maybe don't mention recursion at all, it's just walking the type equivalent of a syntax tree.
- I think using the term "count" in this function name and its parameter (
identifierCounts) is misleading since it doesn't really return a count. It kind of returns a lower bound, but not really because of theIdentify/NonNullishexamples above. I'm not sure what a better name would be, maybe just something generic liketypeParameterWorker? - For
asRepeatedType, maybe the name could beassumeMultipleUses? Or maybeasRepeatedTypeis fine, just add a comment where you set it totrue, e.g.:
// We don't know how many times the type argument is used in this type alias.
// Assume it's at least twice.
visitType(typeArgument, true);| // Generic type references like `Map<K, V>` | ||
| else if (tsutils.isTupleType(type) || tsutils.isTypeReference(type)) { | ||
| for (const typeArgument of type.typeArguments ?? []) { | ||
| visitType(typeArgument, true); |
There was a problem hiding this comment.
I understand the idea behind this for type aliases (see my long comment above) but I don't understand it for tuples. In fact, I think it might be wrong! This is considered valid code by this rule but it should not be:
function takeTuple<T>(x: [T]): void;
There was a problem hiding this comment.
Wouldn't array methods like map also count as references to T, though?
There was a problem hiding this comment.
Good point, I guess they would.
I do think it would be interesting to explore adding a list of "singular types" to this rule in the future. Array<T> does, in theory, define a complex set of relationships between methods involving T. But in practice, you probably just want to pass in a JS array. Same for Set<T>, Map<K, V> and I'm sure other types, too.
|
Question: what's the motivation for counting the references to |
@Josh-Cena see discussion about return types here: #8173 (review). Depending on |
JoshuaKGoldberg
added
the
team assigned
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Checklist
Overview
See linked issue. I've incorporated the tests from my blog post and all the existing rules. We sometimes (but not always) agree with them.
I'm pretty happy with how this turned out! The logic winds up being pretty straightforward: count the number of times each type parameter is referenced, whether explicitly or implicitly in an inferred return type. If that's exactly one, then report an issue.
One fun discovery: I always thought that this rule had some unavoidable false positives, e.g. my
bothfunction from cartant/eslint-plugin-etc#15. But looking at that again a few years later, I think it's a true positive that can be rewritten to avoid breaking the golden rule:❌ Incorrect
✅ Correct
🎉
Notes from Josh (April 2024): Dan and I chatted and I'm pitching in to help get this to review state. The design is actually more intricate than we'd initially hoped:
The logic is still straightforward: recursively descend into types, kind of like how scope analyzer does in AST-land. But the edge cases were quite tricky.
💖