Skip to content

Commit f883e28

Browse files
borsbors
committed
Auto merge of rust-lang#13542 - blyxyas:fix-implicit_saturating_sub, r=y21
[`implicit_saturating_sub`] Fix suggestion with a less volatile approach Related to rust-lang#13533, such and obvious mistake got pass my watch, quite embarassing :/ Revert rust-lang#13533 and implement a more robust solution. Revert "Fix span issue on `implicit_saturating_sub` This reverts commit 140a127. changelog: [`lint_name`]: Fix suggestion for `if {} else if {} else {}` cases r? `@y21`
2 parents 236751d + 2b562de commit f883e28

File tree

6 files changed

+92
-33
lines changed

6 files changed

+92
-33
lines changed

‎clippy_lints/src/implicit_saturating_sub.rs‎

Lines changed: 71 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,9 @@ impl<'tcx> LateLintPass<'tcx> for ImplicitSaturatingSub {
107107
}) = higher::If::hir(expr)
108108
&& let ExprKind::Binary(ref cond_op, cond_left, cond_right) = cond.kind
109109
{
110-
check_manual_check(cx, cond_op, cond_left, cond_right, if_block, else_block, &self.msrv);
110+
check_manual_check(
111+
cx, expr, cond_op, cond_left, cond_right, if_block, else_block, &self.msrv,
112+
);
111113
}
112114
}
113115

@@ -117,6 +119,7 @@ impl<'tcx> LateLintPass<'tcx> for ImplicitSaturatingSub {
117119
#[allow(clippy::too_many_arguments)]
118120
fn check_manual_check<'tcx>(
119121
cx: &LateContext<'tcx>,
122+
expr: &Expr<'tcx>,
120123
condition: &BinOp,
121124
left_hand: &Expr<'tcx>,
122125
right_hand: &Expr<'tcx>,
@@ -127,12 +130,40 @@ fn check_manual_check<'tcx>(
127130
let ty = cx.typeck_results().expr_ty(left_hand);
128131
if ty.is_numeric() && !ty.is_signed() {
129132
match condition.node {
130-
BinOpKind::Gt | BinOpKind::Ge => {
131-
check_gt(cx, condition.span, left_hand, right_hand, if_block, else_block, msrv);
132-
},
133-
BinOpKind::Lt | BinOpKind::Le => {
134-
check_gt(cx, condition.span, right_hand, left_hand, if_block, else_block, msrv);
135-
},
133+
BinOpKind::Gt | BinOpKind::Ge => check_gt(
134+
cx,
135+
condition.span,
136+
expr.span,
137+
left_hand,
138+
right_hand,
139+
if_block,
140+
else_block,
141+
msrv,
142+
matches!(
143+
clippy_utils::get_parent_expr(cx, expr),
144+
Some(Expr {
145+
kind: ExprKind::If(..),
146+
..
147+
})
148+
),
149+
),
150+
BinOpKind::Lt | BinOpKind::Le => check_gt(
151+
cx,
152+
condition.span,
153+
expr.span,
154+
right_hand,
155+
left_hand,
156+
if_block,
157+
else_block,
158+
msrv,
159+
matches!(
160+
clippy_utils::get_parent_expr(cx, expr),
161+
Some(Expr {
162+
kind: ExprKind::If(..),
163+
..
164+
})
165+
),
166+
),
136167
_ => {},
137168
}
138169
}
@@ -142,16 +173,28 @@ fn check_manual_check<'tcx>(
142173
fn check_gt(
143174
cx: &LateContext<'_>,
144175
condition_span: Span,
176+
expr_span: Span,
145177
big_var: &Expr<'_>,
146178
little_var: &Expr<'_>,
147179
if_block: &Expr<'_>,
148180
else_block: &Expr<'_>,
149181
msrv: &Msrv,
182+
is_composited: bool,
150183
) {
151184
if let Some(big_var) = Var::new(big_var)
152185
&& let Some(little_var) = Var::new(little_var)
153186
{
154-
check_subtraction(cx, condition_span, big_var, little_var, if_block, else_block, msrv);
187+
check_subtraction(
188+
cx,
189+
condition_span,
190+
expr_span,
191+
big_var,
192+
little_var,
193+
if_block,
194+
else_block,
195+
msrv,
196+
is_composited,
197+
);
155198
}
156199
}
157200

@@ -173,11 +216,13 @@ impl Var {
173216
fn check_subtraction(
174217
cx: &LateContext<'_>,
175218
condition_span: Span,
219+
expr_span: Span,
176220
big_var: Var,
177221
little_var: Var,
178222
if_block: &Expr<'_>,
179223
else_block: &Expr<'_>,
180224
msrv: &Msrv,
225+
is_composited: bool,
181226
) {
182227
let if_block = peel_blocks(if_block);
183228
let else_block = peel_blocks(else_block);
@@ -189,7 +234,17 @@ fn check_subtraction(
189234
}
190235
// If the subtraction is done in the `else` block, then we need to also revert the two
191236
// variables as it means that the check was reverted too.
192-
check_subtraction(cx, condition_span, little_var, big_var, else_block, if_block, msrv);
237+
check_subtraction(
238+
cx,
239+
condition_span,
240+
expr_span,
241+
little_var,
242+
big_var,
243+
else_block,
244+
if_block,
245+
msrv,
246+
is_composited,
247+
);
193248
return;
194249
}
195250
if is_integer_literal(else_block, 0)
@@ -205,13 +260,18 @@ fn check_subtraction(
205260
&& let Some(little_var_snippet) = snippet_opt(cx, little_var.span)
206261
&& (!is_in_const_context(cx) || msrv.meets(msrvs::SATURATING_SUB_CONST))
207262
{
263+
let sugg = format!(
264+
"{}{big_var_snippet}.saturating_sub({little_var_snippet}){}",
265+
if is_composited { "{ " } else { "" },
266+
if is_composited { " }" } else { "" }
267+
);
208268
span_lint_and_sugg(
209269
cx,
210270
IMPLICIT_SATURATING_SUB,
211-
else_block.span,
271+
expr_span,
212272
"manual arithmetic check found",
213273
"replace it with",
214-
format!("{big_var_snippet}.saturating_sub({little_var_snippet})"),
274+
sugg,
215275
Applicability::MachineApplicable,
216276
);
217277
}

‎tests/ui/implicit_saturating_sub.fixed‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -223,13 +223,8 @@ fn main() {
223223
};
224224
}
225225

226-
// https://github.com/rust-lang/rust-clippy/issues/13524
227226
fn regression_13524(a: usize, b: usize, c: bool) -> usize {
228227
if c {
229228
123
230-
} else if a >= b {
231-
b.saturating_sub(a)
232-
} else {
233-
b - a
234-
}
229+
} else { b.saturating_sub(a) }
235230
}

‎tests/ui/implicit_saturating_sub.rs‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -269,7 +269,6 @@ fn main() {
269269
};
270270
}
271271

