Add new restriction lint on assert!(!..)

Author: couchand

CHANGELOG.md

@@ -3057,6 +3057,7 @@ Released 2018-09-13
 [`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_assert_inverted`]: https://rust-lang.github.io/rust-clippy/master/index.html#bool_assert_inverted
 [`bool_comparison`]: https://rust-lang.github.io/rust-clippy/master/index.html#bool_comparison
 [`borrow_as_ptr`]: https://rust-lang.github.io/rust-clippy/master/index.html#borrow_as_ptr
 [`borrow_interior_mutable_const`]: https://rust-lang.github.io/rust-clippy/master/index.html#borrow_interior_mutable_const

clippy_lints/src/bool_assert_comparison.rs

@@ -26,7 +26,7 @@ declare_clippy_lint! {
     /// ```
     #[clippy::version = "1.53.0"]
     pub BOOL_ASSERT_COMPARISON,
-    style,
+    pedantic,
     "Using a boolean as comparison value in an assert_* macro when there is no need"
 }
 

clippy_lints/src/bool_assert_inverted.rs

@@ -0,0 +1,64 @@
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::macros::{find_assert_args, root_macro_call_first_node};
+use rustc_errors::Applicability;
+use rustc_hir::{Expr, ExprKind, UnOp};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// This lint warns about the use of inverted conditions in assert-like macros.
+    ///
+    /// ### Why is this bad?
+    /// It is all too easy to misread the semantics of an assertion when the
+    /// logic of the condition is reversed.  Explicitly comparing to a boolean
+    /// value is preferable.
+    ///
+    /// ### Example
+    /// ```rust
+    /// // Bad
+    /// assert!(!"a".is_empty());
+    ///
+    /// // Good
+    /// assert_eq!("a".is_empty(), false);
+    ///
+    /// // Okay
+    /// assert_ne!("a".is_empty(), true);
+    /// ```
+    #[clippy::version = "1.58.0"]
+    pub BOOL_ASSERT_INVERTED,
+    restriction,
+    "Asserting on an inverted condition"
+}
+
+declare_lint_pass!(BoolAssertInverted => [BOOL_ASSERT_INVERTED]);
+
+fn is_inverted(e: &Expr<'_>) -> bool {
+    matches!(e.kind, ExprKind::Unary(UnOp::Not, _),) && !e.span.from_expansion()
+}
+
+impl<'tcx> LateLintPass<'tcx> for BoolAssertInverted {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
+        let Some(macro_call) = root_macro_call_first_node(cx, expr) else { return };
+        let macro_name = cx.tcx.item_name(macro_call.def_id);
+        if !matches!(macro_name.as_str(), "assert" | "debug_assert") {
+            return;
+        }
+        let Some ((a, _)) = find_assert_args(cx, expr, macro_call.expn) else { return };
+        if !is_inverted(a) {
+            return;
+        }
+
+        let macro_name = macro_name.as_str();
+        let eq_mac = format!("{}_eq", macro_name);
+        span_lint_and_sugg(
+            cx,
+            BOOL_ASSERT_INVERTED,
+            macro_call.span,
+            &format!("used `{}!` with an inverted condition", macro_name),
+            "replace it with",
+            format!("{}!(.., false, ..)", eq_mac),
+            Applicability::MaybeIncorrect,
+        );
+    }
+}

clippy_lints/src/lib.register_all.rs

@@ -18,7 +18,6 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
     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),

clippy_lints/src/lib.register_lints.rs

