strict-boolean-expressions should not fire for logical operators used in a non-boolean context

[marcind]

Bug Report

• TSLint version: 5.7
• TypeScript version: 2.5.3
• Running TSLint via: CLI

TypeScript code being linted

// code snippet
function valueOrDefault(a?: string) {
  return a || "the default";
}

with tslint.json configuration:

{
  "defaultSeverity": "error",
  "extends": [
      "tslint:all"
  ]
}

Actual behavior

Getting report of 2 errors:
strict-boolean-expressions This type is not allowed in the operand for the '||' operator because it could be undefined. Only booleans are allowed.
strict-boolean-expressions This type is not allowed in the operand for the '||' operator because it is always truthy. Only booleans are allowed.

Expected behavior

No errors being reported. I'm using the logical || operator to provide a default value for the parameter (I know I could use a function parameter default declaration, the snippet is just to illustrate the general problem). There's no boolean in play anywhere: neither the input or the output of the expression.

[marcind]

Thoughts? Prayers? Suggestions on where/how to get started?

[IllusionMH]

@marcind can you provide examples on how it should behave differently from turning of the rule?

[marcind]

Sorry, which part my initial submission is unclear?

I guess my point is that this rule should only run in a "boolean context", I can think of two cases:

1. Used in an if, while, or for
2. Assigned to a variable typed as boolean.

The rule should not run when I'm trying to provide a default value or short-circuit evaluation

1. const a: string = potentiallyUndefinedString || "the default";
2. const a: string | undefined = potentiallyUndefinedObject && potentiallyUndefinedObject.getString()

[ajafff]

@marcind the examples from your last post would work if you use the "allow-undefined-union" option.

The other part of your request should be a separate rule named strict-boolean-conditions which only checks if, for, while, do ... while and conditional expressions (x ? y : z).
I can imagine it would only check the type of the whole condition and not it's constituents:

function foo(a: boolean, b?: boolean) {
    if (b || a) {} // passes, result is always boolean
    if (b && a) {} // fails, result is boolean | undefined
    if (a || b) {} // fails, result is boolean | undefined
    if (a || !!b) {} // passes
}

Maybe this can be an option for the existing rule instead of a new rule...

[whatisaphone]

In my opinion, strict-boolean-expressions should only check the left operand of && and ||. Those operators are (essentially) sugar for ternaries: a && b is equal to a ? b : a, and a || b is equal to a ? a : b. When you think about it in those terms, ignoring the RHS makes a lot of sense, and it would bring the behavior for the short-circuiting operators in line with the behavior for ternaries.

Then, if the whole thing is inside an if/for/while, strict-boolean-conditions could come into play and check the overall expression. I think this would cover all the useful cases, and I'd be able to turn this rule back on.

The code that flagged this as a problem for me was this documented common React pattern:

function Foo(props: { showToggle: boolean }) {
  return <div>{props.showToggle && <Toggle />}</div>;
}

ERROR: 2:36 strict-boolean-expressions This type is not allowed in the operand for the '&&' operator because it is always truthy. Allowed types are boolean, null-union, undefined-union, string, or number.

---

@ajafff His code doesn't pass the linter for me, even with all the rule's options enabled, because of the constant RHS:

export function valueOrDefault(a?: string) {
  return a || "the default";
}

"strict-boolean-expressions": [true, "allow-null-union", "allow-undefined-union", "allow-string", "allow-number", "allow-mix"]

ERROR: 2:15 strict-boolean-expressions This type is not allowed in the operand for the '||' operator because it is always truthy. Allowed types are boolean, null-union, undefined-union, string, or number.

The intent of the expression is a conditional on the LHS, so my proposal of ignoring the RHS would solve his problem as well.

[ajafff]

I agree that checking the RHS of && and || is a bug.

---

I'm also inclined to relax the checks for the LHS of &&, || and the operand of !. These should only be checked whether the expression is always truthy or always falsy.
That leaves only the conditions of if, for, while, do ... while and conditional expressions for the strict checking to allow only booleans (or whatever else is configured). Thoughts @adidahiya?

[rhys-vdw]

