Auto merge of rust-lang#10293 - Alexendoo:bool-assert-comparison-negation, r=dswij

Negate suggestions when needed in `bool_assert_comparison`

changelog: none assuming this gets into the same release as rust-lang#10218

Fixes rust-lang#10291

r? `@dswij`

Thanks to `@black-puppydog` for spotting it early

Author: bors

clippy_lints/src/bool_assert_comparison.rs

@@ -1,5 +1,6 @@
 use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::macros::{find_assert_eq_args, root_macro_call_first_node};
+use clippy_utils::sugg::Sugg;
 use clippy_utils::ty::{implements_trait, is_copy};
 use rustc_ast::ast::LitKind;
 use rustc_errors::Applicability;
@@ -34,14 +35,16 @@ declare_clippy_lint! {
 
 declare_lint_pass!(BoolAssertComparison => [BOOL_ASSERT_COMPARISON]);
 
-fn is_bool_lit(e: &Expr<'_>) -> bool {
-    matches!(
-        e.kind,
-        ExprKind::Lit(Lit {
-            node: LitKind::Bool(_),
-            ..
-        })
-    ) && !e.span.from_expansion()
+fn extract_bool_lit(e: &Expr<'_>) -> Option<bool> {
+    if let ExprKind::Lit(Lit {
+        node: LitKind::Bool(b), ..
+    }) = e.kind
+        && !e.span.from_expansion()
+    {
+        Some(b)
+    } else {
+        None
+    }
 }
 
 fn is_impl_not_trait_with_bool_out<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
@@ -69,24 +72,23 @@ impl<'tcx> LateLintPass<'tcx> for BoolAssertComparison {
     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_eq" | "debug_assert_eq" | "assert_ne" | "debug_assert_ne"
-        ) {
-            return;
-        }
+        let eq_macro = match macro_name.as_str() {
+            "assert_eq" | "debug_assert_eq" => true,
+            "assert_ne" | "debug_assert_ne" => false,
+            _ => return,
+        };
         let Some ((a, b, _)) = find_assert_eq_args(cx, expr, macro_call.expn) else { return };
 
         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),
+        let (lit_span, bool_value, non_lit_expr) = match (extract_bool_lit(a), extract_bool_lit(b)) {
+            // assert_eq!(true/false, b)
+            //            ^^^^^^^^^^^^
+            (Some(bool_value), None) => (a_span.until(b_span), bool_value, b),
+            // assert_eq!(a, true/false)
+            //             ^^^^^^^^^^^^
+            (None, Some(bool_value)) => (b_span.with_lo(a_span.hi()), bool_value, a),
             // If there are two boolean arguments, we definitely don't understand
             // what's going on, so better leave things as is...
             //
@@ -121,9 +123,16 @@ impl<'tcx> LateLintPass<'tcx> for BoolAssertComparison {
                 // ^^^^^^^^^
                 let name_span = cx.sess().source_map().span_until_char(macro_call.span, '!');
 
+                let mut suggestions = vec![(name_span, non_eq_mac.to_string()), (lit_span, String::new())];
+
+                if bool_value ^ eq_macro {
+                    let Some(sugg) = Sugg::hir_opt(cx, non_lit_expr) else { return };
+                    suggestions.push((non_lit_expr.span, (!sugg).to_string()));
+                }
+
                 diag.multipart_suggestion(
                     format!("replace it with `{non_eq_mac}!(..)`"),
-                    vec![(name_span, non_eq_mac.to_string()), (lit_span, String::new())],
+                    suggestions,
                     Applicability::MachineApplicable,
                 );
             },

tests/ui/bool_assert_comparison.fixed

@@ -86,7 +86,7 @@ fn main() {
     let b = ImplNotTraitWithBool;
 
     assert_eq!("a".len(), 1);
-    assert!("a".is_empty());
+    assert!(!"a".is_empty());
     assert!("".is_empty());
     assert!("".is_empty());
     assert_eq!(a!(), b!());
@@ -97,16 +97,16 @@ fn main() {
 
     assert_ne!("a".len(), 1);
     assert!("a".is_empty());
-    assert!("".is_empty());
-    assert!("".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);
+    assert!(!b);
 
     debug_assert_eq!("a".len(), 1);
-    debug_assert!("a".is_empty());
+    debug_assert!(!"a".is_empty());
     debug_assert!("".is_empty());
     debug_assert!("".is_empty());
     debug_assert_eq!(a!(), b!());
@@ -117,27 +117,27 @@ fn main() {
 
     debug_assert_ne!("a".len(), 1);
     debug_assert!("a".is_empty());
-    debug_assert!("".is_empty());
-    debug_assert!("".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);
+    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!(!"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!(!"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!());
@@ -158,4 +158,14 @@ fn main() {
         }};
     }
     in_macro!(a);
+
+    assert!("".is_empty());
+    assert!("".is_empty());
+    assert!(!"requires negation".is_empty());
+    assert!(!"requires negation".is_empty());
+
+    debug_assert!("".is_empty());
+    debug_assert!("".is_empty());
+    debug_assert!(!"requires negation".is_empty());
+    debug_assert!(!"requires negation".is_empty());
 }

tests/ui/bool_assert_comparison.rs

@@ -158,4 +158,14 @@ fn main() {
         }};
     }
     in_macro!(a);
+
+    assert_eq!("".is_empty(), true);
+    assert_ne!("".is_empty(), false);
+    assert_ne!("requires negation".is_empty(), true);
+    assert_eq!("requires negation".is_empty(), false);
+
+    debug_assert_eq!("".is_empty(), true);
+    debug_assert_ne!("".is_empty(), false);
+    debug_assert_ne!("requires negation".is_empty(), true);
+    debug_assert_eq!("requires negation".is_empty(), false);
 }