[strict-boolean-expressions] option to allow checks against non-falsey types (i.e. non string / number / boolean)

[EdJoPaTo]

Repro

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

interface Bar {
	bla: string;
}

interface Foo {
	bar?: Bar;
}

const foo: Foo = {};
const stringOrUndefined = foo.bar && foo.bar.bla;
if (foo.bar) {
	// …
}

Expected Result

There shouldn't be an error as the option allowNullable states to include undefined.
#698 asked for a quick null / undefined check which is not the case yet.

Actual Result

10:27  Unexpected non-boolean in conditional.
11:5   Unexpected non-boolean in conditional. 

Additional Info

Currently I assume allowNullable only allows for true | false | undefined | null and does not allow anything else (which would be Foo in this case). This is useful for something like property?: boolean which can be true | false | undefined and is still allowed by this rule but is not helpful for quick null / undefined checks.

Versions

package	version
@typescript-eslint/eslint-plugin	2.3.1
@typescript-eslint/parser	2.3.1
TypeScript	3.6.3
ESLint	6.5.0
node	12.10.0
npm	6.11.3

[bradzacher]

allowNullable is a safe-ish option because null == false and undefined == false, so it's easy to be safe whilst using this option, as long as you never write code that treats false and unset differently.
(I wouldn't use this option for this very reason - it's easy to do unsafe things).

Strings are different though because '' == false.

function foo(arg: string | undefined | null) {
    if (arg) {
        // never gets reached because '' == false
    }
}

foo('');

Which makes a boolean check on a string (OR a number) type inherently unsafe.

[EdJoPaTo]

I know empty strings for example are still seen as false which might be something different from what the user intended. In that case I'm with you as it should be a more specific check from the user.
But in the case if an object which follows a specific interface here I think it should work without errors.

Examples:

This should cause an error as it might do unintended things (string '' returns undefined instead of 0):

function foo(text: string | undefined): number | undefined {
  return text && text.length
}

But this should work with this rule in my opinion:

function foo(regex: RegExp | undefined): string | undefined {
	return regex && regex.flags;
}

[bradzacher]

definitely could add an option to allow objects, as that's always safe.

Eg:

// { allowObjects: true, allowNullable: true }
const stringOrUndefined =
  foo.bar && // no error
  foo.bar.bla; // error - string in a boolean
if (foo.bar) { // no error
	// …
}

[EdJoPaTo]

line 4 in the last example should only create an error when ignoreRhs is false. If it's true it shouldn't create one?

[bradzacher]

correct!

but ofc this would still error:

// { allowObjects: true, allowNullable: true, ignoreRhs: true }
const stringOrUndefined =
  foo.bar && // no error
  foo.bar.bla; // no error
if (foo.bar) { // no error
  // …
}
if (stringOrUndefined) { // error - string in a boolean location
  // …
}

[ark120202]

I don't think that it should just allow objects, but all unions with types that can't be falsy (objects, symbols and functions). So maybe something like allowSafe?

Also, IMO that should be default behavior when allowNullable is turned on, is there really a need to add more configuration for that? Now allowNullable appears to be the same as allow-boolean-or-undefined option in tslint rule, but I think usually it would be expected to work like allow-null-union and allow-undefined-union.

[bradzacher]

all unions with types that can't be falsy... maybe something like allowSafe?

Definitely! I just couldn't think of the other non-falsey types at the time.

I kind of wish I hadn't allowed allowNullable, because it's unsafe. But that's in there now and people are relying on it.
I don't really want to load more meaning into that option. I'd rather rename it to allowUnsafeNullableBoolean, and name the new option something like allowSafeNonFalseyTypes, with a default of true.
I'd then be comfortable adding allowUnsafeNullableNumber and allowUnsafeNullableString, because it'd be clear what's bad practice to use.

[ark120202]

I kind of wish I hadn't allowed allowNullable, because it's unsafe.

I don't really understand why it is unsafe, as long as string | undefined and number | undefined aren't allowed, nullable values should have expected results.

I'd then be comfortable adding allowUnsafeNullableNumber and allowUnsafeNullableString, because it'd be clear what's bad practice to use.

