diff --git a/clippy_lints/src/bool_assert_comparison.rs b/clippy_lints/src/bool_assert_comparison.rs index db5792188dd4..b5cb2c8d4ec6 100644 --- a/clippy_lints/src/bool_assert_comparison.rs +++ b/clippy_lints/src/bool_assert_comparison.rs @@ -1,14 +1,17 @@ use clippy_utils::diagnostics::span_lint_and_then; -use clippy_utils::macros::{find_assert_eq_args, root_macro_call_first_node}; +use clippy_utils::macros::{find_assert_args, find_assert_eq_args, root_macro_call_first_node, MacroCall}; use clippy_utils::sugg::Sugg; use clippy_utils::ty::{implements_trait, is_copy}; use rustc_ast::ast::LitKind; +use rustc_ast::BinOpKind; use rustc_errors::Applicability; use rustc_hir::{Expr, ExprKind, Lit}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::ty::{self, Ty}; use rustc_session::declare_lint_pass; +use rustc_span::source_map::Spanned; use rustc_span::symbol::Ident; +use rustc_span::Span; declare_clippy_lint! { /// ### What it does @@ -74,74 +77,115 @@ impl<'tcx> LateLintPass<'tcx> for BoolAssertComparison { return; }; let macro_name = cx.tcx.item_name(macro_call.def_id); - 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 macro_name = macro_name.as_str(); + match macro_name { + "assert_eq" | "debug_assert_eq" => check_eq(cx, expr, ¯o_call, macro_name, true), + "assert_ne" | "debug_assert_ne" => check_eq(cx, expr, ¯o_call, macro_name, false), + "assert" | "debug_assert" => check(cx, expr, ¯o_call, macro_name), + _ => {}, }; + } +} - let a_span = a.span.source_callsite(); - let b_span = b.span.source_callsite(); - - 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... - // - // Or there is simply no boolean and then we can leave things as is! - _ => return, - }; +fn check_eq<'tcx>( + cx: &LateContext<'tcx>, + expr: &'tcx Expr<'_>, + macro_call: &MacroCall, + macro_name: &str, + eq_macro: bool, +) { + let Some((a, b, _)) = find_assert_eq_args(cx, expr, macro_call.expn) else { + return; + }; - let non_lit_ty = cx.typeck_results().expr_ty(non_lit_expr); + let a_span = a.span.source_callsite(); + let b_span = b.span.source_callsite(); - 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; - } + 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... + // + // Or there is simply no boolean and then we can leave things as is! + _ => 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 non_lit_ty = cx.typeck_results().expr_ty(non_lit_expr); - let macro_name = macro_name.as_str(); - let non_eq_mac = ¯o_name[..macro_name.len() - 3]; - span_lint_and_then( - cx, - BOOL_ASSERT_COMPARISON, - macro_call.span, - format!("used `{macro_name}!` with a literal bool"), - |diag| { - // assert_eq!(...) - // ^^^^^^^^^ - 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}!(..)`"), - suggestions, - Applicability::MachineApplicable, - ); - }, - ); + 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 non_eq_mac = ¯o_name[..macro_name.len() - 3]; + span_lint_and_then( + cx, + BOOL_ASSERT_COMPARISON, + macro_call.span, + format!("used `{macro_name}!` with a literal bool"), + |diag| { + // assert_eq!(...) + // ^^^^^^^^^ + 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}!(..)`"), + suggestions, + Applicability::MachineApplicable, + ); + }, + ); +} + +/// Checks for `assert!(a == b)` and `assert!(a != b)` +fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, macro_call: &MacroCall, macro_name: &str) { + let Some((cond, _)) = find_assert_args(cx, expr, macro_call.expn) else { + return; + }; + let ExprKind::Binary(Spanned { node, span }, lhs, rhs) = cond.kind else { + return; + }; + // TODO: is `cond.span.from_expansion()` correct / needed? + if (node != BinOpKind::Eq && node != BinOpKind::Ne) || cond.span.from_expansion() { + return; } + + let new_name = format!("{macro_name}_{}", if node == BinOpKind::Eq { "eq" } else { "ne" }); + let msg = format!("replace it with `{new_name}!(..)`"); + span_lint_and_then( + cx, + BOOL_ASSERT_COMPARISON, + macro_call.span, + format!("used `{macro_name}!` with an equality comparison"), + |diag| { + let macro_name_span = cx.sess().source_map().span_until_char(macro_call.span, '!'); + // TODO: should this be `cond.span`, expanded to include preceding whitespace? If so, how? + let equality_span = Span::new(lhs.span.hi(), rhs.span.lo(), span.ctxt(), span.parent()); + let suggestions = vec![(macro_name_span, new_name), (equality_span, ", ".to_string())]; + + diag.multipart_suggestion(msg, suggestions, Applicability::MachineApplicable); + }, + ); } diff --git a/clippy_lints/src/eta_reduction.rs b/clippy_lints/src/eta_reduction.rs index cabc65922582..421849f8fffb 100644 --- a/clippy_lints/src/eta_reduction.rs +++ b/clippy_lints/src/eta_reduction.rs @@ -331,7 +331,7 @@ fn has_late_bound_to_non_late_bound_regions(from_sig: FnSig<'_>, to_sig: FnSig<' } } - assert!(from_sig.inputs_and_output.len() == to_sig.inputs_and_output.len()); + assert_eq!(from_sig.inputs_and_output.len(), to_sig.inputs_and_output.len()); from_sig .inputs_and_output .iter() diff --git a/clippy_lints/src/functions/renamed_function_params.rs b/clippy_lints/src/functions/renamed_function_params.rs index c7de0385c021..d9b46018e780 100644 --- a/clippy_lints/src/functions/renamed_function_params.rs +++ b/clippy_lints/src/functions/renamed_function_params.rs @@ -58,7 +58,7 @@ impl RenamedFnArgs { { let mut renamed: Vec<(Span, String)> = vec![]; - debug_assert!(default_names.size_hint() == current_names.size_hint()); + debug_assert_eq!(default_names.size_hint(), current_names.size_hint()); while let (Some(def_name), Some(cur_name)) = (default_names.next(), current_names.next()) { let current_name = cur_name.name; let default_name = def_name.name; diff --git a/clippy_utils/src/ty.rs b/clippy_utils/src/ty.rs index 5756e56c262e..569863516cac 100644 --- a/clippy_utils/src/ty.rs +++ b/clippy_utils/src/ty.rs @@ -1040,8 +1040,9 @@ fn assert_generic_args_match<'tcx>(tcx: TyCtxt<'tcx>, did: DefId, args: &[Generi .chain(&g.own_params) .map(|x| &x.kind); - assert!( - count == args.len(), + assert_eq!( + count, + args.len(), "wrong number of arguments for `{did:?}`: expected `{count}`, found {}\n\ note: the expected arguments are: `[{}]`\n\ the given arguments are: `{args:#?}`", diff --git a/lintcheck/src/output.rs b/lintcheck/src/output.rs index e38036315c2c..6bb4ee735ddd 100644 --- a/lintcheck/src/output.rs +++ b/lintcheck/src/output.rs @@ -228,8 +228,8 @@ fn print_stats(old_stats: HashMap, new_stats: HashMap<&String, us // remove duplicates from both hashmaps for (k, v) in &same_in_both_hashmaps { - assert!(old_stats_deduped.remove(k) == Some(*v)); - assert!(new_stats_deduped.remove(k) == Some(*v)); + assert_eq!(old_stats_deduped.remove(k), Some(*v)); + assert_eq!(new_stats_deduped.remove(k), Some(*v)); } println!("\nStats:"); diff --git a/tests/ui/assertions_on_constants.rs b/tests/ui/assertions_on_constants.rs index 957154e60dec..13b043fcf748 100644 --- a/tests/ui/assertions_on_constants.rs +++ b/tests/ui/assertions_on_constants.rs @@ -1,4 +1,4 @@ -#![allow(non_fmt_panics, clippy::needless_bool, clippy::eq_op)] +#![allow(non_fmt_panics, clippy::needless_bool, clippy::eq_op, clippy::bool_assert_comparison)] macro_rules! assert_const { ($len:expr) => { diff --git a/tests/ui/bool_assert_comparison.fixed b/tests/ui/bool_assert_comparison.fixed index b05166a055ee..4a5b1a199282 100644 --- a/tests/ui/bool_assert_comparison.fixed +++ b/tests/ui/bool_assert_comparison.fixed @@ -166,4 +166,9 @@ fn main() { debug_assert!("".is_empty()); debug_assert!(!"requires negation".is_empty()); debug_assert!(!"requires negation".is_empty()); + + assert_eq!("a", "a".to_ascii_lowercase()); + assert_eq!("a", "a".to_ascii_lowercase(), "a==a"); + assert_ne!("A", "A".to_ascii_lowercase()); + assert_ne!("A", "A".to_ascii_lowercase(), "A!=a"); } diff --git a/tests/ui/bool_assert_comparison.rs b/tests/ui/bool_assert_comparison.rs index dc51fcf1d36b..eb49f77554bf 100644 --- a/tests/ui/bool_assert_comparison.rs +++ b/tests/ui/bool_assert_comparison.rs @@ -166,4 +166,9 @@ fn main() { debug_assert_ne!("".is_empty(), false); debug_assert_ne!("requires negation".is_empty(), true); debug_assert_eq!("requires negation".is_empty(), false); + + assert!("a" == "a".to_ascii_lowercase()); + assert!("a" == "a".to_ascii_lowercase(), "a==a"); + assert!("A" != "A".to_ascii_lowercase()); + assert!("A" != "A".to_ascii_lowercase(), "A!=a"); } diff --git a/tests/ui/bool_assert_comparison.stderr b/tests/ui/bool_assert_comparison.stderr index 41183c61ee01..81a98181553e 100644 --- a/tests/ui/bool_assert_comparison.stderr +++ b/tests/ui/bool_assert_comparison.stderr @@ -396,5 +396,49 @@ LL - debug_assert_eq!("requires negation".is_empty(), false); LL + debug_assert!(!"requires negation".is_empty()); | -error: aborting due to 33 previous errors +error: used `assert!` with an equality comparison + --> tests/ui/bool_assert_comparison.rs:170:5 + | +LL | assert!("a" == "a".to_ascii_lowercase()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: replace it with `assert_eq!(..)` + | +LL | assert_eq!("a", "a".to_ascii_lowercase()); + | ~~~~~~~~~ ~ + +error: used `assert!` with an equality comparison + --> tests/ui/bool_assert_comparison.rs:171:5 + | +LL | assert!("a" == "a".to_ascii_lowercase(), "a==a"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: replace it with `assert_eq!(..)` + | +LL | assert_eq!("a", "a".to_ascii_lowercase(), "a==a"); + | ~~~~~~~~~ ~ + +error: used `assert!` with an equality comparison + --> tests/ui/bool_assert_comparison.rs:172:5 + | +LL | assert!("A" != "A".to_ascii_lowercase()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: replace it with `assert_ne!(..)` + | +LL | assert_ne!("A", "A".to_ascii_lowercase()); + | ~~~~~~~~~ ~ + +error: used `assert!` with an equality comparison + --> tests/ui/bool_assert_comparison.rs:173:5 + | +LL | assert!("A" != "A".to_ascii_lowercase(), "A!=a"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: replace it with `assert_ne!(..)` + | +LL | assert_ne!("A", "A".to_ascii_lowercase(), "A!=a"); + | ~~~~~~~~~ ~ + +error: aborting due to 37 previous errors diff --git a/tests/ui/infinite_loops.rs b/tests/ui/infinite_loops.rs index b2d522fa011b..2f2041b9583f 100644 --- a/tests/ui/infinite_loops.rs +++ b/tests/ui/infinite_loops.rs @@ -1,7 +1,7 @@ //@no-rustfix //@aux-build:proc_macros.rs -#![allow(clippy::never_loop)] +#![allow(clippy::never_loop, clippy::bool_assert_comparison)] #![warn(clippy::infinite_loop)] extern crate proc_macros; diff --git a/tests/ui/missing_asserts_for_indexing.fixed b/tests/ui/missing_asserts_for_indexing.fixed index ac44a6f3fdb2..f4f81217ec73 100644 --- a/tests/ui/missing_asserts_for_indexing.fixed +++ b/tests/ui/missing_asserts_for_indexing.fixed @@ -1,4 +1,4 @@ -#![allow(unused)] +#![allow(unused, clippy::bool_assert_comparison)] #![warn(clippy::missing_asserts_for_indexing)] // ok diff --git a/tests/ui/missing_asserts_for_indexing.rs b/tests/ui/missing_asserts_for_indexing.rs index f05d5fea57dc..58f56150ab7c 100644 --- a/tests/ui/missing_asserts_for_indexing.rs +++ b/tests/ui/missing_asserts_for_indexing.rs @@ -1,4 +1,4 @@ -#![allow(unused)] +#![allow(unused, clippy::bool_assert_comparison)] #![warn(clippy::missing_asserts_for_indexing)] // ok diff --git a/tests/ui/panic_in_result_fn_assertions.rs b/tests/ui/panic_in_result_fn_assertions.rs index 672c4c738339..c586a44a862d 100644 --- a/tests/ui/panic_in_result_fn_assertions.rs +++ b/tests/ui/panic_in_result_fn_assertions.rs @@ -1,6 +1,6 @@ #![warn(clippy::panic_in_result_fn)] #![allow(clippy::uninlined_format_args, clippy::unnecessary_wraps)] - +#![allow(clippy::bool_assert_comparison)] struct A; impl A { diff --git a/tests/ui/panic_in_result_fn_debug_assertions.rs b/tests/ui/panic_in_result_fn_debug_assertions.rs index df89d8c50246..0ffef95c9051 100644 --- a/tests/ui/panic_in_result_fn_debug_assertions.rs +++ b/tests/ui/panic_in_result_fn_debug_assertions.rs @@ -1,6 +1,6 @@ #![warn(clippy::panic_in_result_fn)] #![allow(clippy::uninlined_format_args, clippy::unnecessary_wraps)] - +#![allow(clippy::bool_assert_comparison)] // debug_assert should never trigger the `panic_in_result_fn` lint struct A; diff --git a/tests/ui/uninit_vec.rs b/tests/ui/uninit_vec.rs index 464f88140bdb..baf30128466d 100644 --- a/tests/ui/uninit_vec.rs +++ b/tests/ui/uninit_vec.rs @@ -1,5 +1,5 @@ #![warn(clippy::uninit_vec)] - +#![allow(clippy::bool_assert_comparison)] use std::cell::UnsafeCell; use std::mem::MaybeUninit; diff --git a/tests/ui/uninlined_format_args_panic.edition2018.fixed b/tests/ui/uninlined_format_args_panic.edition2018.fixed index f0d570efdcee..2f3b59d82345 100644 --- a/tests/ui/uninlined_format_args_panic.edition2018.fixed +++ b/tests/ui/uninlined_format_args_panic.edition2018.fixed @@ -1,7 +1,7 @@ //@revisions: edition2018 edition2021 //@[edition2018] edition:2018 //@[edition2021] edition:2021 - +#![allow(clippy::bool_assert_comparison)] #![warn(clippy::uninlined_format_args)] fn main() { diff --git a/tests/ui/uninlined_format_args_panic.edition2021.fixed b/tests/ui/uninlined_format_args_panic.edition2021.fixed index 7c0f28c45764..8cf3f44dffa5 100644 --- a/tests/ui/uninlined_format_args_panic.edition2021.fixed +++ b/tests/ui/uninlined_format_args_panic.edition2021.fixed @@ -1,7 +1,7 @@ //@revisions: edition2018 edition2021 //@[edition2018] edition:2018 //@[edition2021] edition:2021 - +#![allow(clippy::bool_assert_comparison)] #![warn(clippy::uninlined_format_args)] fn main() { diff --git a/tests/ui/uninlined_format_args_panic.rs b/tests/ui/uninlined_format_args_panic.rs index fa594d9a96f0..70ea8589e829 100644 --- a/tests/ui/uninlined_format_args_panic.rs +++ b/tests/ui/uninlined_format_args_panic.rs @@ -1,7 +1,7 @@ //@revisions: edition2018 edition2021 //@[edition2018] edition:2018 //@[edition2021] edition:2021 - +#![allow(clippy::bool_assert_comparison)] #![warn(clippy::uninlined_format_args)] fn main() {