diff --git a/clippy_lints/src/bool_assert_comparison.rs b/clippy_lints/src/bool_assert_comparison.rs index 8d3f68565..cdc192a47 100644 --- 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! + 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 !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 index be9777618..5987f9e5b 100644 --- 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::>(); diff --git a/tests/ui/bool_assert_comparison.rs b/tests/ui/bool_assert_comparison.rs index 2de402fae..ec4d6f3ff 100644 --- 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,7 +13,58 @@ 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 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 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); @@ -19,6 +72,8 @@ fn main() { 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,6 +102,8 @@ 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); @@ -50,10 +111,12 @@ fn main() { 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 index f57acf520..da9b56aa7 100644 --- a/tests/ui/bool_assert_comparison.stderr +++ b/tests/ui/bool_assert_comparison.stderr @@ -1,5 +1,5 @@ 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!(..)` @@ -7,106 +7,130 @@ LL | assert_eq!("a".is_empty(), false); = 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