-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Rule proposal: warn against unnecessary type parameters #3729
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
@danvk and I paired on an implementation today - it looks very doable! Especially encouraging given there are several re-implementations in the wild:
Assigning to myself to track that @danvk is planning on cleaning up the results of pairing a bit and sending a PR soon. Aside: per @mattpocock's There Is No Such Thing As A Generic, the rule name we tentatively settled on is |
I spent some more time looking at this code. I think I see a good path forward but I have a TS API question. I think the barebones implementation could be as simple as this: const usage = tsutils.collectVariableUsage(tsNode.getSourceFile());
for (const typeParameter of tsNode.typeParameters) {
const { uses } = usage.get(typeParameter.name)!;
if (uses.length === 1) {
context.report({
data: {
name: typeParameter.name.text,
},
node: parserServices.tsNodeToESTreeNodeMap.get(typeParameter),
messageId: 'sole',
});
}
} This is a very direct encoding of the rule: if a type parameter is only used once then it's bad. Unfortunately, the usage map returned by function getProperty<T, K extends keyof T>(obj: T, key: K) {
return obj[key];
} To get the inferred return type for that function I can do something like this: const checker = parserServices.program.getTypeChecker();
const type = checker.getTypeAtLocation(tsNode);
const r = type.getCallSignatures().map(s => s.getReturnType())[0]; That definitely has the information I need to find the missing usage: checker.typeToString(r) // 'T[K]'
tsNode.typeParameters[0] === r.objectType.symbol.declarations[0] // true So I'm wondering if there's a variation on Side note: why don't the wotan / eslint-plugin-etc rules have this issue? If I'm reading the implementations correctly, they're hard-wired to assume all type parameters are used if a function has an implementation ( |
I don't think such a thing exists, no. Maybe this is a good opportunity to add an option or fork into a more dedicated
This should be roughly doable with the scope manager.
...maybe they do have the issue? It'd be interesting to see a comparison of what edge cases are/n't caught by each rule... |
Yeah the only way to truly handle the case of an inferred return type is by using type information. But all other cases can be handled using our scope manager as Josh mentioned - no need to dip into the TS AST or tsutils for that! Question - are we just handling functions here? What about function types, method signatures and interface call signatures? |
It would be quite limited if the rule only considers explicitly written return types, as many developers rely on TypeScript's type inference and do not explicitly specify return types in their code. By including support for inferred return types, the rule would be able to accurately identify cases where type parameters are unnecessary and provide more comprehensive feedback to developers. This would greatly enhance the effectiveness and usefulness of the rule in real-world scenarios. |
With the upcoming But definitely I do understand why you'd want a type-aware rule. But it does ofc come with the drawback of the rule being less used. There's a trade-off to both approaches. |
That flag is not going to be widely used, IMO. Many people use |
I can see a lot of value in it for many reasons on any codebase experiencing slowness. With There's so much potential win here for most codebases to shave time off builds easily! Most companies will likely look to adopt this to improve build times and save CI $$$. |
Maybe I'm not in touch with this, but there should be nothing preventing people from building and using such tools today, maybe with another linter to report un-analyzable code (that relies on inference). Are there popular examples? |
There's nothing explicitly stopping - no. People could have devised their own set of rules and built their own transpilers to do this. But that is a heap of work that requires specialised knowledge so instead people have gone other more traditional routes like caching (see solutions like nx). Once the standard is encoded in TS itself, then transpilers like SWC can build support and then users can reap the benefits for free. |
I started down this road, check out This seems to work pretty well and I'm happy with all the passing tests. Since the One thing I could use some pointers on: in order to apply this rule to methods in classes, I had to add a pretty broad selector: Without that, the rule only runs for declare class C<V> {
method<T, U>(param: T): U;
prop: <P>() => P;
}
I think we're getting pretty close to having this rule ready. |
Ah, this would be the |
Adding this did get my rule to run but it wound up being a little too broad -- my rule wound up running twice on some method definitions. The narrower fix wound up being to add |
Adding this to our |
https://github.com/microsoft/dtslint/blob/master/docs/no-unnecessary-generics.md
The text was updated successfully, but these errors were encountered: