[strict-boolean-expressions] Option to enable only in while, do-while, if and ternary

[astoilkov]

I am proposing an option to allow non-boolean expressions when outside if, while, do-while and ternary. I believe this is useful because it is way easier to read foo || bar than foo === undefined ? bar : foo and when there are more than two values things get complicated - foo || bar || baz.

Repro

{
  "rules": {
    "@typescript-eslint/strict-boolean-expressions": ["error"]
  }
}

const value = foo || bar

Expected Result

Currently, it produces an error.

Actual Result

To not produce an error if the new option is turned on.

Additional Info

I know you are aiming for TSLint feature parity first but after I turned this rule and fixed around 400 errors in our codebase I found out it is extremely useful in conditions and somewhat hard to fix(code becomes less readable) in other expressions.

Versions

package	version
@typescript-eslint/eslint-plugin	2.0.0
@typescript-eslint/parser	2.0.0
TypeScript	3.5.3
ESLint	6.2.2
node	10.16.0
npm	6.9.0

[astoilkov]

After I though a little more about this. Maybe it is possible the option to only skip expressions part of assignments.

[phaux]

There's already ignoreRhs option which does that

[astoilkov]

@phaux Hmm, I have ignoreRhs turned on and the code below produces an error for the rule:

const foo = undefined
const bar = {}
const baz = foo || bar // Unexpected non-boolean in conditional.

From what I have seen ignoreRhs won't produce an error if the foo variable is a boolean but the bar variable remains an object:

const foo = false
const bar = {}
const baz = foo || bar // No error with `ignoreRhs: true`

[bradzacher]

I think you're misunderstanding the option.

The option is "ignore RIGHT hand side".
Which means that it ignores the final value in a chain of booleans, so that you can use short circuiting to conditionally evaluate one expression.

This means that the other expressions in the chain must evaluate to a boolean.

The problem is that logical operators are subject to the same boolean coercion rules as an if/while/ternary/etc. Eg:

function unsafeIf(foo?: string) {
  if (foo) {
    return foo;
  }
  return {};
}
function unsafeLogical(foo?: string) {
  return foo || {};
}

unsafeIf('aaa'); // === 'aaa'
unsafeLogical('aaa'); // === {}

unsafeIf(undefined); // === {}
unsafeLogical(undefined); // === {}

// note that this is still unsafe
unsafeIf(''); // === {}
unsafeLogical(''); // === {}

---

Thinking about this more - we won't add an option to disable the rule for certain boolean expressions. It's either on for all of them, or none of them.
Otherwise people will do dodgy stuff like switching from an if to a || because it lets them do a looser check (speaking as someone working on a large codebase - "hacks" to get around lint rules spread by word of mouth, even if you have a good culture).

We will consider options to relax the boolean check, however. Examples of options we'd accept are:
https://palantir.github.io/tslint/rules/strict-boolean-expressions/
Happy if you'd like one of these, please open a new issue.

[astoilkov]

@bradzacher Thanks for the reply. I like your argument with developers finding their way around rules. I am trying to find a solution to the problem which is elegant so thanks for your argument.

From looking at the TSLint options I want to ask if allow-null-union option will make this code not error? (Tried to figure the answer on my own but the TSLint Playground doesn't seem to work.)

function getPattern(line: string) {
  const unorderedListRegEx = /^\s*[*+-]\s+/u
  const orderedListRegEx = /^\s*(\d{1,9})[.)]\s+/u
  const taskListRegEx = /^(\s*[*+-]\s+)(\[[x X]\]\s+)/u
  const quoteRegEx = /^([ ]*>[ ]*\s+)/u
  const match =
      line.match(taskListRegEx) ||
      line.match(unorderedListRegEx) ||
      line.match(orderedListRegEx) ||
      line.match(quoteRegEx)
}

[bradzacher]

It's always hard to tell, because tslint doesn't include examples in their readme...

But looking at the tests for allow-null-union, it looks like this would work.

Though note that this option does reduce the safety provided by the rule, as the above example with a sketchy string check will now pass through.

const foo: string | null = '';
const x = foo || 'foo was falsey';
// x === 'foo was falsey';

---

A big part of me would just recommend that you postpone adding this rule for now.
The typescript team is currently working on implementing the "null coalesce operator", which will make your code type safe without having to use looser options.

Scheduled for release in TS3.7 (i.e. the next version): microsoft/TypeScript#26578

function getPattern(line: string) {
  const unorderedListRegEx = /^\s*[*+-]\s+/u
  const orderedListRegEx = /^\s*(\d{1,9})[.)]\s+/u
  const taskListRegEx = /^(\s*[*+-]\s+)(\[[x X]\]\s+)/u
  const quoteRegEx = /^([ ]*>[ ]*\s+)/u
  const match =
      line.match(taskListRegEx) ??
      line.match(unorderedListRegEx) ??
      line.match(orderedListRegEx) ??
      line.match(quoteRegEx)
}

IIRC they aim for a ~2 month release cadence. So I'd say you'd be looking at the 3.7 RC being ready in ~1.5 months.

---

Up to you - might be worth just eslint-disableing it for now.

  // TODO - https://github.com/microsoft/TypeScript/issues/26578
  /* eslint-disable @typescript-eslint/strict-boolean-expressions */
  const match =
      line.match(taskListRegEx) ||
      line.match(unorderedListRegEx) ||
      line.match(orderedListRegEx) ||
      line.match(quoteRegEx)
  /* eslint-enable @typescript-eslint/strict-boolean-expressions */

[astoilkov]

@bradzacher Thank you so much for the response. This gave so much clarity to my questions. I didn't answer because I didn't receive an email notification for your answer. I accidentally returned to the thread to remember what happened as the new allowNullable option was introduced. You are awesome!