Auto merge of #7605 - xordi:issue-7548-fix, r=giraffate

Issue 7548 fix Close #7548 changelog: [`bool_assert_comparison`] fixes should be emitted only in case they are comparing a value of a type that implements the `Not` trait with an output of type `bool` against a boolean literal.
rust-lang · Aug 31, 2021 · 87c375f · 87c375f
2 parents fd30241 + 83f1454
commit 87c375f
Show file tree

Hide file tree

Showing 4 changed files with 169 additions and 48 deletions.
diff --git a/clippy_lints/src/bool_assert_comparison.rs b/clippy_lints/src/bool_assert_comparison.rs
@@ -1,9 +1,11 @@
-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 clippy_utils::{diagnostics::span_lint_and_sugg, higher, is_direct_expn_of, ty::implements_trait};
+use rustc_ast::ast::LitKind;
 use rustc_errors::Applicability;
-use rustc_lint::{EarlyContext, EarlyLintPass};
+use rustc_hir::{Expr, ExprKind, Lit};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::ty;
 use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::symbol::Ident;
 
 declare_clippy_lint! {
     /// ### What it does
@@ -28,45 +30,77 @@ declare_clippy_lint! {
 
 declare_lint_pass!(BoolAssertComparison => [BOOL_ASSERT_COMPARISON]);
 
-fn is_bool_lit(e: &Expr) -> bool {
+fn is_bool_lit(e: &Expr<'_>) -> bool {
     matches!(
         e.kind,
         ExprKind::Lit(Lit {
-            kind: LitKind::Bool(_),
+            node: LitKind::Bool(_),
             ..
         })
     ) && !e.span.from_expansion()
 }
 
-impl EarlyLintPass for BoolAssertComparison {
-    fn check_expr(&mut self, cx: &EarlyContext<'_>, e: &Expr) {
+fn is_impl_not_trait_with_bool_out(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> bool {
+    let ty = cx.typeck_results().expr_ty(e);
+
+    cx.tcx
+        .lang_items()
+        .not_trait()
+        .filter(|trait_id| implements_trait(cx, ty, *trait_id, &[]))
+        .and_then(|trait_id| {
+            cx.tcx.associated_items(trait_id).find_by_name_and_kind(
+                cx.tcx,
+                Ident::from_str("Output"),
+                ty::AssocKind::Type,
+                trait_id,
+            )
+        })
+        .map_or(false, |assoc_item| {
+            let proj = cx.tcx.mk_projection(assoc_item.def_id, cx.tcx.mk_substs_trait(ty, &[]));
+            let nty = cx.tcx.normalize_erasing_regions(cx.param_env, proj);
+
+            nty.is_bool()
+        })
+}
+
+impl<'tcx> LateLintPass<'tcx> for BoolAssertComparison {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx 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 let Some(span) = is_direct_expn_of(expr.span, mac) {
+                if let Some(args) = higher::extract_assert_macro_args(expr) {
+                    if let [a, b, ..] = args[..] {
+                        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;
+                        }
 
-                    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!
+                        if !is_impl_not_trait_with_bool_out(cx, a) || !is_impl_not_trait_with_bool_out(cx, b) {
+                            // 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;
+                        }
+
+                        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;
                     }
-
-                    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
@@ -2115,7 +2115,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_late_pass(|| box bool_assert_comparison::BoolAssertComparison);
     store.register_early_pass(move || box module_style::ModStyle);
     store.register_late_pass(|| box unused_async::UnusedAsync);
     let disallowed_types = conf.disallowed_types.iter().cloned().collect::<FxHashSet<_>>();

diff --git a/tests/ui/bool_assert_comparison.rs b/tests/ui/bool_assert_comparison.rs
@@ -1,5 +1,7 @@
 #![warn(clippy::bool_assert_comparison)]
 
+use std::ops::Not;
+
 macro_rules! a {
     () => {
         true
@@ -11,14 +13,67 @@ macro_rules! b {
     };
 }
 
+// Implements the Not trait but with an output type
+// that's not bool. Should not suggest a rewrite
+#[derive(Debug)]
+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)]
+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 = ImplNotTraitWithoutBool::VariantX(true);
+    let b = ImplNotTraitWithBool;
+
     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_eq!(a, true);
+    assert_eq!(b, true);
 
     assert_ne!("a".len(), 1);
     assert_ne!("a".is_empty(), false);
@@ -27,6 +82,8 @@ fn main() {
     assert_ne!(a!(), b!());
     assert_ne!(a!(), "".is_empty());
     assert_ne!("".is_empty(), b!());
+    assert_ne!(a, true);
+    assert_ne!(b, true);
 
     debug_assert_eq!("a".len(), 1);
     debug_assert_eq!("a".is_empty(), false);
@@ -35,6 +92,8 @@ fn main() {
     debug_assert_eq!(a!(), b!());
     debug_assert_eq!(a!(), "".is_empty());
     debug_assert_eq!("".is_empty(), b!());
+    debug_assert_eq!(a, true);
+    debug_assert_eq!(b, true);
 
     debug_assert_ne!("a".len(), 1);
     debug_assert_ne!("a".is_empty(), false);
@@ -43,17 +102,21 @@ fn main() {
     debug_assert_ne!(a!(), b!());
     debug_assert_ne!(a!(), "".is_empty());
     debug_assert_ne!("".is_empty(), b!());
+    debug_assert_ne!(a, true);
+    debug_assert_ne!(b, true);
 
     // 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);
+    assert_eq!(a, true, "tadam {}", false);
 
     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);
+    debug_assert_eq!(a, true, "tadam {}", false);
 }
diff --git a/tests/ui/bool_assert_comparison.stderr b/tests/ui/bool_assert_comparison.stderr
@@ -1,112 +1,136 @@
 error: used `assert_eq!` with a literal bool
-  --> $DIR/bool_assert_comparison.rs:16:5
+  --> $DIR/bool_assert_comparison.rs:69: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
+  --> $DIR/bool_assert_comparison.rs:70: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
+  --> $DIR/bool_assert_comparison.rs:71:5
    |
 LL |     assert_eq!(true, "".is_empty());
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)`
 
+error: used `assert_eq!` with a literal bool
+  --> $DIR/bool_assert_comparison.rs:76:5
+   |
+LL |     assert_eq!(b, true);
+   |     ^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)`
+
 error: used `assert_ne!` with a literal bool
-  --> $DIR/bool_assert_comparison.rs:24:5
+  --> $DIR/bool_assert_comparison.rs:79: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
+  --> $DIR/bool_assert_comparison.rs:80: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
+  --> $DIR/bool_assert_comparison.rs:81:5
    |
 LL |     assert_ne!(true, "".is_empty());
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)`
 
+error: used `assert_ne!` with a literal bool
+  --> $DIR/bool_assert_comparison.rs:86:5
+   |
+LL |     assert_ne!(b, true);
+   |     ^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)`
+
 error: used `debug_assert_eq!` with a literal bool
-  --> $DIR/bool_assert_comparison.rs:32:5
+  --> $DIR/bool_assert_comparison.rs:89: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
+  --> $DIR/bool_assert_comparison.rs:90: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
+  --> $DIR/bool_assert_comparison.rs:91:5
    |
 LL |     debug_assert_eq!(true, "".is_empty());
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)`
 
+error: used `debug_assert_eq!` with a literal bool
+  --> $DIR/bool_assert_comparison.rs:96:5
+   |
+LL |     debug_assert_eq!(b, true);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)`
+
 error: used `debug_assert_ne!` with a literal bool
-  --> $DIR/bool_assert_comparison.rs:40:5
+  --> $DIR/bool_assert_comparison.rs:99: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
+  --> $DIR/bool_assert_comparison.rs:100: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
+  --> $DIR/bool_assert_comparison.rs:101:5
    |
 LL |     debug_assert_ne!(true, "".is_empty());
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)`
 
+error: used `debug_assert_ne!` with a literal bool
+  --> $DIR/bool_assert_comparison.rs:106:5
+   |
+LL |     debug_assert_ne!(b, true);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)`
+
 error: used `assert_eq!` with a literal bool
-  --> $DIR/bool_assert_comparison.rs:50:5
+  --> $DIR/bool_assert_comparison.rs:111: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
+  --> $DIR/bool_assert_comparison.rs:112: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
+  --> $DIR/bool_assert_comparison.rs:113: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
+  --> $DIR/bool_assert_comparison.rs:118: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
+  --> $DIR/bool_assert_comparison.rs:119: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
+  --> $DIR/bool_assert_comparison.rs:120:5
    |
 LL |     debug_assert_eq!(false, "a".is_empty(), "tadam {}", true);
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)`
 
-error: aborting due to 18 previous errors
+error: aborting due to 22 previous errors