[strict-boolean-expression] Rework options

[bradzacher]

Original comment by @phaux in #1480

Here are the cases that I encounter often and could have an option to allow them:

• boolean – always allowed
• boolean | null | undefined aka optional/nullable boolean – developers often want to treat the nullish case the same as false instead of explicitly saying value ?? false or value == null ? false : value
  • allowed by allowNullable
• string | number – developers often want to test for zero/empty string without explicit comparison value != 0 etc.
  • would be allowed by e.g. allowPrimitive or something
• object | function | null | undefined aka nullable object/function – developers often check against the nullish case without explicitly comparing to null. I've met a lot of people that don't even know that value == null also checks for undefined.
  • would be allowed by allowSafe if we can fix it, or allowNullableObject or something

---

We've learned a lot about this rule and how people want to use it. We should look into reworking the options so that it matches how people want to use the rule.

[bradzacher]

I see this rule as being analogous to flow's inbuilt sketchy-null lint rule.

So expanding on @phaux's cases, I see the following cases that we need to care about:

• boolean
• boolean | null | undefined
• string
• string | null | undefined
• number
• number | null | undefined
• object | function | null | undefined
• any

With those cases listed out, this is what I think should happen:

• boolean should just be allowed by the rule by default, obviously.
• string, number, and object | function | null | undefined should be behind separate options.
  • the options should all be default true.
  • these are all "safe" - and the intention is clear from the fact that there is no ambiguity in the meaning, as there is exactly 1 falsey state for each of them.
  • the reason they should be behind options is so that people can opt to enforce every single conditional is a boolean.
• boolean | null | undefined, string | null | undefined, number | null | undefined should be behind separate options.
  • the options should all be default false.
  • these are all "sketchy" (i.e. carry some risk), because each one has one truthy, and two falsey states.
• any should be behind a separate option.
  • the option should be default false.
  • this one is mega sketchy, because it has one truthy, and many falsey states.

I.e. I propose the following options

interface Options {
  // default = true
  allowString?: boolean;
  allowNumber?: boolean;
  allowNullableObject?: boolean;

  // default = false
  allowNullableBoolean?: boolean;
  allowNullableString?: boolean;
  allowNullableNumber?: boolean;
  allowAny?: boolean;
}

---

Adding in extra information about the old ignoreRHS option:

ignoreRHS was implemented as a way to let people use do sketchy checks in a short-circuit context const x = someBool && someNullishString;. The right-hand side of a short-circuit chain is not evaluated as a boolean, so there's no real need to apply a strict boolean check on it in this case.

HOWEVER, when this same code is used in the context of a place expecting a boolean like if (someBool && someNullishString) {}, then this should be checked by the rule, regardless of the ignoreRHS setting.

Currently, the rule option doesn't do that, which makes the rule a lot less safe.
The reworked rule should take this into account, and only evaluate the RHS logical expressions that are in boolean locations.

[phaux]

BTW with these options, we can end up with a logical expression where all the operands are allowed, but its resulting type is something we don't allow.

E.g. numVal && strVal && nullObjVal && boolVal would be of type 0 | "" | null | undefined | boolean and would still be reported if used inside of an if/while, etc.

[Retsam]

It's not mentioned on way or another in this issue, and it's not part of #1480; but I think the ignoreRHS option can probably be removed - it should be the same fundamental change as #1163 which removes the same option from no-unnecessary-condition:

Instead of always or never checking the right hand side of a boolean logical operator depending on the option, the right-hand side should be checked only when it's in a "conditional context":

// Non-conditional contexts - y isn't checked.
const a = x && y;
return x && y;

// Conditional contexts - y is checked
if(x && y) {}
const b = x && y && z;
// Special case (#1038)
[1,2,3].filter(x => x && y)

[bradzacher]

agree!

example of flow's sketchy-null check doing the same contextual check:

/* @flow */
// flowlint sketchy-null:error

declare var x: ?string;
if (x) {} // Error: Sketchy null check on string [1] which is potentially an empty string. Perhaps you meant to check for null or undefined [2]?

const y = true && x; // no error
if (true && x) {} // Error: Sketchy null check on string [1] which is potentially an empty string. Perhaps you meant to check for null or undefined [2]?

repl

[phaux]

So the logic should be:

• When checking conditional expression – recurse over its test expression if it's a logical binary expression and check only the outermost operands.
• When checking a logical expression whose parent is not a conditional – also recurse to the outermost operands but don't check the rightmost one.

