-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Bug: [no-unnecessary-condition] does not work properly with void #5254
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
This is a bug in the rule - though as per #5255 - you really should avoid using |
If the For example: type FooReturnType: number | void;
function foo1(): FooReturnType{
// Do some stuff.
// In this function, I'm not returning anything, so I can avoid typing a return statement
// at all to keep things nice and clean.
}
function foo2(): FooReturnType {
if (someCondition) {
return 123;
}
// Base case - return nothing.
// - If I don't include a return statement, then "consistent-return" complains.
// - If I just include a "return" statement, then "consistent-return" complains.
// - Thus, I must explicitly use a "return undefined" statement.
return undefined;
} |
With TS if you turn on If you start explicitly unioning This is why you should use the rule - you should not use type BadFooReturnType = number | void;
function foo1(): BadFooReturnType {
// no return needed even though there is an expectation this function MIGHT return a number
}
function foo2(someCondition: boolean): BadFooReturnType {
if (someCondition) {
return 123;
}
// TS does not need a return even though we *want* consistent returns
// consistent-return errors
}
type GoodFooReturnType = number | undefined;
function foo3(): GoodFooReturnType {
// TS expects a return because we have a non-void return type
// consistent-return does not need a return
}
function foo4(someCondition: boolean): GoodFooReturnType {
if (someCondition) {
return 123;
}
// TS expects a return because we have a non-void return type
// consistent-return also expects a return
}
//
// This is where consistent-return sucks with TS code:
//
enum Test { A, B, C }
function foo5(arg: Test) {
switch (arg) {
case Test.A:
return 1;
case Test.B:
return 2;
case Test.C:
return 3;
}
// TS knows the switch is exhaustive and thus all branches of the function are covered
// consistent-return does not understand the switch is exhaustive and thus forces you
// to add an unnecessary, unreachable return
} |
Thanks Brad, that makes a lot of sense. Regarding your final example, I use Airbnb rules, which mandates the export const ensureAllCases = (obj: never): never => obj; Doing this obviates the needs for TS to make the return type exhaustive, since the switch becomes exhaustive. Which I guess would be considered worse? But since I have to make a default case anyway, it seems like a good idea. I imagine that you might counter that I should turn off the Furthermore, in my notes, I have written down that For now, I'll try turning the rule off and see how things go. :) |
Yeah, exactly correct - There are some cases where it can help enforce good code; like if you're declare const someString: string;
switch (someString) {
case 'someValue1': { ... }
case 'someValue2': { ... }
// ...
// really should have a default to handle the 'other' case
} But from experience
I don't - I'm sure there would be some because TS is driven by types but the lint rule is driven purely by syntax. Personally I've found TS to cover all the cases I want for codebase standardisation! |
Brad, Thanks for the reply. I've turned off function foo() { // Implicit return type
if (someCondition) {
return;
}
return undefined;
} And, very similarly: function foo(): undefined { // Explicit return type
if (someCondition) {
return;
}
return undefined;
} Here, either implicitly or explicitly, the return lines are heterogeneous. Previously, the In this situation, I am stuck, because I can't turn |
I personally don't see the difference between the two - either way they both result in In which case I'd leverage this FAQ to ban |
Before You File a Bug Report Please Confirm You Have Done The Following...
Playground Link
https://typescript-eslint.io/play/#ts=4.7.2&sourceType=module&code=CYUwxgNghgTiAEAzArgOzAFwJYHtVJxwAoBKALnlWQFsAjEGeAH3gDcctgBuAWACh+YPAGcMBHACUQw5BDEBecaV58hqUfDgZkMVCGABBAHI16jRYkJSZc+PPvw0oRFj3d+-IA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6Jge1tiacTJTIAhtEK0yHJgBNK+SpPRQA7iKaRwYAL4htQA&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylBm3TgwAXxCSgA
The text was updated successfully, but these errors were encountered: