Auto merge of rust-lang#7083 - GuillaumeGomez:bool-assert-eq, r=camsteffen

bors · bors · commit bbc22e2ef308 · 2021-04-21T13:58:53.000Z
Add lint to check for boolean comparison in assert macro calls

This PR adds a lint to check if an assert macro is using a boolean as "comparison value". For example:

```rust
assert_eq!("a".is_empty(), false);
```

Could be rewritten as:

```rust
assert!(!"a".is_empty());
```

PS: The dev guidelines are amazing. Thanks a lot for writing them!

changelog: Add `bool_assert_comparison` lint
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2124,6 +2124,7 @@ Released 2018-09-13
 [`blacklisted_name`]: https://rust-lang.github.io/rust-clippy/master/index.html#blacklisted_name
 [`blanket_clippy_restriction_lints`]: https://rust-lang.github.io/rust-clippy/master/index.html#blanket_clippy_restriction_lints
 [`blocks_in_if_conditions`]: https://rust-lang.github.io/rust-clippy/master/index.html#blocks_in_if_conditions
+[`bool_assert_comparison`]: https://rust-lang.github.io/rust-clippy/master/index.html#bool_assert_comparison
 [`bool_comparison`]: https://rust-lang.github.io/rust-clippy/master/index.html#bool_comparison
 [`borrow_interior_mutable_const`]: https://rust-lang.github.io/rust-clippy/master/index.html#borrow_interior_mutable_const
 [`borrowed_box`]: https://rust-lang.github.io/rust-clippy/master/index.html#borrowed_box
diff --git a/clippy_lints/src/bool_assert_comparison.rs b/clippy_lints/src/bool_assert_comparison.rs
@@ -0,0 +1,75 @@
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::{ast_utils, is_direct_expn_of};
+use rustc_ast::ast::{Expr, ExprKind, Lit, LitKind};
+use rustc_errors::Applicability;
+use rustc_lint::{EarlyContext, EarlyLintPass};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+
+declare_clippy_lint! {
+    /// **What it does:** This lint warns about boolean comparisons in assert-like macros.
+    ///
+    /// **Why is this bad?** It is shorter to use the equivalent.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    ///
+    /// ```rust
+    /// // Bad
+    /// assert_eq!("a".is_empty(), false);
+    /// assert_ne!("a".is_empty(), true);
+    ///
+    /// // Good
+    /// assert!(!"a".is_empty());
+    /// ```
+    pub BOOL_ASSERT_COMPARISON,
+    style,
+    "Using a boolean as comparison value in an assert_* macro when there is no need"
+}
+
+declare_lint_pass!(BoolAssertComparison => [BOOL_ASSERT_COMPARISON]);
+
+fn is_bool_lit(e: &Expr) -> bool {
+    matches!(
+        e.kind,
+        ExprKind::Lit(Lit {
+            kind: LitKind::Bool(_),
+            ..
+        })
+    ) && !e.span.from_expansion()
+}
+
+impl EarlyLintPass for BoolAssertComparison {
+    fn check_expr(&mut self, cx: &EarlyContext<'_>, e: &Expr) {
+        let macros = ["assert_eq", "debug_assert_eq"];
+        let inverted_macros = ["assert_ne", "debug_assert_ne"];
+
+        for mac in macros.iter().chain(inverted_macros.iter()) {
+            if let Some(span) = is_direct_expn_of(e.span, mac) {
+                if let Some([a, b]) = ast_utils::extract_assert_macro_args(e) {
+                    let nb_bool_args = is_bool_lit(a) as usize + is_bool_lit(b) as usize;
+
+                    if nb_bool_args != 1 {
+                        // If there are two boolean arguments, we definitely don't understand
+                        // what's going on, so better leave things as is...
+                        //
+                        // Or there is simply no boolean and then we can leave things as is!
+                        return;
+                    }
+
+                    let non_eq_mac = &mac[..mac.len() - 3];
+                    span_lint_and_sugg(
+                        cx,
+                        BOOL_ASSERT_COMPARISON,
+                        span,
+                        &format!("used `{}!` with a literal bool", mac),
+                        "replace it with",
+                        format!("{}!(..)", non_eq_mac),
+                        Applicability::MaybeIncorrect,
+                    );
+                    return;
+                }
+            }
+        }
+    }
+}
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -178,6 +178,7 @@ mod await_holding_invalid;
 mod bit_mask;
 mod blacklisted_name;
 mod blocks_in_if_conditions;
+mod bool_assert_comparison;
 mod booleans;
 mod bytecount;
 mod cargo_common_metadata;
@@ -568,6 +569,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         bit_mask::VERBOSE_BIT_MASK,
         blacklisted_name::BLACKLISTED_NAME,
         blocks_in_if_conditions::BLOCKS_IN_IF_CONDITIONS,
+        bool_assert_comparison::BOOL_ASSERT_COMPARISON,
         booleans::LOGIC_BUG,
         booleans::NONMINIMAL_BOOL,
         bytecount::NAIVE_BYTECOUNT,
