Auto merge of rust-lang#10218 - Alexendoo:bool-assert-comparison-sugg…

…estion, r=dswij

Add machine applicable suggestion for `bool_assert_comparison`

Fixes rust-lang#7598
Also closes rust-lang#8118, it had already been fixed by an earlier change but I've added a test for it

changelog: [`bool_assert_comparison`] The suggestion is now machine applicable

clippy_lints/src/bool_assert_comparison.rs

@@ -1,10 +1,11 @@
+use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::macros::{find_assert_eq_args, root_macro_call_first_node};
-use clippy_utils::{diagnostics::span_lint_and_sugg, ty::implements_trait};
+use clippy_utils::ty::{implements_trait, is_copy};
 use rustc_ast::ast::LitKind;
 use rustc_errors::Applicability;
 use rustc_hir::{Expr, ExprKind, Lit};
-use rustc_lint::{LateContext, LateLintPass};
-use rustc_middle::ty;
+use rustc_lint::{LateContext, LateLintPass, LintContext};
+use rustc_middle::ty::{self, Ty};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 use rustc_span::symbol::Ident;
 
@@ -43,9 +44,7 @@ fn is_bool_lit(e: &Expr<'_>) -> bool {
     ) && !e.span.from_expansion()
 }
 
-fn is_impl_not_trait_with_bool_out(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
-    let ty = cx.typeck_results().expr_ty(e);
-
+fn is_impl_not_trait_with_bool_out<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
     cx.tcx
         .lang_items()
         .not_trait()
@@ -77,31 +76,57 @@ impl<'tcx> LateLintPass<'tcx> for BoolAssertComparison {
             return;
         }
         let Some ((a, b, _)) = find_assert_eq_args(cx, expr, macro_call.expn) else { return };
-        if !(is_bool_lit(a) ^ is_bool_lit(b)) {
+
+        let a_span = a.span.source_callsite();
+        let b_span = b.span.source_callsite();
+
+        let (lit_span, non_lit_expr) = match (is_bool_lit(a), is_bool_lit(b)) {
+            // assert_eq!(true, b)
+            //            ^^^^^^
+            (true, false) => (a_span.until(b_span), b),
+            // assert_eq!(a, true)
+            //             ^^^^^^
+            (false, true) => (b_span.with_lo(a_span.hi()), a),
             // 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;
-        }
+            _ => return,
+        };
 
-        if !is_impl_not_trait_with_bool_out(cx, a) || !is_impl_not_trait_with_bool_out(cx, b) {
+        let non_lit_ty = cx.typeck_results().expr_ty(non_lit_expr);
+
+        if !is_impl_not_trait_with_bool_out(cx, non_lit_ty) {
             // At this point the expression which is not a boolean
             // literal does not implement Not trait with a bool output,
             // so we cannot suggest to rewrite our code
             return;
         }
 
+        if !is_copy(cx, non_lit_ty) {
+            // Only lint with types that are `Copy` because `assert!(x)` takes
+            // ownership of `x` whereas `assert_eq(x, true)` does not
+            return;
+        }
+
         let macro_name = macro_name.as_str();
         let non_eq_mac = &macro_name[..macro_name.len() - 3];
-        span_lint_and_sugg(
+        span_lint_and_then(
             cx,
             BOOL_ASSERT_COMPARISON,
             macro_call.span,
             &format!("used `{macro_name}!` with a literal bool"),
-            "replace it with",
-            format!("{non_eq_mac}!(..)"),
-            Applicability::MaybeIncorrect,
+            |diag| {
+                // assert_eq!(...)
+                // ^^^^^^^^^
+                let name_span = cx.sess().source_map().span_until_char(macro_call.span, '!');
+
+                diag.multipart_suggestion(
+                    format!("replace it with `{non_eq_mac}!(..)`"),
+                    vec![(name_span, non_eq_mac.to_string()), (lit_span, String::new())],
+                    Applicability::MachineApplicable,
+                );
+            },
         );
     }
 }

tests/ui/bool_assert_comparison.fixed

@@ -0,0 +1,161 @@
+// run-rustfix
+
+#![allow(unused, clippy::assertions_on_constants)]
+#![warn(clippy::bool_assert_comparison)]
+
+use std::ops::Not;
+
+macro_rules! a {
+    () => {
+        true
+    };
+}
+macro_rules! b {
+    () => {
+        true
+    };
+}
+
+// Implements the Not trait but with an output type
+// that's not bool. Should not suggest a rewrite
+#[derive(Debug, Clone, Copy)]
+enum ImplNotTraitWithoutBool {
+    VariantX(bool),
+    VariantY(u32),
+}
+
+impl PartialEq<bool> for ImplNotTraitWithoutBool {
+    fn eq(&self, other: &bool) -> bool {
+        match *self {
+            ImplNotTraitWithoutBool::VariantX(b) => b == *other,
+            _ => false,
+        }
+    }
+}
+
+impl Not for ImplNotTraitWithoutBool {
+    type Output = Self;
+
+    fn not(self) -> Self::Output {
+        match self {
+            ImplNotTraitWithoutBool::VariantX(b) => ImplNotTraitWithoutBool::VariantX(!b),
+            ImplNotTraitWithoutBool::VariantY(0) => ImplNotTraitWithoutBool::VariantY(1),
+            ImplNotTraitWithoutBool::VariantY(_) => ImplNotTraitWithoutBool::VariantY(0),
+        }
+    }
+}
+
+// This type implements the Not trait with an Output of
+// type bool. Using assert!(..) must be suggested
+#[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
+    }
+}
+
+#[derive(Debug)]
+struct NonCopy;
+
+impl PartialEq<bool> for NonCopy {
+    fn eq(&self, other: &bool) -> bool {
+        false
+    }
+}
+
+impl Not for NonCopy {
+    type Output = bool;
+
+    fn not(self) -> Self::Output {
+        true
+    }
+}
+
+fn main() {
+    let a = ImplNotTraitWithoutBool::VariantX(true);
+    let b = ImplNotTraitWithBool;
+
+    assert_eq!("a".len(), 1);
+    assert!("a".is_empty());
+    assert!("".is_empty());
+    assert!("".is_empty());
+    assert_eq!(a!(), b!());
+    assert_eq!(a!(), "".is_empty());
+    assert_eq!("".is_empty(), b!());
+    assert_eq!(a, true);
+    assert!(b);
+
+    assert_ne!("a".len(), 1);
+    assert!("a".is_empty());
+    assert!("".is_empty());
+    assert!("".is_empty());
+    assert_ne!(a!(), b!());
+    assert_ne!(a!(), "".is_empty());
+    assert_ne!("".is_empty(), b!());
+    assert_ne!(a, true);
+    assert!(b);
+
+    debug_assert_eq!("a".len(), 1);
+    debug_assert!("a".is_empty());
+    debug_assert!("".is_empty());
+    debug_assert!("".is_empty());
+    debug_assert_eq!(a!(), b!());
+    debug_assert_eq!(a!(), "".is_empty());
+    debug_assert_eq!("".is_empty(), b!());
+    debug_assert_eq!(a, true);
+    debug_assert!(b);
+
+    debug_assert_ne!("a".len(), 1);
+    debug_assert!("a".is_empty());
+    debug_assert!("".is_empty());
+    debug_assert!("".is_empty());
+    debug_assert_ne!(a!(), b!());
+    debug_assert_ne!(a!(), "".is_empty());
+    debug_assert_ne!("".is_empty(), b!());
+    debug_assert_ne!(a, true);
+    debug_assert!(b);
+
+    // assert with error messages
+    assert_eq!("a".len(), 1, "tadam {}", 1);
+    assert_eq!("a".len(), 1, "tadam {}", true);
+    assert!("a".is_empty(), "tadam {}", 1);
+    assert!("a".is_empty(), "tadam {}", true);
+    assert!("a".is_empty(), "tadam {}", true);
+    assert_eq!(a, true, "tadam {}", false);
+
+    debug_assert_eq!("a".len(), 1, "tadam {}", 1);
+    debug_assert_eq!("a".len(), 1, "tadam {}", true);
+    debug_assert!("a".is_empty(), "tadam {}", 1);
+    debug_assert!("a".is_empty(), "tadam {}", true);
+    debug_assert!("a".is_empty(), "tadam {}", true);
+    debug_assert_eq!(a, true, "tadam {}", false);
+
+    assert!(a!());
+    assert!(b!());
+
+    use debug_assert_eq as renamed;
+    renamed!(a, true);
+    debug_assert!(b);
+
+    let non_copy = NonCopy;
+    assert_eq!(non_copy, true);
+    // changing the above to `assert!(non_copy)` would cause a `borrow of moved value`
+    println!("{non_copy:?}");
+
+    macro_rules! in_macro {
+        ($v:expr) => {{
+            assert_eq!($v, true);
+        }};
+    }
+    in_macro!(a);
+}

