From 140a1275f24ab951ffb0daee568385049de153d5 Mon Sep 17 00:00:00 2001 From: blyxyas Date: Thu, 10 Oct 2024 19:10:04 +0200 Subject: [PATCH] Fix span issue on `implicit_saturating_sub` Fixes #13524 --- clippy_lints/src/implicit_saturating_sub.rs | 57 ++++----------------- tests/ui/implicit_saturating_sub.fixed | 11 ++++ tests/ui/implicit_saturating_sub.rs | 11 ++++ tests/ui/implicit_saturating_sub.stderr | 8 ++- tests/ui/manual_arithmetic_check.fixed | 8 +-- tests/ui/manual_arithmetic_check.stderr | 16 +++--- 6 files changed, 51 insertions(+), 60 deletions(-) diff --git a/clippy_lints/src/implicit_saturating_sub.rs b/clippy_lints/src/implicit_saturating_sub.rs index f4a64f5c20b4..5dc5233409bd 100644 --- a/clippy_lints/src/implicit_saturating_sub.rs +++ b/clippy_lints/src/implicit_saturating_sub.rs @@ -107,9 +107,7 @@ 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, expr, cond_op, cond_left, cond_right, if_block, else_block, &self.msrv, - ); + check_manual_check(cx, cond_op, cond_left, cond_right, if_block, else_block, &self.msrv); } } @@ -119,7 +117,6 @@ 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>, @@ -130,26 +127,12 @@ 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, - expr.span, - left_hand, - right_hand, - if_block, - else_block, - msrv, - ), - BinOpKind::Lt | BinOpKind::Le => check_gt( - cx, - condition.span, - expr.span, - right_hand, - left_hand, - if_block, - else_block, - msrv, - ), + 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); + }, _ => {}, } } @@ -159,7 +142,6 @@ 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<'_>, @@ -169,16 +151,7 @@ fn check_gt( if let Some(big_var) = Var::new(big_var) && let Some(little_var) = Var::new(little_var) { - check_subtraction( - cx, - condition_span, - expr_span, - big_var, - little_var, - if_block, - else_block, - msrv, - ); + check_subtraction(cx, condition_span, big_var, little_var, if_block, else_block, msrv); } } @@ -200,7 +173,6 @@ impl Var { fn check_subtraction( cx: &LateContext<'_>, condition_span: Span, - expr_span: Span, big_var: Var, little_var: Var, if_block: &Expr<'_>, @@ -217,16 +189,7 @@ 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, - expr_span, - little_var, - big_var, - else_block, - if_block, - msrv, - ); + check_subtraction(cx, condition_span, little_var, big_var, else_block, if_block, msrv); return; } if is_integer_literal(else_block, 0) @@ -245,7 +208,7 @@ fn check_subtraction( span_lint_and_sugg( cx, IMPLICIT_SATURATING_SUB, - expr_span, + else_block.span, "manual arithmetic check found", "replace it with", format!("{big_var_snippet}.saturating_sub({little_var_snippet})"), diff --git a/tests/ui/implicit_saturating_sub.fixed b/tests/ui/implicit_saturating_sub.fixed index 81cc14949144..4ece3e66a37e 100644 --- a/tests/ui/implicit_saturating_sub.fixed +++ b/tests/ui/implicit_saturating_sub.fixed @@ -222,3 +222,14 @@ fn main() { a - b }; } + +// 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 + } +} diff --git a/tests/ui/implicit_saturating_sub.rs b/tests/ui/implicit_saturating_sub.rs index f73396ebd27e..dc57b652441c 100644 --- a/tests/ui/implicit_saturating_sub.rs +++ b/tests/ui/implicit_saturating_sub.rs @@ -268,3 +268,14 @@ fn main() { a - b }; } + +// 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 { + 0 + } else { + b - a + } +} diff --git a/tests/ui/implicit_saturating_sub.stderr b/tests/ui/implicit_saturating_sub.stderr index 59a9ddbff2d6..e5073f2318b8 100644 --- a/tests/ui/implicit_saturating_sub.stderr +++ b/tests/ui/implicit_saturating_sub.stderr @@ -185,5 +185,11 @@ LL | | i_64 -= 1; LL | | } | |_____^ help: try: `i_64 = i_64.saturating_sub(1);` -error: aborting due to 23 previous errors +error: manual arithmetic check found + --> tests/ui/implicit_saturating_sub.rs:277:9 + | +LL | 0 + | ^ 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 29ecbb9ad2ad..1eebd4eadf5f 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 = a.saturating_sub(b); + let result = if a > b { a - b } else { a.saturating_sub(b) }; //~^ ERROR: manual arithmetic check found - let result = a.saturating_sub(b); + let result = if b < a { a - b } else { a.saturating_sub(b) }; //~^ ERROR: manual arithmetic check found - let result = a.saturating_sub(b); + let result = if a < b { a.saturating_sub(b) } else { a - b }; //~^ ERROR: manual arithmetic check found - let result = a.saturating_sub(b); + let result = if b > a { a.saturating_sub(b) } else { a - 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 b0cf73cd9151..a874d92a7290 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:18 + --> tests/ui/manual_arithmetic_check.rs:9:44 | 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:18 + --> tests/ui/manual_arithmetic_check.rs:11:44 | 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:18 + --> tests/ui/manual_arithmetic_check.rs:14:29 | 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:18 + --> tests/ui/manual_arithmetic_check.rs:16:29 | 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