@@ -1273,6 +1275,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| box from_str_radix_10::FromStrRadix10);
     store.register_late_pass(|| box manual_map::ManualMap);
     store.register_late_pass(move || box if_then_some_else_none::IfThenSomeElseNone::new(msrv));
+    store.register_early_pass(|| box bool_assert_comparison::BoolAssertComparison);
 
     store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![
         LintId::of(arithmetic::FLOAT_ARITHMETIC),
@@ -1453,6 +1456,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(bit_mask::INEFFECTIVE_BIT_MASK),
         LintId::of(blacklisted_name::BLACKLISTED_NAME),
         LintId::of(blocks_in_if_conditions::BLOCKS_IN_IF_CONDITIONS),
+        LintId::of(bool_assert_comparison::BOOL_ASSERT_COMPARISON),
         LintId::of(booleans::LOGIC_BUG),
         LintId::of(booleans::NONMINIMAL_BOOL),
         LintId::of(casts::CAST_REF_TO_MUT),
@@ -1739,6 +1743,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(attrs::BLANKET_CLIPPY_RESTRICTION_LINTS),
         LintId::of(blacklisted_name::BLACKLISTED_NAME),
         LintId::of(blocks_in_if_conditions::BLOCKS_IN_IF_CONDITIONS),
+        LintId::of(bool_assert_comparison::BOOL_ASSERT_COMPARISON),
         LintId::of(casts::FN_TO_NUMERIC_CAST),
         LintId::of(casts::FN_TO_NUMERIC_CAST_WITH_TRUNCATION),
         LintId::of(collapsible_if::COLLAPSIBLE_ELSE_IF),
diff --git a/clippy_utils/src/ast_utils.rs b/clippy_utils/src/ast_utils.rs
@@ -5,6 +5,7 @@
 #![allow(clippy::similar_names, clippy::wildcard_imports, clippy::enum_glob_use)]
 
 use crate::{both, over};
+use if_chain::if_chain;
 use rustc_ast::ptr::P;
 use rustc_ast::{self as ast, *};
 use rustc_span::symbol::Ident;
@@ -571,3 +572,34 @@ pub fn eq_mac_args(l: &MacArgs, r: &MacArgs) -> bool {
         _ => false,
     }
 }
