bool_assert_comparison false positive when one side is non-bool with PartialEq<bool> impl

[tl-alex-butler]

Lint name: bool_assert_comparison

When testing serde_json::Value values, or more generally more-complex-than-bool types that implement PartialEq<bool>, assert_eq! testing should not be linted against.

Example

// serde_json = "1"

let val: serde_json::Value = serde_json::from_str(r#"{"bar": true, "baz": 123}"#).unwrap();

assert_eq!(val["bar"], true);
assert_eq!(val["baz"], false);

These trigger bool_assert_comparison lint

warning: used `assert_eq!` with a literal bool
 --> src/main.rs:5:5
  |
5 |     assert_eq!(val["baz"], false);
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)`

However, these aren't trivially fixable with assert!(..). Moreover, converting to a bool for testing is worse code as it provides less info in a test failure scenario. For example, running the example code tells us not only that the 2nd assert fails, but exactly what the actual value was.

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `Number(123)`,
 right: `false`', src/main.rs:5:5

A workaround like assert_eq!(val["baz"].as_bool(), Some(false)) would make this test code worse, and does not seem to be the intention of this lint.

Meta

• cargo clippy -V: clippy 0.1.56 (ad981d5 2021-08-08)
• rustc -Vv:

  rustc 1.56.0-nightly (ad981d58e 2021-08-08)
  binary: rustc
  commit-hash: ad981d58e1ca16bcf4072577934630deb11c5e14
  commit-date: 2021-08-08
  host: x86_64-unknown-linux-gnu
  release: 1.56.0-nightly
  LLVM version: 12.0.1
  

[xordi]

Hi @Manishearth! I would like to try fixing this issue if you don't mind. I'm absolutely new to Rust community.

I've seen that currently this lint is only checking if the assert parameters are boolean literals. My idea here is the following:

• Leave everything as it is when both arguments are boolean literals or none is.
• In case one of them is a boolean literal, check if the other one is an index expression as the one @tl-alex-butler is showing, or if it's a field expression (I think things like assert_eq!(mystruct.v, true) should be valid too). In case the argument is a field or an index expression, we can leave things as they are.

What do you think about it?

[xFrednet]

Welcome to the project @xordi,

for lints that work with macros, it's usually good to understand how they work. For that, you can open the Rust playground and see the macro expansion using tools > Expand macros. (See playground)

In this case, the difference comes from the way the values are checked. assert_eq! actually compares the two values using the equal operator (if !(*left_val == *right_val) {) while assert! is using the not operator !val["bar"]. So, this bug most likely originates from the fact that val["bar"] has implemented the PartialEq for the comparison with bools but not the ops::Not operator used in the assert! macro and the lint doesn't check for this.

Based on this, I would suggest to check for the Not trait to decide if the code can be rewritten. See Checking if a type implements a specific trait. The trait should be available under the lang item "not".

If this sounds interesting to you, and you want to try your hand on it, feel free to claim the issue via @rustbot claim. You can also ping me or other members if you have any questions 🙃

[xordi]

@rustbot claim

[xordi]

Thanks for the explanation @xFrednet.

Let's say we have the following code, with an enum type that implements PartialEq<bool> (similar to serde_json::Value) and also Not traits:

use std::ops::Not;

#[derive(Debug)]
enum Test {
    Bool(bool),
    Number(u32)
}

impl PartialEq<bool> for Test {
    fn eq(&self, other: &bool) -> bool {
        match *self {
            Test::Bool(b) => b == *other,
            _ => false
        }
    }
}

impl Not for Test {
    type Output = Self;

    fn not(self) -> Self::Output {
        match self {
            Test::Bool(b) => Test::Bool(!b),            
            Test::Number(0) => Test::Number(1),
            Test::Number(_) => Test::Number(0),
        }
    }
}

fn main() {    
    let t = Test::Bool(true);
    
    // currently clippy suggests that this should be replaced
    // with assert!(...)
    assert_eq!(t, true);

    // assert! expects a boolean expression, so this
    // doesn't compile
    assert!(t);
}

It looks wrong when clippy suggests a rewrite that leads into a compilation error like in this case. So maybe just checking for the Not trait is not enough. Maybe we should check that the non literal part is in fact a boolean expression prior to suggesting rewriting to assert!, although it may be that I am missing something in your view on this.

[xFrednet]

You're welcome!

That's a very good test 👍 Rust allows operators to define their output type. The given example defines the output type as Self which is not a bool. (type Output = Self; line 19). The solution in this case would be to check that the type of Output from the Not implementation is bool. (See clippy_utils::ty::get_iterator_item_ty() as a reference)

Maybe we should check that the non literal part is in fact a boolean expression prior to suggesting rewriting to assert!.

I'm guessing that you're referring here to checking the actual type of the expression? That would also prevent some false positives, like in this case, but could also lead to false negatives. For instance, if serde_json::Value would actually implement the Not trait. I would therefore suggest first checking for the Not trait, with the output being a bool. If that doesn't work, we can take a look at this 🙃