@@ -57,6 +57,7 @@ store.register_lints(&[
     blacklisted_name::BLACKLISTED_NAME,
     blocks_in_if_conditions::BLOCKS_IN_IF_CONDITIONS,
     bool_assert_comparison::BOOL_ASSERT_COMPARISON,
+    bool_assert_inverted::BOOL_ASSERT_INVERTED,
     booleans::LOGIC_BUG,
     booleans::NONMINIMAL_BOOL,
     borrow_as_ptr::BORROW_AS_PTR,

clippy_lints/src/lib.register_pedantic.rs

@@ -7,6 +7,7 @@ store.register_group(true, "clippy::pedantic", Some("clippy_pedantic"), vec![
     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(borrow_as_ptr::BORROW_AS_PTR),
     LintId::of(bytecount::NAIVE_BYTECOUNT),
     LintId::of(case_sensitive_file_extension_comparisons::CASE_SENSITIVE_FILE_EXTENSION_COMPARISONS),

clippy_lints/src/lib.register_restriction.rs

@@ -8,6 +8,7 @@ store.register_group(true, "clippy::restriction", Some("clippy_restriction"), ve
     LintId::of(as_conversions::AS_CONVERSIONS),
     LintId::of(asm_syntax::INLINE_ASM_X86_ATT_SYNTAX),
     LintId::of(asm_syntax::INLINE_ASM_X86_INTEL_SYNTAX),
+    LintId::of(bool_assert_inverted::BOOL_ASSERT_INVERTED),
     LintId::of(casts::FN_TO_NUMERIC_CAST_ANY),
     LintId::of(create_dir::CREATE_DIR),
     LintId::of(dbg_macro::DBG_MACRO),

clippy_lints/src/lib.register_style.rs

@@ -7,7 +7,6 @@ store.register_group(true, "clippy::style", Some("clippy_style"), vec![
     LintId::of(assign_ops::ASSIGN_OP_PATTERN),
     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),

clippy_lints/src/lib.rs

@@ -175,6 +175,7 @@ mod bit_mask;
 mod blacklisted_name;
 mod blocks_in_if_conditions;
 mod bool_assert_comparison;
+mod bool_assert_inverted;
 mod booleans;
 mod borrow_as_ptr;
 mod bytecount;
@@ -821,6 +822,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| Box::new(manual_map::ManualMap));
     store.register_late_pass(move || Box::new(if_then_some_else_none::IfThenSomeElseNone::new(msrv)));
     store.register_late_pass(|| Box::new(bool_assert_comparison::BoolAssertComparison));
+    store.register_late_pass(|| Box::new(bool_assert_inverted::BoolAssertInverted));
     store.register_early_pass(move || Box::new(module_style::ModStyle));
     store.register_late_pass(|| Box::new(unused_async::UnusedAsync));
     let disallowed_types = conf.disallowed_types.clone();

tests/ui/bool_assert_inverted.rs

@@ -0,0 +1,55 @@
+#![warn(clippy::bool_assert_inverted)]
+
+use std::ops::Not;
+
+macro_rules! a {
+    () => {
+        true
+    };
+}
+macro_rules! b {
+    () => {
+        true
+    };
+}
+
+#[derive(Debug, Clone, Copy)]
+struct ImplNotTraitWithBool;
+
+impl PartialEq<bool> for ImplNotTraitWithBool {
+    fn eq(&self, other: &bool) -> bool {
+        false
+    }
+}
+
+impl Not for ImplNotTraitWithBool {
+    type Output = bool;
+
+    fn not(self) -> Self::Output {
+        true
+    }
+}
+
+fn main() {
+    let a = ImplNotTraitWithBool;
+
+    assert!(!"a".is_empty());
+    assert!("".is_empty());
+    assert!(!a);
+    assert!(a);
+
+    debug_assert!(!"a".is_empty());
+    debug_assert!("".is_empty());
+    debug_assert!(!a);
+    debug_assert!(a);
+
+    assert!(!"a".is_empty(), "tadam {}", false);
+    assert!("".is_empty(), "tadam {}", false);
+    assert!(!a, "tadam {}", false);
+    assert!(a, "tadam {}", false);
+
+    debug_assert!(!"a".is_empty(), "tadam {}", false);
+    debug_assert!("".is_empty(), "tadam {}", false);
+    debug_assert!(!a, "tadam {}", false);
+    debug_assert!(a, "tadam {}", false);
+}

tests/ui/bool_assert_inverted.stderr

@@ -0,0 +1,28 @@
+error: used `debug_assert!` with an inverted condition
+  --> $DIR/bool_assert_inverted.rs:41:5
+   |
+LL |     debug_assert!(!"a".is_empty());
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert_eq!(.., false, ..)`
+   |
+   = note: `-D clippy::bool-assert-inverted` implied by `-D warnings`
+
+error: used `debug_assert!` with an inverted condition
+  --> $DIR/bool_assert_inverted.rs:43:5
+   |
+LL |     debug_assert!(!a);
+   |     ^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert_eq!(.., false, ..)`
+
+error: used `debug_assert!` with an inverted condition
+  --> $DIR/bool_assert_inverted.rs:51:5
+   |
+LL |     debug_assert!(!"a".is_empty(), "tadam {}", false);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert_eq!(.., false, ..)`
+
+error: used `debug_assert!` with an inverted condition
+  --> $DIR/bool_assert_inverted.rs:53:5
+   |
+LL |     debug_assert!(!a, "tadam {}", false);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert_eq!(.., false, ..)`
+
+error: aborting due to 4 previous errors
+