+
+/// Extract args from an assert-like macro.
+///
+/// Currently working with:
+/// - `assert_eq!` and `assert_ne!`
+/// - `debug_assert_eq!` and `debug_assert_ne!`
+///
+/// For example:
+///
+/// `debug_assert_eq!(a, b)` will return Some([a, b])
+pub fn extract_assert_macro_args(mut expr: &Expr) -> Option<[&Expr; 2]> {
+    if_chain! {
+        if let ExprKind::If(_, ref block, _) = expr.kind;
+        if let StmtKind::Semi(ref e) = block.stmts.get(0)?.kind;
+        then {
+            expr = e;
+        }
+    }
+    if_chain! {
+        if let ExprKind::Block(ref block, _) = expr.kind;
+        if let StmtKind::Expr(ref expr) = block.stmts.get(0)?.kind;
+        if let ExprKind::Match(ref match_expr, _) = expr.kind;
+        if let ExprKind::Tup(ref tup) = match_expr.kind;
+        if let [a, b, ..] = tup.as_slice();
+        if let (&ExprKind::AddrOf(_, _, ref a), &ExprKind::AddrOf(_, _, ref b)) = (&a.kind, &b.kind);
+        then {
+            return Some([&*a, &*b]);
+        }
+    }
+    None
+}
diff --git a/tests/ui/bool_assert_comparison.rs b/tests/ui/bool_assert_comparison.rs
@@ -0,0 +1,59 @@
+#![warn(clippy::bool_assert_comparison)]
+
+macro_rules! a {
+    () => {
+        true
+    };
+}
+macro_rules! b {
+    () => {
+        true
+    };
+}
+
+fn main() {
+    assert_eq!("a".len(), 1);
+    assert_eq!("a".is_empty(), false);
+    assert_eq!("".is_empty(), true);
+    assert_eq!(true, "".is_empty());
+    assert_eq!(a!(), b!());
+    assert_eq!(a!(), "".is_empty());
+    assert_eq!("".is_empty(), b!());
+
+    assert_ne!("a".len(), 1);
+    assert_ne!("a".is_empty(), false);
+    assert_ne!("".is_empty(), true);
+    assert_ne!(true, "".is_empty());
+    assert_ne!(a!(), b!());
+    assert_ne!(a!(), "".is_empty());
+    assert_ne!("".is_empty(), b!());
+
+    debug_assert_eq!("a".len(), 1);
+    debug_assert_eq!("a".is_empty(), false);
+    debug_assert_eq!("".is_empty(), true);
+    debug_assert_eq!(true, "".is_empty());
+    debug_assert_eq!(a!(), b!());
+    debug_assert_eq!(a!(), "".is_empty());
+    debug_assert_eq!("".is_empty(), b!());
+
+    debug_assert_ne!("a".len(), 1);
+    debug_assert_ne!("a".is_empty(), false);
+    debug_assert_ne!("".is_empty(), true);
+    debug_assert_ne!(true, "".is_empty());
+    debug_assert_ne!(a!(), b!());
+    debug_assert_ne!(a!(), "".is_empty());
+    debug_assert_ne!("".is_empty(), b!());
+
+    // assert with error messages
+    assert_eq!("a".len(), 1, "tadam {}", 1);
+    assert_eq!("a".len(), 1, "tadam {}", true);
+    assert_eq!("a".is_empty(), false, "tadam {}", 1);
+    assert_eq!("a".is_empty(), false, "tadam {}", true);
+    assert_eq!(false, "a".is_empty(), "tadam {}", true);
+
+    debug_assert_eq!("a".len(), 1, "tadam {}", 1);
+    debug_assert_eq!("a".len(), 1, "tadam {}", true);
+    debug_assert_eq!("a".is_empty(), false, "tadam {}", 1);
+    debug_assert_eq!("a".is_empty(), false, "tadam {}", true);
+    debug_assert_eq!(false, "a".is_empty(), "tadam {}", true);
+}
diff --git a/tests/ui/bool_assert_comparison.stderr b/tests/ui/bool_assert_comparison.stderr
@@ -0,0 +1,112 @@
+error: used `assert_eq!` with a literal bool
+  --> $DIR/bool_assert_comparison.rs:16:5
+   |
+LL |     assert_eq!("a".is_empty(), false);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)`
+   |
+   = note: `-D clippy::bool-assert-comparison` implied by `-D warnings`
+
+error: used `assert_eq!` with a literal bool
+  --> $DIR/bool_assert_comparison.rs:17:5
+   |
+LL |     assert_eq!("".is_empty(), true);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)`
+
+error: used `assert_eq!` with a literal bool
+  --> $DIR/bool_assert_comparison.rs:18:5
+   |
+LL |     assert_eq!(true, "".is_empty());
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)`
+
+error: used `assert_ne!` with a literal bool
+  --> $DIR/bool_assert_comparison.rs:24:5
+   |
+LL |     assert_ne!("a".is_empty(), false);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)`
+
+error: used `assert_ne!` with a literal bool
+  --> $DIR/bool_assert_comparison.rs:25:5
+   |
+LL |     assert_ne!("".is_empty(), true);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)`
+
+error: used `assert_ne!` with a literal bool
+  --> $DIR/bool_assert_comparison.rs:26:5
+   |
+LL |     assert_ne!(true, "".is_empty());
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)`
+
+error: used `debug_assert_eq!` with a literal bool
+  --> $DIR/bool_assert_comparison.rs:32:5
+   |
+LL |     debug_assert_eq!("a".is_empty(), false);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)`
+
+error: used `debug_assert_eq!` with a literal bool
+  --> $DIR/bool_assert_comparison.rs:33:5
+   |
+LL |     debug_assert_eq!("".is_empty(), true);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)`
+
+error: used `debug_assert_eq!` with a literal bool
+  --> $DIR/bool_assert_comparison.rs:34:5
+   |
+LL |     debug_assert_eq!(true, "".is_empty());
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)`
+
+error: used `debug_assert_ne!` with a literal bool
+  --> $DIR/bool_assert_comparison.rs:40:5
+   |
+LL |     debug_assert_ne!("a".is_empty(), false);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)`
+
+error: used `debug_assert_ne!` with a literal bool
+  --> $DIR/bool_assert_comparison.rs:41:5
+   |
+LL |     debug_assert_ne!("".is_empty(), true);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)`
+
+error: used `debug_assert_ne!` with a literal bool
+  --> $DIR/bool_assert_comparison.rs:42:5
+   |
+LL |     debug_assert_ne!(true, "".is_empty());
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)`
+
+error: used `assert_eq!` with a literal bool
+  --> $DIR/bool_assert_comparison.rs:50:5
+   |
+LL |     assert_eq!("a".is_empty(), false, "tadam {}", 1);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)`
+
+error: used `assert_eq!` with a literal bool
+  --> $DIR/bool_assert_comparison.rs:51:5
+   |
+LL |     assert_eq!("a".is_empty(), false, "tadam {}", true);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)`
+
+error: used `assert_eq!` with a literal bool
+  --> $DIR/bool_assert_comparison.rs:52:5
+   |
+LL |     assert_eq!(false, "a".is_empty(), "tadam {}", true);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)`
+
+error: used `debug_assert_eq!` with a literal bool
+  --> $DIR/bool_assert_comparison.rs:56:5
+   |
+LL |     debug_assert_eq!("a".is_empty(), false, "tadam {}", 1);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)`
+
+error: used `debug_assert_eq!` with a literal bool
+  --> $DIR/bool_assert_comparison.rs:57:5
+   |
+LL |     debug_assert_eq!("a".is_empty(), false, "tadam {}", true);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)`
+
+error: used `debug_assert_eq!` with a literal bool
+  --> $DIR/bool_assert_comparison.rs:58:5
+   |
+LL |     debug_assert_eq!(false, "a".is_empty(), "tadam {}", true);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)`
+
+error: aborting due to 18 previous errors
+
