Add lint to check for boolean comparison in assert macro calls

GuillaumeGomez · GuillaumeGomez · commit 47fdfc8f9828 · 2021-04-16T21:15:47.000+02:00
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,127 @@
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::source::snippet;
+use rustc_ast::ast::{MacArgs, MacCall};
+use rustc_ast::token::{Token, TokenKind};
+use rustc_ast::tokenstream::TokenTree;
+use rustc_errors::Applicability;
+use rustc_lint::{EarlyContext, EarlyLintPass};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::symbol::kw::{False, True};
+
+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,
+    pedantic,
+    "Using a boolean as comparison value when there is no need"
+}
+
+declare_lint_pass!(BoolAssertComparison => [BOOL_ASSERT_COMPARISON]);
+
+fn is_bool(tt: Option<&TokenTree>) -> bool {
+    match tt {
+        Some(TokenTree::Token(Token {
+            kind: TokenKind::Ident(i, _),
+            ..
+        })) => *i == True || *i == False,
+        _ => false,
+    }
+}
+
+impl EarlyLintPass for BoolAssertComparison {
+    fn check_mac(&mut self, cx: &EarlyContext<'_>, mac: &MacCall) {
+        let macros = ["assert_eq", "debug_assert_eq"];
+        let inverted_macros = ["assert_ne", "debug_assert_ne"];
+
+        if let MacArgs::Delimited(_, _, ts) = &*mac.args {
+            let name;
+            if let [seg] = &*mac.path.segments {
+                name = seg.ident.name.as_str().to_string();
+                if !macros.contains(&name.as_str()) && !inverted_macros.contains(&name.as_str()) {
+                    return;
+                }
+            } else {
+                return;
+            }
+            let mut has_true = false;
+            let mut has_false = false;
+            let mut bool_span = None;
+            let mut spans = Vec::new();
+            let mut nb_comma = 0;
+            let mut iter = ts.trees().peekable();
+            while let Some(tt) = iter.next() {
+                if let TokenTree::Token(ref token) = tt {
+                    match token.kind {
+                        TokenKind::Ident(i, _) => {
+                            // We only want to check the comparison arguments, nothing else!
+                            if nb_comma < 2 {
+                                if i == True {
+                                    has_true = true;
+                                    bool_span = Some(token.span);
+                                    continue;
+                                } else if i == False {
+                                    has_false = true;
+                                    bool_span = Some(token.span);
+                                    continue;
+                                }
+                            }
+                        },
+                        TokenKind::Comma => {
+                            nb_comma += 1;
+                            if nb_comma > 1 || !is_bool(iter.peek()) {
+                                spans.push((token.span, true));
+                            }
+                            continue;
+                        },
+                        _ => {},
+                    }
+                }
+                spans.push((tt.span(), false));
+            }
+            #[allow(clippy::if_same_then_else)] // It allows better code reability here.
+            if has_false && has_true {
+                // Don't know what's going on here, but better leave things as is...
+                return;
+            } else if !has_true && !has_false {
+                // No problem detected!
+                return;
+            }
+            let text = spans
+                .into_iter()
+                .map(|(s, needs_whitespace)| {
+                    let mut s = snippet(cx, s, "arg").to_string();
+                    if needs_whitespace {
+                        s.push(' ');
+                    }
+                    s
+                })
+                .collect::<String>();
+            let is_cond_inverted = inverted_macros.contains(&name.as_str());
+            let extra = (has_true && is_cond_inverted) || has_false;
+
+            span_lint_and_sugg(
+                cx,
+                BOOL_ASSERT_COMPARISON,
+                bool_span.expect("failed to get the bool span"),
+                "assert macro with a boolean comparison",
+                "replace it with",
+                format!("{}!({}{})", &name[..name.len() - 3], if extra { "!" } else { "" }, text),
+                Applicability::MachineApplicable,
+            );
+        }
+    }
+}
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;
@@ -393,6 +394,7 @@ pub fn register_pre_expansion_lints(store: &mut rustc_lint::LintStore) {
     store.register_pre_expansion_pass(|| box write::Write::default());
     store.register_pre_expansion_pass(|| box attrs::EarlyAttributes);
     store.register_pre_expansion_pass(|| box dbg_macro::DbgMacro);
+    store.register_pre_expansion_pass(|| box bool_assert_comparison::BoolAssertComparison);
 }
 
 #[doc(hidden)]
@@ -567,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,
@@ -1332,6 +1335,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(await_holding_invalid::AWAIT_HOLDING_LOCK),
         LintId::of(await_holding_invalid::AWAIT_HOLDING_REFCELL_REF),
         LintId::of(bit_mask::VERBOSE_BIT_MASK),
+        LintId::of(bool_assert_comparison::BOOL_ASSERT_COMPARISON),
         LintId::of(bytecount::NAIVE_BYTECOUNT),
         LintId::of(case_sensitive_file_extension_comparisons::CASE_SENSITIVE_FILE_EXTENSION_COMPARISONS),
         LintId::of(casts::CAST_LOSSLESS),
