From 76545660a14b8ca4e76bcdd807816f566f6861f2 Mon Sep 17 00:00:00 2001 From: klensy Date: Sat, 14 Dec 2024 18:45:03 +0300 Subject: [PATCH 1/4] eq_op: allow optionally check for call fn --- clippy_config/src/conf.rs | 3 +++ clippy_lints/src/operators/eq_op.rs | 20 ++++++++++++++++---- clippy_lints/src/operators/mod.rs | 6 ++++-- clippy_utils/src/hir_utils.rs | 5 +++++ clippy_utils/src/lib.rs | 3 ++- 5 files changed, 30 insertions(+), 7 deletions(-) diff --git a/clippy_config/src/conf.rs b/clippy_config/src/conf.rs index 841facdca06d..9bb15b381a19 100644 --- a/clippy_config/src/conf.rs +++ b/clippy_config/src/conf.rs @@ -540,6 +540,9 @@ define_Conf! { /// For internal testing only, ignores the current `publish` settings in the Cargo manifest. #[lints(cargo_common_metadata)] cargo_ignore_publish: bool = false, + /// Check fn calls in eq_op lint + #[lints(expect_used)] + check_fn_call_in_eq_op: bool = false, /// Whether to check MSRV compatibility in `#[test]` and `#[cfg(test)]` code. #[lints(incompatible_msrv)] check_incompatible_msrv_in_tests: bool = false, diff --git a/clippy_lints/src/operators/eq_op.rs b/clippy_lints/src/operators/eq_op.rs index d79101a687df..7510e333bd81 100644 --- a/clippy_lints/src/operators/eq_op.rs +++ b/clippy_lints/src/operators/eq_op.rs @@ -1,20 +1,24 @@ use clippy_utils::ast_utils::is_useless_with_eq_exprs; use clippy_utils::diagnostics::{span_lint, span_lint_and_then}; use clippy_utils::macros::{find_assert_eq_args, first_node_macro_backtrace}; -use clippy_utils::{eq_expr_value, is_in_test_function, sym}; +use clippy_utils::{eq_expr_value, eq_expr_value_with_sideffects, is_in_test_function, sym}; use rustc_hir::{BinOpKind, Expr}; use rustc_lint::LateContext; use super::EQ_OP; -pub(crate) fn check_assert<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) { +pub(crate) fn check_assert<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>, with_sideffects: bool) { if let Some(macro_call) = first_node_macro_backtrace(cx, e).find(|macro_call| { matches!( cx.tcx.get_diagnostic_name(macro_call.def_id), Some(sym::assert_eq_macro | sym::assert_ne_macro | sym::debug_assert_eq_macro | sym::debug_assert_ne_macro) ) }) && let Some((lhs, rhs, _)) = find_assert_eq_args(cx, e, macro_call.expn) - && eq_expr_value(cx, lhs, rhs) + && if with_sideffects { + eq_expr_value_with_sideffects(cx, lhs, rhs) + } else { + eq_expr_value(cx, lhs, rhs) + } && macro_call.is_local() && !is_in_test_function(cx.tcx, e.hir_id) { @@ -36,8 +40,16 @@ pub(crate) fn check<'tcx>( op: BinOpKind, left: &'tcx Expr<'_>, right: &'tcx Expr<'_>, + with_sideffects: bool, ) { - if is_useless_with_eq_exprs(op) && eq_expr_value(cx, left, right) && !is_in_test_function(cx.tcx, e.hir_id) { + if is_useless_with_eq_exprs(op) + && if with_sideffects { + eq_expr_value_with_sideffects(cx, left, right) + } else { + eq_expr_value(cx, left, right) + } + && !is_in_test_function(cx.tcx, e.hir_id) + { span_lint_and_then( cx, EQ_OP, diff --git a/clippy_lints/src/operators/mod.rs b/clippy_lints/src/operators/mod.rs index 2f4e8e995886..e6b9a4e2a284 100644 --- a/clippy_lints/src/operators/mod.rs +++ b/clippy_lints/src/operators/mod.rs @@ -835,6 +835,7 @@ pub struct Operators { verbose_bit_mask_threshold: u64, modulo_arithmetic_allow_comparison_to_zero: bool, msrv: Msrv, + check_fn_call_in_eq_op: bool, } impl Operators { pub fn new(conf: &'static Conf) -> Self { @@ -843,6 +844,7 @@ impl Operators { verbose_bit_mask_threshold: conf.verbose_bit_mask_threshold, modulo_arithmetic_allow_comparison_to_zero: conf.allow_comparison_to_zero, msrv: conf.msrv, + check_fn_call_in_eq_op: conf.check_fn_call_in_eq_op, } } } @@ -878,13 +880,13 @@ impl_lint_pass!(Operators => [ impl<'tcx> LateLintPass<'tcx> for Operators { fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) { - eq_op::check_assert(cx, e); + eq_op::check_assert(cx, e, self.check_fn_call_in_eq_op); match e.kind { ExprKind::Binary(op, lhs, rhs) => { if !e.span.from_expansion() { absurd_extreme_comparisons::check(cx, e, op.node, lhs, rhs); if !(macro_with_not_op(lhs) || macro_with_not_op(rhs)) { - eq_op::check(cx, e, op.node, lhs, rhs); + eq_op::check(cx, e, op.node, lhs, rhs, self.check_fn_call_in_eq_op); op_ref::check(cx, e, op.node, lhs, rhs); } erasing_op::check(cx, e, op.node, lhs, rhs); diff --git a/clippy_utils/src/hir_utils.rs b/clippy_utils/src/hir_utils.rs index c37231d09312..78024124bd00 100644 --- a/clippy_utils/src/hir_utils.rs +++ b/clippy_utils/src/hir_utils.rs @@ -774,6 +774,11 @@ pub fn eq_expr_value(cx: &LateContext<'_>, left: &Expr<'_>, right: &Expr<'_>) -> SpanlessEq::new(cx).deny_side_effects().eq_expr(left, right) } +/// Checks if two expressions evaluate to the same value, ignoring side effects. +pub fn eq_expr_value_with_sideffects(cx: &LateContext<'_>, left: &Expr<'_>, right: &Expr<'_>) -> bool { + SpanlessEq::new(cx).eq_expr(left, right) +} + /// Returns the segments of a path that might have generic parameters. /// Usually just the last segment for free items, except for when the path resolves to an associated /// item, in which case it is the last two diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 7fa52229fef8..a2c043cb325a 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -77,7 +77,8 @@ pub mod visitors; pub use self::attrs::*; pub use self::check_proc_macro::{is_from_proc_macro, is_span_if, is_span_match}; pub use self::hir_utils::{ - HirEqInterExpr, SpanlessEq, SpanlessHash, both, count_eq, eq_expr_value, hash_expr, hash_stmt, is_bool, over, + HirEqInterExpr, SpanlessEq, SpanlessHash, both, count_eq, eq_expr_value, eq_expr_value_with_sideffects, hash_expr, + hash_stmt, is_bool, over, }; use core::mem; From b4f8332b3617507a32fdf5a75df3abb605bda42a Mon Sep 17 00:00:00 2001 From: klensy Date: Sat, 14 Dec 2024 18:45:37 +0300 Subject: [PATCH 2/4] [dont merge] turn on --- clippy_config/src/conf.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_config/src/conf.rs b/clippy_config/src/conf.rs index 9bb15b381a19..5c76e528302b 100644 --- a/clippy_config/src/conf.rs +++ b/clippy_config/src/conf.rs @@ -542,7 +542,7 @@ define_Conf! { cargo_ignore_publish: bool = false, /// Check fn calls in eq_op lint #[lints(expect_used)] - check_fn_call_in_eq_op: bool = false, + check_fn_call_in_eq_op: bool = true, /// Whether to check MSRV compatibility in `#[test]` and `#[cfg(test)]` code. #[lints(incompatible_msrv)] check_incompatible_msrv_in_tests: bool = false, From 251e58cb89d5e73053f9ed9774f8736237096f1e Mon Sep 17 00:00:00 2001 From: klensy Date: Wed, 18 Jun 2025 17:08:21 +0300 Subject: [PATCH 3/4] [dont merge] lets see fallout --- clippy_utils/src/hir_utils.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_utils/src/hir_utils.rs b/clippy_utils/src/hir_utils.rs index 78024124bd00..a6568d126913 100644 --- a/clippy_utils/src/hir_utils.rs +++ b/clippy_utils/src/hir_utils.rs @@ -771,7 +771,7 @@ pub fn count_eq( /// Checks if two expressions evaluate to the same value, and don't contain any side effects. pub fn eq_expr_value(cx: &LateContext<'_>, left: &Expr<'_>, right: &Expr<'_>) -> bool { - SpanlessEq::new(cx).deny_side_effects().eq_expr(left, right) + eq_expr_value_with_sideffects(cx, left, right) } /// Checks if two expressions evaluate to the same value, ignoring side effects. From 76114febd6d14b323d639c2df7b86df4a52017f8 Mon Sep 17 00:00:00 2001 From: klensy Date: Wed, 18 Jun 2025 17:46:15 +0300 Subject: [PATCH 4/4] [dont merge] touch ifs_same_cond too --- clippy_lints/src/copies.rs | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-) diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index 5ef726638a56..97daff193852 100644 --- a/clippy_lints/src/copies.rs +++ b/clippy_lints/src/copies.rs @@ -614,23 +614,7 @@ fn method_caller_is_mutable<'tcx>( /// Implementation of `IFS_SAME_COND`. fn lint_same_cond<'tcx>(cx: &LateContext<'tcx>, conds: &[&Expr<'_>], interior_mut: &mut InteriorMut<'tcx>) { - for group in search_same( - conds, - |e| hash_expr(cx, e), - |lhs, rhs| { - // Ignore eq_expr side effects iff one of the expression kind is a method call - // and the caller is not a mutable, including inner mutable type. - if let ExprKind::MethodCall(_, caller, _, _) = lhs.kind { - if method_caller_is_mutable(cx, caller, interior_mut) { - false - } else { - SpanlessEq::new(cx).eq_expr(lhs, rhs) - } - } else { - eq_expr_value(cx, lhs, rhs) - } - }, - ) { + for group in search_same(conds, |e| hash_expr(cx, e), |lhs, rhs| eq_expr_value(cx, lhs, rhs)) { let spans: Vec<_> = group.into_iter().map(|expr| expr.span).collect(); span_lint(cx, IFS_SAME_COND, spans, "these `if` branches have the same condition"); }