From 2b562dece6ceb492828ded6571f00fa283a3a81f Mon Sep 17 00:00:00 2001 From: blyxyas Date: Fri, 11 Oct 2024 21:10:34 +0200 Subject: [PATCH] Fix suggestion with a less volatile approach Revert "Fix span issue on `implicit_saturating_sub`" This reverts commit 140a1275f24ab951ffb0daee568385049de153d5. --- clippy_lints/src/implicit_saturating_sub.rs | 82 ++++++++++++++++++--- tests/ui/implicit_saturating_sub.fixed | 7 +- tests/ui/implicit_saturating_sub.rs | 1 - tests/ui/implicit_saturating_sub.stderr | 11 ++- tests/ui/manual_arithmetic_check.fixed | 8 +- tests/ui/manual_arithmetic_check.stderr | 16 ++-- 6 files changed, 92 insertions(+), 33 deletions(-) diff --git a/clippy_lints/src/implicit_saturating_sub.rs b/clippy_lints/src/implicit_saturating_sub.rs index 5dc5233409bdc..3b84b569c3edf 100644 --- a/clippy_lints/src/implicit_saturating_sub.rs +++ b/clippy_lints/src/implicit_saturating_sub.rs @@ -107,7 +107,9 @@ impl<'tcx> LateLintPass<'tcx> for ImplicitSaturatingSub { }) = higher::If::hir(expr) && let ExprKind::Binary(ref cond_op, cond_left, cond_right) = cond.kind { - check_manual_check(cx, cond_op, cond_left, cond_right, if_block, else_block, &self.msrv); + check_manual_check( + cx, expr, cond_op, cond_left, cond_right, if_block, else_block, &self.msrv, + ); } } @@ -117,6 +119,7 @@ impl<'tcx> LateLintPass<'tcx> for ImplicitSaturatingSub { #[allow(clippy::too_many_arguments)] fn check_manual_check<'tcx>( cx: &LateContext<'tcx>, + expr: &Expr<'tcx>, condition: &BinOp, left_hand: &Expr<'tcx>, right_hand: &Expr<'tcx>, @@ -127,12 +130,40 @@ fn check_manual_check<'tcx>( let ty = cx.typeck_results().expr_ty(left_hand); if ty.is_numeric() && !ty.is_signed() { match condition.node { - BinOpKind::Gt | BinOpKind::Ge => { - check_gt(cx, condition.span, left_hand, right_hand, if_block, else_block, msrv); - }, - BinOpKind::Lt | BinOpKind::Le => { - check_gt(cx, condition.span, right_hand, left_hand, if_block, else_block, msrv); - }, + BinOpKind::Gt | BinOpKind::Ge => check_gt( + cx, + condition.span, + expr.span, + left_hand, + right_hand, + if_block, + else_block, + msrv, + matches!( + clippy_utils::get_parent_expr(cx, expr), + Some(Expr { + kind: ExprKind::If(..), + .. + }) + ), + ), + BinOpKind::Lt | BinOpKind::Le => check_gt( + cx, + condition.span, + expr.span, + right_hand, + left_hand, + if_block, + else_block, + msrv, + matches!( + clippy_utils::get_parent_expr(cx, expr), + Some(Expr { + kind: ExprKind::If(..), + .. + }) + ), + ), _ => {}, } } @@ -142,16 +173,28 @@ fn check_manual_check<'tcx>( fn check_gt( cx: &LateContext<'_>, condition_span: Span, + expr_span: Span, big_var: &Expr<'_>, little_var: &Expr<'_>, if_block: &Expr<'_>, else_block: &Expr<'_>, msrv: &Msrv, + is_composited: bool, ) { if let Some(big_var) = Var::new(big_var) && let Some(little_var) = Var::new(little_var) { - check_subtraction(cx, condition_span, big_var, little_var, if_block, else_block, msrv); + check_subtraction( + cx, + condition_span, + expr_span, + big_var, + little_var, + if_block, + else_block, + msrv, + is_composited, + ); } } @@ -173,11 +216,13 @@ impl Var { fn check_subtraction( cx: &LateContext<'_>, condition_span: Span, + expr_span: Span, big_var: Var, little_var: Var, if_block: &Expr<'_>, else_block: &Expr<'_>, msrv: &Msrv, + is_composited: bool, ) { let if_block = peel_blocks(if_block); let else_block = peel_blocks(else_block); @@ -189,7 +234,17 @@ fn check_subtraction( } // If the subtraction is done in the `else` block, then we need to also revert the two // variables as it means that the check was reverted too. - check_subtraction(cx, condition_span, little_var, big_var, else_block, if_block, msrv); + check_subtraction( + cx, + condition_span, + expr_span, + little_var, + big_var, + else_block, + if_block, + msrv, + is_composited, + ); return; } if is_integer_literal(else_block, 0) @@ -205,13 +260,18 @@ fn check_subtraction( && let Some(little_var_snippet) = snippet_opt(cx, little_var.span) && (!is_in_const_context(cx) || msrv.meets(msrvs::SATURATING_SUB_CONST)) { + let sugg = format!( + "{}{big_var_snippet}.saturating_sub({little_var_snippet}){}", + if is_composited { "{ " } else { "" }, + if is_composited { " }" } else { "" } + ); span_lint_and_sugg( cx, IMPLICIT_SATURATING_SUB, - else_block.span, + expr_span, "manual arithmetic check found", "replace it with", - format!("{big_var_snippet}.saturating_sub({little_var_snippet})"), + sugg, Applicability::MachineApplicable, ); } diff --git a/tests/ui/implicit_saturating_sub.fixed b/tests/ui/implicit_saturating_sub.fixed index 4ece3e66a37ea..136238f9ecacc 100644 --- a/tests/ui/implicit_saturating_sub.fixed +++ b/tests/ui/implicit_saturating_sub.fixed @@ -223,13 +223,8 @@ fn main() { }; } -// https://github.com/rust-lang/rust-clippy/issues/13524 fn regression_13524(a: usize, b: usize, c: bool) -> usize { if c { 123 - } else if a >= b { - b.saturating_sub(a) - } else { - b - a - } + } else { b.saturating_sub(a) } } diff --git a/tests/ui/implicit_saturating_sub.rs b/tests/ui/implicit_saturating_sub.rs index dc57b652441c0..e371e37fb2f65 100644 --- a/tests/ui/implicit_saturating_sub.rs +++ b/tests/ui/implicit_saturating_sub.rs @@ -269,7 +269,6 @@ fn main() { }; } -// https://github.com/rust-lang/rust-clippy/issues/13524 fn regression_13524(a: usize, b: usize, c: bool) -> usize { if c { 123 diff --git a/tests/ui/implicit_saturating_sub.stderr b/tests/ui/implicit_saturating_sub.stderr index e5073f2318b89..6131985122805 100644 --- a/tests/ui/implicit_saturating_sub.stderr +++ b/tests/ui/implicit_saturating_sub.stderr @@ -186,10 +186,15 @@ LL | | } | |_____^ help: try: `i_64 = i_64.saturating_sub(1);` error: manual arithmetic check found - --> tests/ui/implicit_saturating_sub.rs:277:9 + --> tests/ui/implicit_saturating_sub.rs:275:12 | -LL | 0 - | ^ help: replace it with: `b.saturating_sub(a)` +LL | } else if a >= b { + | ____________^ +LL | | 0 +LL | | } else { +LL | | b - a +LL | | } + | |_____^ help: replace it with: `{ b.saturating_sub(a) }` error: aborting due to 24 previous errors diff --git a/tests/ui/manual_arithmetic_check.fixed b/tests/ui/manual_arithmetic_check.fixed index 1eebd4eadf5fb..29ecbb9ad2adb 100644 --- a/tests/ui/manual_arithmetic_check.fixed +++ b/tests/ui/manual_arithmetic_check.fixed @@ -6,14 +6,14 @@ fn main() { let b = 13u32; let c = 8u32; - let result = if a > b { a - b } else { a.saturating_sub(b) }; + let result = a.saturating_sub(b); //~^ ERROR: manual arithmetic check found - let result = if b < a { a - b } else { a.saturating_sub(b) }; + let result = a.saturating_sub(b); //~^ ERROR: manual arithmetic check found - let result = if a < b { a.saturating_sub(b) } else { a - b }; + let result = a.saturating_sub(b); //~^ ERROR: manual arithmetic check found - let result = if b > a { a.saturating_sub(b) } else { a - b }; + let result = a.saturating_sub(b); //~^ ERROR: manual arithmetic check found // Should not warn! diff --git a/tests/ui/manual_arithmetic_check.stderr b/tests/ui/manual_arithmetic_check.stderr index a874d92a72900..b0cf73cd9151d 100644 --- a/tests/ui/manual_arithmetic_check.stderr +++ b/tests/ui/manual_arithmetic_check.stderr @@ -1,29 +1,29 @@ error: manual arithmetic check found - --> tests/ui/manual_arithmetic_check.rs:9:44 + --> tests/ui/manual_arithmetic_check.rs:9:18 | LL | let result = if a > b { a - b } else { 0 }; - | ^ help: replace it with: `a.saturating_sub(b)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `a.saturating_sub(b)` | = note: `-D clippy::implicit-saturating-sub` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::implicit_saturating_sub)]` error: manual arithmetic check found - --> tests/ui/manual_arithmetic_check.rs:11:44 + --> tests/ui/manual_arithmetic_check.rs:11:18 | LL | let result = if b < a { a - b } else { 0 }; - | ^ help: replace it with: `a.saturating_sub(b)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `a.saturating_sub(b)` error: manual arithmetic check found - --> tests/ui/manual_arithmetic_check.rs:14:29 + --> tests/ui/manual_arithmetic_check.rs:14:18 | LL | let result = if a < b { 0 } else { a - b }; - | ^ help: replace it with: `a.saturating_sub(b)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `a.saturating_sub(b)` error: manual arithmetic check found - --> tests/ui/manual_arithmetic_check.rs:16:29 + --> tests/ui/manual_arithmetic_check.rs:16:18 | LL | let result = if b > a { 0 } else { a - b }; - | ^ help: replace it with: `a.saturating_sub(b)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `a.saturating_sub(b)` error: aborting due to 4 previous errors