Skip to content

Commit

Permalink
Fix span issue on implicit_saturating_sub
Browse files Browse the repository at this point in the history
  • Loading branch information
blyxyas committed Oct 10, 2024
1 parent d578f6a commit 140a127
Show file tree
Hide file tree
Showing 6 changed files with 51 additions and 60 deletions.
57 changes: 10 additions & 47 deletions clippy_lints/src/implicit_saturating_sub.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand All @@ -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>,
Expand All @@ -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);
},
_ => {},
}
}
Expand All @@ -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<'_>,
Expand All @@ -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);
}
}

Expand All @@ -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<'_>,
Expand All @@ -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)
Expand All @@ -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})"),
Expand Down
11 changes: 11 additions & 0 deletions tests/ui/implicit_saturating_sub.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
11 changes: 11 additions & 0 deletions tests/ui/implicit_saturating_sub.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
8 changes: 7 additions & 1 deletion tests/ui/implicit_saturating_sub.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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

8 changes: 4 additions & 4 deletions tests/ui/manual_arithmetic_check.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -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!
Expand Down
16 changes: 8 additions & 8 deletions tests/ui/manual_arithmetic_check.stderr
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 140a127

Please sign in to comment.