From f75d1b2bc5b5794ccc43925f36900d404211eeda Mon Sep 17 00:00:00 2001 From: xordi Date: Thu, 26 Aug 2021 16:14:37 +0200 Subject: [PATCH 1/4] Check for Not trait implementation --- clippy_lints/src/bool_assert_comparison.rs | 90 +++++++++++++++------- clippy_lints/src/lib.rs | 2 +- 2 files changed, 63 insertions(+), 29 deletions(-) diff --git a/clippy_lints/src/bool_assert_comparison.rs b/clippy_lints/src/bool_assert_comparison.rs index 8d3f68565b22..171d22f99866 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::*; +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 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(|id| implements_trait(cx, ty, *id, &[])) + .and_then(|id| { + cx.tcx.associated_items(id).find_by_name_and_kind( + cx.tcx, + Ident::from_str("Output"), + ty::AssocKind::Type, + id, + ) + }) + .map_or(false, |item| { + let proj = cx.tcx.mk_projection(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 !impl_not_trait_with_bool_out(cx, a) || !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 be97776187ff..5987f9e5b0cb 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::>(); From abbcea86ffc5972f4489d55afed39390ec68f670 Mon Sep 17 00:00:00 2001 From: xordi Date: Thu, 26 Aug 2021 16:15:04 +0200 Subject: [PATCH 2/4] Add new ui tests --- tests/ui/bool_assert_comparison.rs | 63 ++++++++++++++++++++++++++ tests/ui/bool_assert_comparison.stderr | 62 +++++++++++++++++-------- 2 files changed, 106 insertions(+), 19 deletions(-) diff --git a/tests/ui/bool_assert_comparison.rs b/tests/ui/bool_assert_comparison.rs index 2de402fae8c7..3961b9432a30 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 A { + VariantX(bool), + VariantY(u32), +} + +impl PartialEq for A { + fn eq(&self, other: &bool) -> bool { + match *self { + A::VariantX(b) => b == *other, + _ => false, + } + } +} + +impl Not for A { + type Output = Self; + + fn not(self) -> Self::Output { + match self { + A::VariantX(b) => A::VariantX(!b), + A::VariantY(0) => A::VariantY(1), + A::VariantY(_) => A::VariantY(0), + } + } +} + +// This type implements the Not trait with an Output of +// type bool. Using assert!(..) must be suggested +#[derive(Debug)] +struct B; + +impl PartialEq for B { + fn eq(&self, other: &bool) -> bool { + false + } +} + +impl Not for B { + type Output = bool; + + fn not(self) -> Self::Output { + true + } +} + fn main() { + let a = A::VariantX(true); + let b = B {}; + 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 f57acf520d5f..da9b56aa7795 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 From aee4f1fc0cae5ac2c044e4f1b6ff015bbb9405b4 Mon Sep 17 00:00:00 2001 From: xordi Date: Thu, 26 Aug 2021 18:18:17 +0200 Subject: [PATCH 3/4] Fix CI errors --- clippy_lints/src/bool_assert_comparison.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/bool_assert_comparison.rs b/clippy_lints/src/bool_assert_comparison.rs index 171d22f99866..2f8f61f7d33a 100644 --- a/clippy_lints/src/bool_assert_comparison.rs +++ b/clippy_lints/src/bool_assert_comparison.rs @@ -1,7 +1,7 @@ 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_hir::*; +use rustc_hir::{Expr, ExprKind, Lit}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty; use rustc_session::{declare_lint_pass, declare_tool_lint}; From 83f1454ade1bfa9a797b4bdccd8bd2432c110641 Mon Sep 17 00:00:00 2001 From: xordi Date: Tue, 31 Aug 2021 09:06:14 +0200 Subject: [PATCH 4/4] Fix function and variable names --- clippy_lints/src/bool_assert_comparison.rs | 16 +++++++-------- tests/ui/bool_assert_comparison.rs | 24 +++++++++++----------- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/clippy_lints/src/bool_assert_comparison.rs b/clippy_lints/src/bool_assert_comparison.rs index 2f8f61f7d33a..cdc192a47e48 100644 --- a/clippy_lints/src/bool_assert_comparison.rs +++ b/clippy_lints/src/bool_assert_comparison.rs @@ -40,23 +40,23 @@ fn is_bool_lit(e: &Expr<'_>) -> bool { ) && !e.span.from_expansion() } -fn impl_not_trait_with_bool_out(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> bool { +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(|id| implements_trait(cx, ty, *id, &[])) - .and_then(|id| { - cx.tcx.associated_items(id).find_by_name_and_kind( + .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, - id, + trait_id, ) }) - .map_or(false, |item| { - let proj = cx.tcx.mk_projection(item.def_id, cx.tcx.mk_substs_trait(ty, &[])); + .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() @@ -82,7 +82,7 @@ impl<'tcx> LateLintPass<'tcx> for BoolAssertComparison { return; } - if !impl_not_trait_with_bool_out(cx, a) || !impl_not_trait_with_bool_out(cx, b) { + 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 diff --git a/tests/ui/bool_assert_comparison.rs b/tests/ui/bool_assert_comparison.rs index 3961b9432a30..ec4d6f3ff840 100644 --- a/tests/ui/bool_assert_comparison.rs +++ b/tests/ui/bool_assert_comparison.rs @@ -16,28 +16,28 @@ macro_rules! b { // Implements the Not trait but with an output type // that's not bool. Should not suggest a rewrite #[derive(Debug)] -enum A { +enum ImplNotTraitWithoutBool { VariantX(bool), VariantY(u32), } -impl PartialEq for A { +impl PartialEq for ImplNotTraitWithoutBool { fn eq(&self, other: &bool) -> bool { match *self { - A::VariantX(b) => b == *other, + ImplNotTraitWithoutBool::VariantX(b) => b == *other, _ => false, } } } -impl Not for A { +impl Not for ImplNotTraitWithoutBool { type Output = Self; fn not(self) -> Self::Output { match self { - A::VariantX(b) => A::VariantX(!b), - A::VariantY(0) => A::VariantY(1), - A::VariantY(_) => A::VariantY(0), + ImplNotTraitWithoutBool::VariantX(b) => ImplNotTraitWithoutBool::VariantX(!b), + ImplNotTraitWithoutBool::VariantY(0) => ImplNotTraitWithoutBool::VariantY(1), + ImplNotTraitWithoutBool::VariantY(_) => ImplNotTraitWithoutBool::VariantY(0), } } } @@ -45,15 +45,15 @@ impl Not for A { // This type implements the Not trait with an Output of // type bool. Using assert!(..) must be suggested #[derive(Debug)] -struct B; +struct ImplNotTraitWithBool; -impl PartialEq for B { +impl PartialEq for ImplNotTraitWithBool { fn eq(&self, other: &bool) -> bool { false } } -impl Not for B { +impl Not for ImplNotTraitWithBool { type Output = bool; fn not(self) -> Self::Output { @@ -62,8 +62,8 @@ impl Not for B { } fn main() { - let a = A::VariantX(true); - let b = B {}; + let a = ImplNotTraitWithoutBool::VariantX(true); + let b = ImplNotTraitWithBool; assert_eq!("a".len(), 1); assert_eq!("a".is_empty(), false);