feat(eslint-plugin): [strict-boolean-expressions] refactor, add clearer error messages

[phaux]

• Added more error messages for each case with helpful tips.
• Added tests and fixed [strict-boolean-expressions] ignoreRhs is too lenient #1118

The positions of report messages also changed as a side effect of different logic for handling nested logical expressions. This was necessary to fix #1118

[typescript-eslint]

Thanks for the PR, @phaux!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

---

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

[phaux]

BTW I also noticed that allowSafe option doesn't do what I thought it does at all. I thought it would allow exactly TruthyType | FalsyType (so e.g. object | function | symbol | null | undefined), but it actually doesn't do that – it allows TruthyType | boolean so it's only useful with allowNullable, but even then it's too lenient because then TruthyType | boolean | FalsyType is also okay.

I want to add an option for this in a followup PR and a few others for cases that actually happen often in the wild and could be considered annoying and safe to allow via option.

[glen-84]

That PR mentioned:

Note that setting allowNullable & allowSafe to true allows boolean | null as a boolean expression, which is not safe.

... it would be great if that was disallowed.

[phaux]

It's now trivial to have separate logic for each combination of types. The only question is whether we want a breaking change here instead of creating a new option that does this right.

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

[bradzacher]

Just to keep this PR focused, I've spun up #1515 to discuss new rule options

[phaux]

I changed the check for a primitive type so that the string and number cases are handled separately.

I also improved the handling of nested logical operators so that it now recurses over the operands all the way instead of checking only one level deep. This prevents double-reporting the whole expression and one of its operands at once, no matter how deeply nested they are.

I think I'm done with this PR 👌

[bradzacher]

question:
How does this rule handle never types?
It looks like inspectVariantTypes will not handle it.

---

Mostly LGTM. A few comments.
Thanks for your work here

[phaux]

Improved code style and made it so that the never type is always allowed 😎

[bradzacher]

LGTM - thanks for your work here

[codecov]

Codecov Report

Merging #1480 into master will increase coverage by 0.07%.
The diff coverage is 98.65%.

@@            Coverage Diff             @@
##           master    #1480      +/-   ##
==========================================
+ Coverage   95.47%   95.54%   +0.07%     
==========================================
  Files         140      142       +2     
  Lines        6449     6579     +130     
  Branches     1851     1880      +29     
==========================================
+ Hits         6157     6286     +129     
+ Misses        107      106       -1     
- Partials      185      187       +2

Impacted Files	Coverage Δ
...estree/src/create-program/createIsolatedProgram.ts	75% <ø> (ø)	⬆️
...plugin/src/rules/explicit-module-boundary-types.ts	85.71% <100%> (+0.25%)	⬆️
...pt-estree/src/create-program/createWatchProgram.ts	92.39% <100%> (+0.04%)	⬆️
...es/eslint-plugin/src/rules/no-floating-promises.ts	100% <100%> (ø)	⬆️
...s/eslint-plugin/src/rules/no-dupe-class-members.ts	100% <100%> (ø)
...-estree/src/create-program/createDefaultProgram.ts	76.19% <100%> (ø)	⬆️
...nt-plugin/src/rules/switch-exhaustiveness-check.ts	100% <100%> (ø)
.../eslint-plugin/src/util/explicitReturnTypeUtils.ts	100% <100%> (ø)	⬆️
packages/eslint-plugin/src/rules/unbound-method.ts	96.66% <100%> (+2.22%)	⬆️
...-estree/src/create-program/createProjectProgram.ts	92.85% <85.71%> (-1.43%)	⬇️
... and 5 more