feat(eslint-plugin): add new rule no-extraneous-return-types
#10464
Conversation
|
Thanks for the PR, @ronami! 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. |
|
View your CI Pipeline Execution ↗ for commit 7440e51.
☁️ Nx Cloud last updated this comment at |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10464 +/- ##
==========================================
+ Coverage 86.76% 86.98% +0.21%
==========================================
Files 444 446 +2
Lines 15313 15607 +294
Branches 4459 4547 +88
==========================================
+ Hits 13287 13576 +289
- Misses 1672 1675 +3
- Partials 354 356 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| docs: { | ||
| description: 'Disallow extraneous union types in return type annotations', | ||
| requiresTypeChecking: true, | ||
| }, |
There was a problem hiding this comment.
[Feature] Let's put this in strict? Your note that it found 7 valid reports is encouraging. Dogfooding it on our own code will be good IMO.
There was a problem hiding this comment.
Yes, I'd even consider having such a rule in recommended. Won't this be a breaking change though (and should be done in a new major)?
| | TSESTree.FunctionDeclaration | ||
| | TSESTree.FunctionExpression, | ||
| ): void { | ||
| if (!node.returnType || node.generator) { |
There was a problem hiding this comment.
|| node.generator
[Bug] Generators are indeed wacky at times, but not always. As a user I'd think it a bug that the rule doesn't flag these:
function* test(): Generator<number | string> {
return 1;
}function* test(): Generator<never, number | string, unknown> {
return 1;
}There was a problem hiding this comment.
I've added support Generators (and also AsyncGenerators 🙈). There are some different edge cases with them.
One issue I'll be happy to hear opinions on is a generator that has no yield statements but does have a generator yield return type:
// the following:
function* foo(): Generator<string> {}
// is auto-fixed into:
function* foo(): Generator {}Also, ones that have no yield statements but does have an explicit generator return type:
// the following:
function* foo(): Generator<string, number> {
return 1;
}
// is auto-fixed into:
function* foo(): Generator<unknown, number> {
return 1;
}There was a problem hiding this comment.
The rule doesn't include a suggestion or an auto-fix. I think an auto-fix is correct here, but I'll be happy to hear some opinions.
[Feature] I'd start with an auto-fix. Thinking out loud:
- This is purely in the type system so we're not going to cause runtime error
- Any actual necessary type shouldn't be reported on, so an invalid fix would be a bug in the rule already
There was a problem hiding this comment.
I've added an auto-fix 👍
One place I'm not entirely sure how to handle though, is the following:
function test1(): string {
return 1;
}
function test2(): boolean | string {
return 1;
}In both of those cases, the return type is, technically, unused. There's a type error, but it's unused. I chose to report these return types as unused, but not apply an auto-fix. Wdyt about this? Should these cases be reported? Potentially with a different error message?
| return; | ||
| } | ||
|
|
||
| const actualReturnTypes = getPotentialReturnTypes(node); |
There was a problem hiding this comment.
[Style] This confused me at first 😄. Is there a reason to name one "actual" and the other "potential"?
If not, request: use the same term for both?
After reading through it, "actual" matches how I'm understanding it. So +1 from me on that one.
| node: TSESTree.TypeNode, | ||
| returnTypes: ts.Type[], | ||
| allowUnusedVoidOrUndefinedValues = false, | ||
| ): void { |
There was a problem hiding this comment.
[Style] This is totally optional and if you don't like it I won't be upset at all 😄, but: I found this function to be a lot to read through. It's essentially a set of individual cases to check: tuple types, promise type argument, array type argument, miscellaneous. It might be clearer to split it up?
function checkReturnType(...) {
const tupleElementTypes = getTypeArgumentIfTupleNode(node);
if (tupleElementTypes) {
checkTupleElementReturnTypes(...);
return;
}
const promiseTypeArgument = getTypeArgumentIfPromiseNode(node);
if (promiseTypeArgument) {
checkPromiseTypeArgumentReturnTypes(...);
return;
}
// (similar const/if/check/return for array type arguments)
// (similar if/check/return for union types)
// ...miscellaneous logic
}If you don't like this, not a problem at all - just a personal preference of mine.
| const services = getParserServices(context); | ||
| const checker = services.program.getTypeChecker(); | ||
|
|
||
| function getRelevantReturnTypes( |
There was a problem hiding this comment.
[Style] Nit: this function both gets and checks. It took me a re-read to get that it does both. WDYT about expanding its name to mention both?
| function getRelevantReturnTypes( | |
| function getRelevantReturnTypesOrReport( |
packages/eslint-plugin/docs/rules/no-extraneous-return-types.mdx
Outdated
Show resolved
Hide resolved
|
|
||
| ## When Not To Use It | ||
|
|
||
| This rule can be completely redundant for projects without explicit return type annotations on functions. It works best when coupled with other rules that enforce explicit return type annotations such as [`explicit-function-return-type`](https://typescript-eslint.io/rules/explicit-function-return-type) or [`explicit-module-boundary-types`](https://typescript-eslint.io/rules/explicit-module-boundary-types). |
There was a problem hiding this comment.
[Docs] I think this accidentally implies that rule is only useful with those functions. Which is not true, I've found myself occasionally wanting it in projects that don't use those rules or isolatedDeclarations. Even though I hate unnecessary types I still sometimes use them to make functions more clear or get better return types.
I think this should instead note the actual edge cases that make the rule less useful. Which are ... not that many that I can think of. Maybe:
- If the functions are early stage and will likely be evolved/widened later
- If you're explicitly trying to match some external, more-permissive interface
...along with the standard notes about considering using inline disables in those cases.
There was a problem hiding this comment.
Yes, I agree.
The current explanation of This rule can be completely redundant for projects... is just wrong (and may make incoming developers miss a lot of the value of such a rule). I usually omit return type annotation in projects, and I still have dozens of functions I choose to annotate.
I updated this section, let me know what you think 👍
packages/eslint-plugin/docs/rules/no-extraneous-return-types.mdx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
The implementation includes special treatment for opening promises (also async functions), arrays, and tuples. Additional ones can be added (e.g.
Set,Map, and potentially object-literal fields)
[Feature] How about ... any generic shape altogether? It's kind of limiting to not be able to check user-defined classes:
class Box<T> {
constructor(public readonly value: T) {}
}
function makeBox(): Box<number | string> {
return new Box("abc");
}How difficult / edge-case-y would that be?
There was a problem hiding this comment.
That's brilliant! It was rather simple to do (though I still need to check I haven't missed any edge cases), thanks!
There was a problem hiding this comment.
Okay, this is significantly more complex than I initially considered. Summarizing my thoughts about this.
While the example above can be tackled easily enough, for some boxed values, it would be difficult to tell which return statements match:
// matching an object literal against a class type
class BoxClass<T> {
constructor(public readonly value: T) {}
}
function test1(): BoxClass<number | string> {
return { value: 'abc' };
}
// matching against a non-class boxed type
interface BoxType<T> {
value: T;
}
function test2(): BoxType<number | string> {
return { value: 'abc' };
}
// type that creates nested boxes
type NestedBox<T> = BoxClass<BoxClass<T>>;
function test3(): NestedBox<number | string> {
return new BoxClass(new BoxClass(10));
}
// types that make computations rather than create boxed values
function test4(): ReturnType<(() => number) | (() => string)> {
return 10;
}I think that as long as the rule doesn't warn unnecessarily, it's OK for it to not be able to catch all of these cases. Some of these cases are missed by similar comparisons (playground link).
Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
|
One issue I noticed, which I'm not sure if the rule should special case, is the following (I noticed this on tseslint's code-base): // noUncheckedIndexedAccess turned off
function foo(a: string[]): string | null {
return a[0] ?? null;
}The type of this expression is Should it be special-cased by this rule too? |
I feel like every 3 months or so I have an eye-opening revelation about how bizarrely TS can break my assumptions, and this is one such case. Coincidentally I was just looking at a very similar situation in microsoft/TypeScript#60813
I'm begrudgingly in favor of trying to special case this... One could imagine recursing on ternary, nullish coalescing, possibly even logical |
|
🤔 I feel like the allowing basic array indexing false positives isn't as good as it used to be. #1534 (January-March 2020) predates TypeScript 4.1 (November 2020). Now that we have |
Anecdotal and it's been a while since I tried it, but I found |
|
Ha, yeah, same. I've tried the flag repeatedly and never ended up liking it. |
|
The corollary though is that then we "should" flag this code too (depending on tsconfig settings) function foo(): string | null {
if (true) {
return ""
} else {
return null;
}
}because this is valid TS function foo(): string {
if (true) {
return ""
} else {
return null;
}
}(playground - sometimes this doesn't work right... make sure |
|
I think it makes more sense for us to check what we can syntactically determine may be returned, even if TS doesn't actually use that strategy (yet?) in its actual return type validation. |
|
I'm having a hard time finding time to finish working on this, I hope I can get back to this soon, but I'm closing this PR until I do. If anyone is reading this and wants to tackle this, please feel free to use this as much as you want ❤️ |
PR Checklist
Overview
This PR attempts to tackle #6442 and add a new rule to report on unused return type annotations:
Some of my thoughts:
The implementation takes the approach described here, and the rule warns on inline union types but not on referenced ones. This is important since referenced union types may be harder to fix and maintain.
Consider the following example:
The function may only be returning a couple of the possible types in the union type and not all of them. Returning a base type is convenient, and having the function to declare each type it may return is tedious, especially in cases with large union types (e.g.
TSESTree.Node).Another example is using a type exposed by a third-party library that is usually used contextually, but if the function is extracted, it may have an explicit return type annotation (vite plugins come to mind):
In my opinion, asking functions to declare exactly the type they're returning can be tedious, especially if some of these types come from a third party and aren't exposed (an example that comes to mind is
React.ReactNode).The implementation includes special treatment for opening promises (also
asyncfunctions), arrays, and tuples. Additional ones can be added (e.g.Set,Map, and potentially object-literal fields):The rule doesn't include a suggestion or an auto-fix. I think an auto-fix is correct here, but I'll be happy to hear some opinions.
Sometimes, not all code paths return a value on a function. Because of this, the rule will not report
voidorundefinedbeing potentially unnecessary on the return type annotation:Running this on
typescript-eslint's code base shows what seems like 7 valid reports (some show wrong types, some highlight a problematic implementation): 1, 2, 3, 4, 5, 6, 7.I think it may be helpful to have the option to ignore module boundaries (e.g. exported functions or public methods of exported classes/objects). It's convenient to sometimes have unused return types, so calling code will need to handle potential future return types.
When the rule reports a failure, it stringifies the name of the redundant type:
Unused or redundant union type '{{type}}' in return type.. While this works well for flat unions (e.g.string | number | boolean), I'm not sure how clear this is when the rule fails on a nested box like a promise or an array:Array<number | string | boolean>. Wdyt? Should the message be different for box (and potentially nested box) values?🚀