I think that's really unnecessary, what would this rule even check in that case?

---

I see 2 different use cases for strict-boolean-expressions:

1. Making sure that explicit comparison operators are used for values with surprising boolean coercion results (i.e. numbers, strings).
2. Checking that all expressions used in conditions are booleans, like in other strictly typed languages (current default behavior).

Are there really any other cases where you'd want more detailed control? Also, I think making allowSafe default should be considered, since second use case doesn't really add any extra safety on top of the first one, it's more like a stylistic preference.

[bradzacher]

I don't really understand why it is unsafe

It's safe as long as you assume that the "did not set" case is exactly the same as the false case.
But I've seen a lot of code over the years which does not assume that, because of a bug which a strict lint rule would have caught.

I.e. I've seen react components with a boolean flag which is assumed to have a default value of true without actually setting a default value (an easy thing to forget).

function foo(showFoo?: boolean) {
  if (!showFoo) {
    // ...
    return 'bar';
  }

  return 'foo';
}

foo(true); // 'foo'
foo(false); // 'bar'
foo(); // 'bar' - forgot the default value, so this evaluates to bar

function fooSafe(showFoo?: boolean) {
  if (showFoo === false) { // this causes the unset case to be treated the same as the true case.
    // ...
    return 'bar';
  }

  return 'foo';
}

foo(true); // 'foo'
foo(false); // 'bar'
foo(); // 'foo' - explicit boolean check means that the omission of the default value doesn't matter

// this would have the same effect
// function fooSafe(showFoo: boolean = true)

There's a reason that flow in strict mode includes compiler errors for nullable strings, nullable numbers and nullable booleans - to make sure you can never mess this case up.

---

I think that's really unnecessary, what would this rule even check in that case?

Definitely is necessary - I wouldn't add them myself. But I would be comfortable accepting PRs for the options if they were all named allowUnsafe*.

I can see an argument that people do explicitly want to allow empty strings to be treated as falsey. ESP when user input comes into play there is a strong argument for empty string being falsey - but it requires a lot of careful planning from all parts of a system.

I don't have an argument for consistently treating 0 as falsey though. Maybe there is one, but I don't have one.

---

I think making allowSafe default should be considered

Definitely would consider that for a 3.0 release (if this option got added before then).

[phaux]

I added allowNullable. I agree that this option makest things more unsafe. I added it because otherwise I had to write conditions like boolVal === true (with eqeqeq rule enabled) which completely change behavior (like when someone doesn't use TS and gives other truthy value to my function and it treats it as not true).

With nullish coalescing we can use boolVal ?? false which looks nice and explicit and this rule wont be necessary.

[phaux]

I'm indifferent on the "allow safe null check" option. I think maybeObj != null is explicit and concise enough. Enabling such option would again change the behavior if your typings are wrong or you're making a library and someone misuses it.

[pgsandstrom]

While maybeObj != null is easy enough to read, I think it is unnecessary for this rule to be so opinionated as to force the user to change bug free code. I currently face a huge barrier of entry in my current project as we have thousands of if(myObj), code that is perfectly safe and would be needed to change to start using this helpful rule.

I honestly dont see why this allowSafe option shouldnt simply be the default behaviour?

[pgsandstrom]

Well, after giving it a minute of thought I do realize that my suggestion would make the name "strict-boolean-expressions" be a bit misleading. But perhaps the issue is with the name of the rule then? Perhaps it should be called "no-unsafe-boolean-coercion" or something to that effect? Because as far as I can see, that is the reason to be using this rule.

[bradzacher]

I'm happy to have an option. Though I would prefer allowSafe being default off.

I think it's probably better for this rule to be "fully strict by default", with the option of relaxing it where you're okay having "a little less explicitness and strictness".
I much prefer that option style rather than "here's an option to relax it and here's an option to tighten it".

this project is driven by the community.
happy to accept a PR.

Nobody has actioned it because nobody has been driven enough to implement it (this issue is sitting at 0 reacts on the OP, which is good signal that it's not high on anyone's want/todo list).