tests/ui/bool_assert_comparison.rs

@@ -1,3 +1,6 @@
+// run-rustfix
+
+#![allow(unused, clippy::assertions_on_constants)]
 #![warn(clippy::bool_assert_comparison)]
 
 use std::ops::Not;
@@ -15,7 +18,7 @@ macro_rules! b {
 
 // Implements the Not trait but with an output type
 // that's not bool. Should not suggest a rewrite
-#[derive(Debug)]
+#[derive(Debug, Clone, Copy)]
 enum ImplNotTraitWithoutBool {
     VariantX(bool),
     VariantY(u32),
@@ -44,7 +47,7 @@ impl Not for ImplNotTraitWithoutBool {
 
 // This type implements the Not trait with an Output of
 // type bool. Using assert!(..) must be suggested
-#[derive(Debug)]
+#[derive(Debug, Clone, Copy)]
 struct ImplNotTraitWithBool;
 
 impl PartialEq<bool> for ImplNotTraitWithBool {
@@ -61,6 +64,23 @@ impl Not for ImplNotTraitWithBool {
     }
 }
 
+#[derive(Debug)]
+struct NonCopy;
+
+impl PartialEq<bool> for NonCopy {
+    fn eq(&self, other: &bool) -> bool {
+        false
+    }
+}
+
+impl Not for NonCopy {
+    type Output = bool;
+
+    fn not(self) -> Self::Output {
+        true
+    }
+}
+
 fn main() {
     let a = ImplNotTraitWithoutBool::VariantX(true);
     let b = ImplNotTraitWithBool;
@@ -119,4 +139,23 @@ fn main() {
     debug_assert_eq!("a".is_empty(), false, "tadam {}", true);
     debug_assert_eq!(false, "a".is_empty(), "tadam {}", true);
     debug_assert_eq!(a, true, "tadam {}", false);
+
+    assert_eq!(a!(), true);
+    assert_eq!(true, b!());
+
+    use debug_assert_eq as renamed;
+    renamed!(a, true);
+    renamed!(b, true);
+
+    let non_copy = NonCopy;
+    assert_eq!(non_copy, true);
+    // changing the above to `assert!(non_copy)` would cause a `borrow of moved value`
+    println!("{non_copy:?}");
+
+    macro_rules! in_macro {
+        ($v:expr) => {{
+            assert_eq!($v, true);
+        }};
+    }
+    in_macro!(a);
 }