272-
// https://github.com/rust-lang/rust-clippy/issues/13524
273272
fn regression_13524(a: usize, b: usize, c: bool) -> usize {
274273
if c {
275274
123

‎tests/ui/implicit_saturating_sub.stderr‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -186,10 +186,15 @@ LL | | }
186186
| |_____^ help: try: `i_64 = i_64.saturating_sub(1);`
187187

188188
error: manual arithmetic check found
189-
--> tests/ui/implicit_saturating_sub.rs:277:9
189+
--> tests/ui/implicit_saturating_sub.rs:275:12
190190
|
191-
LL | 0
192-
| ^ help: replace it with: `b.saturating_sub(a)`
191+
LL | } else if a >= b {
192+
| ____________^
193+
LL | | 0
194+
LL | | } else {
195+
LL | | b - a
196+
LL | | }
197+
| |_____^ help: replace it with: `{ b.saturating_sub(a) }`
193198

194199
error: aborting due to 24 previous errors
195200

‎tests/ui/manual_arithmetic_check.fixed‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,14 +6,14 @@ fn main() {
66
let b = 13u32;
77
let c = 8u32;
88

9-
let result = if a > b { a - b } else { a.saturating_sub(b) };
9+
let result = a.saturating_sub(b);
1010
//~^ ERROR: manual arithmetic check found
11-
let result = if b < a { a - b } else { a.saturating_sub(b) };
11+
let result = a.saturating_sub(b);
1212
//~^ ERROR: manual arithmetic check found
1313

14-
let result = if a < b { a.saturating_sub(b) } else { a - b };
14+
let result = a.saturating_sub(b);
1515
//~^ ERROR: manual arithmetic check found
16-
let result = if b > a { a.saturating_sub(b) } else { a - b };
16+
let result = a.saturating_sub(b);
1717
//~^ ERROR: manual arithmetic check found
1818

1919
// Should not warn!

‎tests/ui/manual_arithmetic_check.stderr‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,29 +1,29 @@
11
error: manual arithmetic check found
2-
--> tests/ui/manual_arithmetic_check.rs:9:44
2+
--> tests/ui/manual_arithmetic_check.rs:9:18
33
|
44
LL | let result = if a > b { a - b } else { 0 };
5-
| ^ help: replace it with: `a.saturating_sub(b)`
5+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `a.saturating_sub(b)`
66
|
77
= note: `-D clippy::implicit-saturating-sub` implied by `-D warnings`
88
= help: to override `-D warnings` add `#[allow(clippy::implicit_saturating_sub)]`
99

1010
error: manual arithmetic check found
11-
--> tests/ui/manual_arithmetic_check.rs:11:44
11+
--> tests/ui/manual_arithmetic_check.rs:11:18
1212
|
1313
LL | let result = if b < a { a - b } else { 0 };
14-
| ^ help: replace it with: `a.saturating_sub(b)`
14+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `a.saturating_sub(b)`
1515

1616
error: manual arithmetic check found
17-
--> tests/ui/manual_arithmetic_check.rs:14:29
17+
--> tests/ui/manual_arithmetic_check.rs:14:18
1818
|
1919
LL | let result = if a < b { 0 } else { a - b };
20-
| ^ help: replace it with: `a.saturating_sub(b)`
20+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `a.saturating_sub(b)`
2121

2222
error: manual arithmetic check found
23-
--> tests/ui/manual_arithmetic_check.rs:16:29
23+
--> tests/ui/manual_arithmetic_check.rs:16:18
2424
|
2525
LL | let result = if b > a { 0 } else { a - b };
26-
| ^ help: replace it with: `a.saturating_sub(b)`
26+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `a.saturating_sub(b)`
2727

2828
error: aborting due to 4 previous errors
2929

0 commit comments

Comments
 (0)