[linter] New lint to suggest avoid `bool == true` and `bool == false`

[FMorschel]

We have use_if_null_to_convert_nulls_to_bools to trigger the first case, but no diagnostic in the other two cases:

void foo(bool? nullable, bool boolean) {
  if (nullable == true) {}
  if (boolean == true) {}
  if (boolean == false) {}
}

CC @srawlins @bwilkerson

[bwilkerson]

@devoncarew @lrhn @natebosch Is this a lint that we'd be likely to add to the recommended set?

[lrhn]

Yes. It's a classic unnecessary code lint.

I don't remember if we have something similar for && and || already, but the following at all unnecessarily overlong:

• bool == true, true == bool => bool
• bool == false, false == bool => !bool
• bool1 ? true : bool2 => bool1 || bool2
• bool1 ? false : bool2 => !bool1 && bool2
• bool1 ? bool2 : true => !bool1 || bool2
• bool1 ? bool2 : false => bool1 && bool2
• bool1 || true, true || bool1 => true
• bool1 || false, false || bool1 => bool1
• bool1 && true, true && bool1 => bool1
• bool1 && false, false && bool1 => false

You never need a boolan literal in a boolean logic expression. In an equality, it only makes sense if the other expression's type is not bool (whether bool? or Object).

I've come to like nullable?.test() == true, and is not sure I prefer nullable?.test() ?? false.
The former does a computation giving you one of three values, and then compares against one of them and the result is the result of that one comparison. For anything other than bool that would be perfect.
The latter does a computation + check to give you either the result of the entire expression, or null, which is then replaced with another value. That's three possible result values, coming from two places, and I have to count on my fingers to figure out what null means in relation to the test of the test() function.

(Maybe it's habit. ?? false can be read as "null means the same as false". I just find it harder to read and reason about, because the boolean being switched on comes from more than one test. Nested ?/: expressions are hard to read too.)

[FMorschel]

To note, I found this when working on https://dart-review.googlesource.com/c/sdk/+/424560 at pkg/analysis_server/lib/src/services/correction/dart/create_getter.dart.

sdk/pkg/analysis_server/lib/src/services/correction/dart/create_getter.dart

Line 197 in b573667

if (targetElement.library2.isInSdk == true) {

[FMorschel]

• bool1 || true => true
• bool1 || false => bool1
• bool1 && true => bool1
• bool1 && false => false

This reminds me that (I'm not sure we should, but) we can test boolean operations with the same expression on both sides (immutable expressions, of course - private final fields, local variables, etc):

• bool1 == bool1 => true
• bool1 != bool1 => false
• bool1 || bool1 => bool1
• bool1 && bool1 => bool1
• bool1 ^ bool1 => false

Also:

• bool1 ? bool1 : bool2 => bool1 || bool2
• bool1 ? bool2 : bool1 => bool1 && bool2

This one is more complex but it can help if we are willing to implement (found at flutter/flutter#156928 (comment))

• bool1 ? (bool2 ? exp1 : exp2) : (bool2 ? exp2 : exp1) => bool1 == bool2 ? exp1 : exp2

[bwilkerson]

It's a classic unnecessary code lint.

Most of the unnecessary code diagnostics are warnings so that they don't need to be enabled by users. Sounds like we should consider making these warnings too.

... we can test boolean operations with the same expression on both sides (immutable expressions, of course - private final fields, local variables, etc)

At one time we had a lint that attempted to identify conditions whose value was always the same. It ended up being a nightmare, so it's going to take a lot of work to convince me that we want to go down that path again.

This one is more complex ...

More complex, but not necessarily harder to read. I don't think I'd want to flag this kind of case, but I could imagine adding an assist to 'Simplify expression' for some cases like this. I'm not convinced that it has enough value to warrant the cost of implementation and maintenance, though, so I'm not proposing that we add it.

[lrhn]

The bool1== bool1 can be generalized to most types that do not contain a double. (You should make == reflexive, but NaN breaks that. Bloody NaN.)

Everything that applies to || and&& should also apply to | and &. They're the same operation, just not short-circuiting.

[FMorschel]

I found out today that no_literal_bool_comparisons exists (docs).

But we are still missing the other operand considerations (||, |, &&, &), and we could probably think about enabling it on the SDK. It is on Effective Dart.

Here are the current covered cases:

https://github.com/dart-lang/sdk/blob/94a36d59ff68b3e53ceacd9b954528365eca8b22/pkg/analysis_server/test/src/services/correction/fix/convert_to_boolean_expression_test.dart

I'll take a look at implementing the other cases.

[FMorschel]

This is almost done, but for some weird reason, my ^ tests are giving a "missing ')'" error, but when I copy that into an IDE, no errors.

https://dart-review.googlesource.com/c/sdk/+/430901

Once I solve that, I think everything is in place.

[FMorschel]

Note, I found this related issue #58966 (I think is the issue that created this rule).