In any case, the logical expression itself should not be checked, only it's operands.

Is that correct?

[phaux]

I was wondering if options like allow number/string should work in the left-hand side of logical expressions in a non-conditional context. If they do then it makes a foot gun which is very common in JSX for example:

<div>{quantity && <p>You have {quantity} items</p>}</div>

If we don't explicitly compare quantity != 0 then a literal zero would be rendered. It's like that in most JSX frameworks that I know and might be a problem in regular JS/TS too.

One way to solve this would be to specify the context for every "allow type" option, then the defaults would be:

{
  "allowNumber": "conditional",
  "allowString": "conditional",
  "allowNullableObject": "always", // this is pretty safe even in JSX
  "allowNullableBoolean": "never",
  ...
}

Or alternatively hardcode the string/number options to not work in non-conditional context.

---

That would disallow strOrNumVal || defaultVal and strOrNumVal && valIfTruthy which is a bad practice anyways.

[Retsam]

I think your description sounds mostly right, though I'm not sure about "recursing to the outermost operands": just to hopefully be clear here's a sketch of what the code in no-unnecessary-condition looks like:

function checkNode(node) {
    if(node.type === AST_NODE.LogicalExpression) {
         // only the rhs needs to be checked, the lhs is already checked
         return checkNode(node.right);
    }
    // report error if node is (unnecessary condition)/(non-strict boolean)
}

function checkLogicalExpression(node) {
    // only check the left when logical expression AST node is encountered
    checkNode(node.left);
}

return {
    IfStatement: (node) => checkNode(node.test),
    ForStatement: (node) => checkNode(node.test),
    LogicalExpression: checkLogicalExpression,
    WhileStatement: (node) => checkNode(node.test),
};

FWIW, I'm willing to implement this logic - might be easier for me, and would help the implementation to be consistent with no-unnecessary-condition; I just didn't want to create a bunch of merge conflicts by having two people making significant changes to this rule at the same time.

---

I don't think I understand the most recent comment, about "allowing number/string in the left-hand-side in a non-conditional context"; because in my usage of the term, the left hand side is always a conditional context.

[bradzacher]

An additional suggestion in #1785

literal string/number types type T = 'a' | 1 should probably be treated as "safe" always, as long as they are not type Unsafe = "" | 0

[phaux]

Can we do this after 3.0 in a separate PR? It wouldn't be a breaking change.

[ericbf]

In regards to treating literal types as safe, the way I see it, even falsey literal types are safe, as they have a distinct truthiness value. In other words, type Unsafe = "" | 0 are always falsey and type Safe = "a" | 1 are always truthy. The unsafety comes in with non-literal types like string or number which could be truthy or falsey based on the actual runtime value.

To illustrate, take if I have a variable declare x: "" | "hello": it would be safe to do something like if (x) { use "hello" } because there is no ambiguity there. x is literally of the type "hello" inside the if statement. However, if the type was just string, x would not be narrowed (from a type perspective) in the if, but "" would be filtered out implicitly. The key distinction is implicit versus explicit filtering. With type literals, the compiler will show readily that the falsey types are filtered out while the truthy are kept, making it "safe".

[markfields]

New here and having a wee bit of a hard time following all the nuances here. I'm coming from a .NET background, new-ish to TypeScript. What I think I want the linter to do is:

• prevent me from conflating undefined with falsy primitive values
• allow me to treat null or undefined values of non-primitive types as false (e.g. declaring foo?: IFoo it could be null or undefined).

e.g. boolean | undefined can't be used in boolean expressions, but IFoo | undefined can be.

Is that a reasonable point of view? Would that be supported in the new options for strict-boolean-expressions?

Also, I'm beginning to see that TypeScript's ?. and ?? are my friend and help with a number of cases, so I can almost live with the strictest form of the rule, except there's not a good workaround for the simple null check on an object if(!foo) { throw new Error("foo is undefined!"); }

[bradzacher]

I've summarised all of this discussion in the top comment:
#1515 (comment)

Short answer is yes.

[ericbf]

there's not a good workaround for the simple null check on an object if(!foo) { throw new Error("foo is undefined!"); }

Really if you're checking nullishness or undefinedness you'd do:

if (foo == null) {
    throw new Error("foo is null or undefined")
}

The double equals checks against both null and undefined.