Currently enabling this rule with available configuration options would necessitate days of work with our code base. :(

I enabled it hoping it would prevent implicit conversion of number, null or undefined into boolean. For example:

if (!!array.length) { /* ... */ }

<div>
  {array.length && <div />}
</div>

if (possiblyNull) {
  callFunction(possiblyNull)
}

However I find the following uses acceptable:

const x = possiblyUndefined || 5; // commented below, this was a mistake

<div>
  {array.length > 0 && <div />}
</div>

I think this agrees with what @marcind suggests.

I'm also inclined to relax the checks for the LHS of &&, || and the operand of !.

@ajafff would this legalize if (!!array.length) or if (!possiblyNull)?

[unindented]

@ajafff @adidahiya any further thoughts here? I'm in the exact same situation as @rhys-vdw.

[ajafff]

Shamelessly advertising my own project:
Since my last activity in this issue I created my own linter, the Fimbullinter project. Read the docs for a quick start: https://github.com/fimbullinter/wotan/blob/master/packages/wotan/README.md

It contains a rule no-useless-predicate, which might be what you are looking for. It's a combination of TSLint's strict-type-predicates and strict-boolean-expressions, but with a few major differences:

In contrast to TSLint's strict-type-predicates is also works without --strictNullChecks and correctly handles type parameters and empty object types. (probably not so interesting for people subscribed to this issue)
The main difference to TSLint's strict-boolean-expressions is that it doesn't require everything to be a boolean (it doesn't detect implicit coercion). It just requires every condition to be possibly truthy and falsy.

Some examples:

if (0) {} // error, always falsy
if (1) {} // error, always truthy
declare let array: string[];
if (array.length) {} // no error
if (!array.length) {} // no error
if (!!array.length) {} // no error
if (array.length === undefined) {} // error, condition is always false
if (!!false) {} // 2 errors, because of the double negation of an always falsy value

declare let someString: string;
return someString || 'some default string'; // no error, because 'someString' might be falsy

declare const foo: 'bar' | 'baz';
return foo || 'bas'; // error, 'foo' is always truthy

declare let optionalFunction: (() => void) | undefined;
optionalFunction && optionalFunction(); // no error

[ghost]

I'd like to chime in with the following use case from my actual code:

export interface ILoggingRule {
    readonly isFinal?: boolean;
    readonly loggerNamePattern?: string;
    readonly maxLogLevel?: LogLevel;
    readonly minLogLevel?: LogLevel;
    readonly target: Target;
}

// ...

/**
 * Creates an instance of LoggingRule.
 * @param options Configuration options of the logging rule.
 */
public constructor(options: ILoggingRule) {
    // tslint:disable:strict-boolean-expressions
    this.isFinal = options.isFinal || false;
    this.loggerNamePattern = options.loggerNamePattern || "*";
    this.maxLogLevel = options.maxLogLevel || LogLevel.Fatal;
    this.minLogLevel = options.minLogLevel || LogLevel.Trace;
    this.target = options.target;
    // tslint:enable:strict-boolean-expressions
}

As you can see I am using || to denote default values if the LHS happens to be undefined. Is there any concise way that makes tslint understand this does adhere to strict-boolean-expressions? Otherwise I am forced to use those disable/enable commands. (Surprisingly this is only an issue with type-checking enabled and not in VSCode.)

[StephenWeatherford]

This usage is standard and extremely common and convenient in JavaScript, and should definitely not trigger this rule to fail.

[krishan]

IMHO the RHS of && and || should not be checked, to allow for short-circuiting. This should be the default behavior (or at least there should be an option like allow-any-rhs). Currently, I cannot use strict-boolean-expressions, since there's a lot of short-circuiting in our code-base.

Checking the LHS should remain as-is IMHO, since using string or number as LHS can be dangerous, see this example:

function foo(x: string | number | null) {
  return x || defaultValue;
}

This is dangerous since many developers will fail to understand that not just null but also 0 and "" will cause defaultValue to be returned.

I have had to deal with major bugs in my project, due to developers failing to take into consideration that "" and 0 are also falsey. Preventing this kind of code, is my primary use-case for strict-boolean-expressions.

[dobesv]

Fixed in #4159 just need a review from a collaborator.

[rhys-vdw]

Oops, just wanted to clarify that one of my examples was bad:

const x = possiblyUndefined || 5;

That shouldn't work because possiblyUndefined might be 0 which could be a logic error. Looks like @dobesv's PR would work here.

[adidahiya]

fixed by #4159, will be available in the next release