diff --git a/clippy_lints/src/bool_assert_comparison.rs b/clippy_lints/src/bool_assert_comparison.rs index adac2f27ea8c..427bb37a25df 100644 --- a/clippy_lints/src/bool_assert_comparison.rs +++ b/clippy_lints/src/bool_assert_comparison.rs @@ -1,14 +1,18 @@ use clippy_utils::diagnostics::span_lint_and_then; -use clippy_utils::macros::{find_assert_eq_args, root_macro_call_first_node}; +use clippy_utils::is_in_const_context; +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 +78,118 @@ 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) { + if is_in_const_context(cx) { + return; + } + 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 f90bf9157aad..460e85aff482 100644 --- a/clippy_lints/src/eta_reduction.rs +++ b/clippy_lints/src/eta_reduction.rs @@ -344,7 +344,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 ac2e866e4ff7..78d8d2356800 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/mod.rs b/clippy_utils/src/ty/mod.rs index f2bbdda70585..fab2956ba7c6 100644 --- a/clippy_utils/src/ty/mod.rs +++ b/clippy_utils/src/ty/mod.rs @@ -1029,8 +1029,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 dcc1ec339ef9..7f36520274fd 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.fixed b/tests/ui/assertions_on_constants.fixed new file mode 100644 index 000000000000..e75ecf74c6f6 --- /dev/null +++ b/tests/ui/assertions_on_constants.fixed @@ -0,0 +1,64 @@ +#![allow(non_fmt_panics, clippy::needless_bool, clippy::eq_op)] + +macro_rules! assert_const { + ($len:expr) => { + assert!($len > 0); + debug_assert!($len < 0); + }; +} +fn main() { + assert!(true); + //~^ ERROR: `assert!(true)` will be optimized out by the compiler + assert!(false); + //~^ ERROR: `assert!(false)` should probably be replaced + assert!(true, "true message"); + //~^ ERROR: `assert!(true)` will be optimized out by the compiler + assert!(false, "false message"); + //~^ ERROR: `assert!(false, ..)` should probably be replaced + + let msg = "panic message"; + assert!(false, "{}", msg.to_uppercase()); + //~^ ERROR: `assert!(false, ..)` should probably be replaced + + const B: bool = true; + assert!(B); + //~^ ERROR: `assert!(true)` will be optimized out by the compiler + + const C: bool = false; + assert!(C); + //~^ ERROR: `assert!(false)` should probably be replaced + assert!(C, "C message"); + //~^ ERROR: `assert!(false, ..)` should probably be replaced + + debug_assert!(true); + //~^ ERROR: `debug_assert!(true)` will be optimized out by the compiler + // Don't lint this, since there is no better way for expressing "Only panic in debug mode". + debug_assert!(false); // #3948 + assert_const!(3); + assert_const!(-1); + + // Don't lint if based on `cfg!(..)`: + assert!(cfg!(feature = "hey") || cfg!(not(feature = "asdf"))); + + let flag: bool = cfg!(not(feature = "asdf")); + assert!(flag); + + const CFG_FLAG: &bool = &cfg!(feature = "hey"); + assert!(!CFG_FLAG); + + const _: () = assert!(true); + //~^ ERROR: `assert!(true)` will be optimized out by the compiler + #[allow(clippy::bool_assert_comparison)] + assert_eq!(8, (7 + 1)); + //~^ ERROR: `assert!(true)` will be optimized out by the compiler + + // Don't lint if the value is dependent on a defined constant: + const N: usize = 1024; + const _: () = assert!(N.is_power_of_two()); +} + +const _: () = { + assert!(true); + //~^ ERROR: `assert!(true)` will be optimized out by the compiler + assert!(8 == (7 + 1)); +}; diff --git a/tests/ui/assertions_on_constants.rs b/tests/ui/assertions_on_constants.rs index 957154e60dec..390b2f5296d5 100644 --- a/tests/ui/assertions_on_constants.rs +++ b/tests/ui/assertions_on_constants.rs @@ -48,7 +48,7 @@ fn main() { const _: () = assert!(true); //~^ ERROR: `assert!(true)` will be optimized out by the compiler - + #[allow(clippy::bool_assert_comparison)] assert!(8 == (7 + 1)); //~^ ERROR: `assert!(true)` will be optimized out by the compiler diff --git a/tests/ui/assertions_on_constants.stderr b/tests/ui/assertions_on_constants.stderr index e164a999c43e..b1f763e1a72a 100644 --- a/tests/ui/assertions_on_constants.stderr +++ b/tests/ui/assertions_on_constants.stderr @@ -88,6 +88,19 @@ LL | assert!(8 == (7 + 1)); | = help: remove it +error: used `assert!` with an equality comparison + --> tests/ui/assertions_on_constants.rs:52:5 + | +LL | assert!(8 == (7 + 1)); + | ^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::bool-assert-comparison` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::bool_assert_comparison)]` +help: replace it with `assert_eq!(..)` + | +LL | assert_eq!(8, (7 + 1)); + | ~~~~~~~~~ ~ + error: `assert!(true)` will be optimized out by the compiler --> tests/ui/assertions_on_constants.rs:61:5 | @@ -96,5 +109,5 @@ LL | assert!(true); | = help: remove it -error: aborting due to 12 previous errors +error: aborting due to 13 previous errors diff --git a/tests/ui/bool_assert_comparison.fixed b/tests/ui/bool_assert_comparison.fixed index b05166a055ee..beb42fb40c2c 100644 --- a/tests/ui/bool_assert_comparison.fixed +++ b/tests/ui/bool_assert_comparison.fixed @@ -1,4 +1,4 @@ -#![allow(unused, clippy::assertions_on_constants, clippy::const_is_empty)] +#![allow(unused, clippy::assertions_on_constants, clippy::const_is_empty, clippy::eq_op)] #![warn(clippy::bool_assert_comparison)] use std::ops::Not; @@ -166,4 +166,14 @@ 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"); + const _: () = assert!(5 == 2 + 3); } + +const _: () = { + assert!(8 == (7 + 1)); +}; diff --git a/tests/ui/bool_assert_comparison.rs b/tests/ui/bool_assert_comparison.rs index dc51fcf1d36b..6b4c45635333 100644 --- a/tests/ui/bool_assert_comparison.rs +++ b/tests/ui/bool_assert_comparison.rs @@ -1,4 +1,4 @@ -#![allow(unused, clippy::assertions_on_constants, clippy::const_is_empty)] +#![allow(unused, clippy::assertions_on_constants, clippy::const_is_empty, clippy::eq_op)] #![warn(clippy::bool_assert_comparison)] use std::ops::Not; @@ -166,4 +166,14 @@ 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"); + const _: () = assert!(5 == 2 + 3); } + +const _: () = { + assert!(8 == (7 + 1)); +}; 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 d7be6f9ce7e9..7964b80191d0 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 9911d1317070..8c4f783fa747 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)] #![allow(clippy::literal_string_with_formatting_args)] diff --git a/tests/ui/uninlined_format_args_panic.edition2021.fixed b/tests/ui/uninlined_format_args_panic.edition2021.fixed index 87b74670565f..8fda7c45defd 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)] #![allow(clippy::literal_string_with_formatting_args)] diff --git a/tests/ui/uninlined_format_args_panic.rs b/tests/ui/uninlined_format_args_panic.rs index 647c69bc5c41..1cc131f11ee2 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)] #![allow(clippy::literal_string_with_formatting_args)]