diff --git a/tests/ui/bool_assert_comparison.rs b/tests/ui/bool_assert_comparison.rs
@@ -0,0 +1,30 @@
+#![warn(clippy::bool_assert_comparison)]
+
+fn main() {
+    assert_eq!("a".len(), 1);
+    assert_eq!("a".is_empty(), false);
+    assert_eq!("".is_empty(), true);
+    assert_eq!(true, "".is_empty());
+
+    assert_ne!("a".len(), 1);
+    assert_ne!("a".is_empty(), false);
+    assert_ne!("".is_empty(), true);
+    assert_ne!(true, "".is_empty());
+
+    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_ne!("a".len(), 1);
+    debug_assert_ne!("a".is_empty(), false);
+    debug_assert_ne!("".is_empty(), true);
+    debug_assert_ne!(true, "".is_empty());
+
+    // 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);
+}
diff --git a/tests/ui/bool_assert_comparison.stderr b/tests/ui/bool_assert_comparison.stderr
@@ -0,0 +1,94 @@
+error: assert macro with a boolean comparison
+  --> $DIR/bool_assert_comparison.rs:5:32
+   |
+LL |     assert_eq!("a".is_empty(), false);
+   |                                ^^^^^ help: replace it with: `assert!(!"a".is_empty())`
+   |
+   = note: `-D clippy::bool-assert-comparison` implied by `-D warnings`
+
+error: assert macro with a boolean comparison
+  --> $DIR/bool_assert_comparison.rs:6:31
+   |
+LL |     assert_eq!("".is_empty(), true);
+   |                               ^^^^ help: replace it with: `assert!("".is_empty())`
+
+error: assert macro with a boolean comparison
+  --> $DIR/bool_assert_comparison.rs:7:16
+   |
+LL |     assert_eq!(true, "".is_empty());
+   |                ^^^^ help: replace it with: `assert!`
+
+error: assert macro with a boolean comparison
+  --> $DIR/bool_assert_comparison.rs:10:32
+   |
+LL |     assert_ne!("a".is_empty(), false);
+   |                                ^^^^^ help: replace it with: `assert!(!"a".is_empty())`
+
+error: assert macro with a boolean comparison
+  --> $DIR/bool_assert_comparison.rs:11:31
+   |
+LL |     assert_ne!("".is_empty(), true);
+   |                               ^^^^ help: replace it with: `assert!(!"".is_empty())`
+
+error: assert macro with a boolean comparison
+  --> $DIR/bool_assert_comparison.rs:12:16
+   |
+LL |     assert_ne!(true, "".is_empty());
+   |                ^^^^ help: replace it with: `assert!`
+
+error: assert macro with a boolean comparison
+  --> $DIR/bool_assert_comparison.rs:15:38
+   |
+LL |     debug_assert_eq!("a".is_empty(), false);
+   |                                      ^^^^^ help: replace it with: `debug_assert!(!"a".is_empty())`
+
+error: assert macro with a boolean comparison
+  --> $DIR/bool_assert_comparison.rs:16:37
+   |
+LL |     debug_assert_eq!("".is_empty(), true);
+   |                                     ^^^^ help: replace it with: `debug_assert!("".is_empty())`
+
+error: assert macro with a boolean comparison
+  --> $DIR/bool_assert_comparison.rs:17:22
+   |
+LL |     debug_assert_eq!(true, "".is_empty());
+   |                      ^^^^ help: replace it with: `debug_assert!`
+
+error: assert macro with a boolean comparison
+  --> $DIR/bool_assert_comparison.rs:20:38
+   |
+LL |     debug_assert_ne!("a".is_empty(), false);
+   |                                      ^^^^^ help: replace it with: `debug_assert!(!"a".is_empty())`
+
+error: assert macro with a boolean comparison
+  --> $DIR/bool_assert_comparison.rs:21:37
+   |
+LL |     debug_assert_ne!("".is_empty(), true);
+   |                                     ^^^^ help: replace it with: `debug_assert!(!"".is_empty())`
+
+error: assert macro with a boolean comparison
+  --> $DIR/bool_assert_comparison.rs:22:22
+   |
+LL |     debug_assert_ne!(true, "".is_empty());
+   |                      ^^^^ help: replace it with: `debug_assert!`
+
+error: assert macro with a boolean comparison
+  --> $DIR/bool_assert_comparison.rs:27:32
+   |
+LL |     assert_eq!("a".is_empty(), false, "tadam {}", 1);
+   |                                ^^^^^ help: replace it with: `assert!(!"a".is_empty(), "tadam {}", 1)`
+
+error: assert macro with a boolean comparison
+  --> $DIR/bool_assert_comparison.rs:28:32
+   |
+LL |     assert_eq!("a".is_empty(), false, "tadam {}", true);
+   |                                ^^^^^ help: replace it with: `assert!(!"a".is_empty(), "tadam {}", true)`
+
+error: assert macro with a boolean comparison
+  --> $DIR/bool_assert_comparison.rs:29:16
+   |
+LL |     assert_eq!(false, "a".is_empty(), "tadam {}", true);
+   |                ^^^^^ help: replace it with: `assert!`
+
+error: aborting due to 15 previous errors
+
