Bug: [no-unnecessary-condition] does not work properly with void

[Zamiell]

Before You File a Bug Report Please Confirm You Have Done The Following...

• I have tried restarting my IDE and the issue persists.
• I have updated to the latest version of the packages.
• I have searched for related issues and found none that matched my issue.
• I have read the FAQ and my problem is not listed.

Playground Link

https://typescript-eslint.io/play/#ts=4.7.2&sourceType=module&code=CYUwxgNghgTiAEAzArgOzAFwJYHtVJxwAoBKALnlWQFsAjEGeAH3gDcctgBuAWACh+YPAGcMBHACUQw5BDEBecaV58hqUfDgZkMVCGABBAHI16jRYkJSZc+PPvw0oRFj3d+-IA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6Jge1tiacTJTIAhtEK0yHJgBNK+SpPRQA7iKaRwYAL4htQA&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylBm3TgwAXxCSgA

[bradzacher]

This is a bug in the rule - though as per #5255 - you really should avoid using | void

[Zamiell]

you really should avoid using | void

If the consistent-return ESLint rule is turned on, I'm not quite sure I understand the rational for no-invalid-void-type.

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;
}

[bradzacher]

With TS if you turn on noImplicitReturns then you don't need consistent-return as TS will enforce the same thing, but do it in a type-aware way.

If you start explicitly unioning void in your return types instead of undefined, however, TS won't be able to help you because it accepts that void needs no return.

This is why you should use the rule - you should not use void unless it's by itself in a return type location.

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
}

repl

[Zamiell]

Thanks Brad, that makes a lot of sense.

Regarding your final example, I use Airbnb rules, which mandates the default-case ESLint rule. So I typically use a ensureAllCases there:

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 default-case rule entirely since it sucks in TypeScript. (And I'd be open to that, although I have historically found the rule quite useful in situations where something isn't being returned from the switch statement.)

Furthermore, in my notes, I have written down that consistent-return catches more bugs and is more exhaustive than noImplicitReturns. But I don't recall specifically why I wrote this. Do you know of any cases where this is true?

For now, I'll try turning the rule off and see how things go. :)

[bradzacher]

I imagine that you might counter that I should turn off the default-case rule entirely since it sucks in TypeScript.

Yeah, exactly correct - default-case is another example of a generally "bad" rule for TS code because again it's not type-aware; so it doesn't understand that a switch can be exhaustive without the need for a default. Our switch-exhaustiveness-check is a better type-aware alternative in that regard.

There are some cases where it can help enforce good code; like if you're switching on a non-union type:

declare const someString: string;
switch (someString) {
  case 'someValue1': { ... }
  case 'someValue2': { ... }
  // ...
  // really should have a default to handle the 'other' case
}

But from experience switching on a pure string/number is pretty rare to to in good TS code because if you're writing out cases then you already have the basis for a union type!

---

Furthermore, in my notes, I have written down that consistent-return catches more bugs and is more exhaustive than noImplicitReturns. But I don't recall specifically why I wrote this. Do you know of any cases where this is true?

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!

[Zamiell]

Brad,

Thanks for the reply. I've turned off consistent-return, but I've noticed that the following erroneous pattern is now undetected:

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 consistent-return rule would force me to change the return; line to return undefined;, which makes sense - all the paths returning out of the function should be the same, and exactly match what the return type is.

In this situation, I am stuck, because I can't turn consistent-return back on without ruining the foo5 case that you outlined in your previous reply. Is there some separate lint rule other than consistent-return that I should use to handle this?

[bradzacher]

I personally don't see the difference between the two - either way they both result in undefined being returned from the function.
In general I'd personally suggest the code style of never writing undefined out in your code, ever and treat undefined as a system-only value.

In which case I'd leverage this FAQ to ban return undefined; altogether in favour of just return; (You could also enforce the inverse with the same rule)