[prefer-nullish-coalescing] Auto-fixer generates incorrect code with optional boolean values

[astorije]

Repro

{
  "rules": {
    "@typescript-eslint/prefer-nullish-coalescing": "error"
  }
}

function before(a: boolean | undefined, b: boolean, c: boolean) {
  return a || b || c;
}

Expected Result

Running the code above on all combinations of values:

> before(true, true, true);
true
> before(false, true, true);
true
> before(undefined, true, true);
true
> before(true, false, true);
true
> before(false, false, true);
true
> before(undefined, false, true);
true
> before(true, true, false);
true
> before(false, true, false);
true
> before(undefined, true, false);
true
> before(true, false, false);
true
> before(false, false, false);
false
> before(undefined, false, false);
false

Actual Result

When running the auto-fixer with this rule enabled, the following code gets generated:

function after(a: boolean | undefined, b: boolean, c: boolean) {
  return (a ?? b) || c;
}

Which, using the same values as above in the same order:

> after(true, true, true);
true
> after(false, true, true);
true
> after(undefined, true, true);
true
> after(true, false, true);
true
> after(false, false, true);
true
> after(undefined, false, true);
true
> after(true, true, false);
true
> after(false, true, false); // Different
false
> after(undefined, true, false);
true
> after(true, false, false);
true
> after(false, false, false);
false
> after(undefined, false, false);
false

So before(false, true, false); and after(false, true, false); return different values.

Versions

package	version
@typescript-eslint/eslint-plugin	2.24.0
@typescript-eslint/parser	2.24.0
TypeScript	3.8.3
ESLint	6.8.0
node	12.14.0
npm	6.13.4

[bradzacher]

Yeah, nullish booleans probably shouldn't be handled by the rule.
Happy to accept a PR to remove the fixer in that case.

[Lusito]

same thing happens with (empty) string. and (0) number as well. I gotta say, using the auto-fixer for this rule is currently dangerous.

[bradzacher]

Happy to accept a PR to remove the fixer in that case.

[greyscaled]

@bradzacher happy to take a crack this week

[greyscaled]

I proposed a change in 1910 and wrote up an explanation (see #1910 (comment)).

When a BooleanLiteral is the left operand of a logical binary expression, nullish coalescing should not be a suggested replacement as the logic diverges for false.

Consider the following example where a: boolean | undefined:

a	a || b
true	true
false	b
undefined	b

a	a ?? b
true	true
false	false
undefined	b

How Is This Different From Falsy Values

For other types such as string or number, a nullish coalescing will differ in logic from the bar bar operator ||:

// before
const s1 = ''
const s2 = s1 || "s2" // --> "s2"

// after
const s1 = ""
const s2 = s1 ?? "s2" // --> ""

This intention is expressed in the documentation https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md

However, that's an intended use-case as the falsy value (ex: empty string) is not a unioned type, but rather a value (ie: "" IN (string | undefined)). On the other hand, false in a nullish boolean is part of the type union: (ie: true | false | undefined) (see microsoft/TypeScript#14668 (comment)).

I'm duplicating that comment here because I'm hoping to generate discussion to clarify the intention of this rule and whether or not the change proposed in 1910 is safe/correct. The proposed change is to ignore checking a LogicalExpression whereby the left operand is a BooleanLiteral

[bradzacher]

It's a difficult thing to get right.

When I wrote this rule, I was excited about the new operator finally coming to TS. I wanted to give people a way to help migrate their codebase to it, and gain the benefits of safer separation of nullish and empty cases.
In my implementation, I made a naive assumption that the auto-fixer would be fine as people would review and test their changes carefully. But that is obviously not the correct approach. Many people trust that the lint rule is right, apply the fixer and move on - introducing regressions in their codebase.

The simple short-term fix is to leave rule's logic as-is, but:

• completely remove the fixer in all cases except for the non-falsey | nullish case.
  • this would remove immediate unsafety from the rule.
• rewrite the report messages in all cases except for non-falsey | nullish to warn about the implications of manually changing from || to ??.

---

The long-term fix is to re-evaluate the intention of the rule, and question whether or not it should even exist in the first place.

There's a lot of overlap with this rule and strict-boolean-expressions. With the rewrite of that rule (#1515), it will be much more configurable, and thus a more usable rule for any codebase.
The more I look at what this rule does, the more I see how it aligns with the new vision for that rule (#1515 (comment)).

So in the long term I see two paths forward:

• Deprecate this rule, remove it in 3.0, and build its functionality as suggestion fixers in strict-boolean-expressions.
• Keep the rule, but rewrite the options completely to mostly mirror those proposed in [strict-boolean-expression] Rework options #1515 (comment)

My preference is the first option, as it saves people double-configuring things, and seems like a natural extension to strict-boolean-expressions.

[greyscaled]

Sorry everyone, I was very busy for a while then away - I want to get a new PR for this change ASAP - I see that it's apart of the upcoming release.

@bradzacher May need some help jumping back into this

---

Did 52b6085 cover this?

[bradzacher]

In terms of the short-term fixes - the automatic fixer has been completely removed in v3.0.0 (52b6085), and it now is always a suggestion fixer.

I think we can close this and at some point in the future look to taking action on the long term fix I mentioned above.