feat(eslint-plugin): [no-unnecessary-boolean-literal-compare] add option to check nullable booleans

[zachkirsch]

No description provided.

[typescript-eslint]

Thanks for the PR, @zachkirsch!

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.

[codecov]

Codecov Report

Merging #1983 into master will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1983      +/-   ##
==========================================
+ Coverage   93.40%   93.43%   +0.03%     
==========================================
  Files         174      174              
  Lines        7896     7938      +42     
  Branches     2256     2274      +18     
==========================================
+ Hits         7375     7417      +42     
  Misses        247      247              
  Partials      274      274              

Flag	Coverage Δ
#unittest	93.43% <100.00%> (+0.03%)	⬆️

Impacted Files	Coverage Δ
packages/eslint-plugin/src/util/types.ts	82.08% <ø> (ø)
...rc/rules/no-unnecessary-boolean-literal-compare.ts	89.61% <100.00%> (+12.46%)	⬆️

[bradzacher]

thanks for the PR @zachkirsch - could you please provide some context behind this PR?
Why do you want this change?

To me it seems like this option removes a large portion of the usefulness of the rule as the most common case would be a nullable boolean.

[zachkirsch]

Yeah! Two reasons:

Reason 1: Comparing nullable booleans to literal true is unnecessary

Bad	Good (and equivalent)
myBooleanOrNullVar === true	myBooleanOrNullVar
myBooleanOrNullVar !== true	!myBooleanOrNullVar

Reason 2: Comparing nullable booleans to literal false is confusing

I see myOptionalBoolean !== false a lot of the time, usually as optional props (that default to true) in react components. For example:

interface Props {
  // whether to display title (defaults to true)
  displayTitle?: boolean   
}

...

  public render() {
    return (
      <div>
        {this.props.displayTitle !== false && renderTitle()}
        ...

Before ?? existed, myOptionalProp !== false was a sensible way to check an optional prop and default to true if it's undefined. In my opinion, I think that myOptionalProp !== false is a very confusing way to express "use this prop but default to true if it's missing," and IMO it's harder to see bugs that are written that way.

?? is the perfect replacement for this scenario - it's a built-in operator that provides a default if your variable is undefined at runtime. (this.props.displayTitle ?? true) && renderTitle() very clearly means "render the title based on the value of this.props.displayTitle (and default to true if it's not defined)"

To me it seems like this option removes a large portion of the usefulness of the rule as the most common case would be a nullable boolean.

I think this rule is still very useful with this option, as it

1. catches unnecessary comparisons with boolean variables
   1. e.g. myBoolean !== true --> !myBoolean
2. catches unnecessary comparisons between nullable boolean variables and true
   1. e.g. myOptionalBoolean !== true --> !myOptionalBoolean
3. cleans up comparisons between optional booleans and false (described above in this comment)

[bradzacher]

mostly lgtm. one comment.
Thanks for your work

[bradzacher]

Suggested change

	allowComparingNullableBooleans: true,
	allowComparingNullableBooleans: false,

[zachkirsch]

Sweet! I actually am having some second thoughts haha. I feel like auto-fixing varBooleanOrNull === false to !(varBooleanOrNull ?? true) is kind of weird - the "fixed" version is also a bit hard to parse visually.

Fixing varBooleanOrNull === true to varBooleanOrNull and varBooleanOrNull !== true to !varBooleanOrNull is good IMO. I think fixing varBooleanOrNull !== false to varBooleanOrNull ?? true is also good but I could see it either way.

What are your thoughts? We could also have additional config options that more granularly control which of these four comparisons are checked/fixed by the rule.

[bradzacher]

Hmmm. Sorry, I did a quick eyeball and didn't notice the fix output.
That's my bad.

treat the false and nullish cases the same

• foo === true => foo
• foo !== true => !foo

treat the true and nullish cases the same

• foo === false => foo ?? true
• foo !== false => !(foo ?? true)

I think that is probably fine.
Personally, I think that the latter two cases might be good to ignore, as IMO they're clearer the other way.

The other option for the last two is to fix to something like foo !== undefined && !foo, but that's pretty verbose, and it's probably better to not fix at all there....

[zachkirsch]

Okay, I broke allowComparingNullableBooleans into two options: allowComparingNullableBooleansToTrue and allowComparingNullableBooleansToFalse, so users can decide what they want to allow. If you want, I'm happy to just have the rule ignore comparisons between nullable boolean variables and literal false, but I think it might be confusing if eslint complains about booleanOrNullVar === true but not booleanOrNullVar === false.

I also added some more docs to the rule's readme, including a table of what exactly the fixer does in each case.

[zachkirsch]

Lemme know if you need anything else from me!

[bradzacher]

lgtm - thanks!