Skip to content

Commit 898e816

Browse files
amerikrainianamerikrainian
committed
Make checked div work on signed ints and collect multiple divisions
1 parent 1316c0e commit 898e816

File tree

4 files changed

+104
-51
lines changed

4 files changed

+104
-51
lines changed

‎clippy_lints/src/manual_checked_div.rs‎

Lines changed: 63 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,19 @@
1-
use clippy_utils::diagnostics::span_lint_and_sugg;
1+
use clippy_utils::diagnostics::span_lint_and_then;
22
use clippy_utils::sugg::Sugg;
33
use clippy_utils::visitors::{Descend, for_each_expr_without_closures};
44
use clippy_utils::{SpanlessEq, is_integer_literal};
5-
use rustc_errors::Applicability;
6-
use rustc_hir::{BinOpKind, Block, Expr, ExprKind};
5+
use rustc_ast::{LitIntType, LitKind};
6+
use rustc_errors::{Applicability, MultiSpan};
7+
use rustc_hir::{BinOpKind, Block, Expr, ExprKind, UnOp};
78
use rustc_lint::{LateContext, LateLintPass};
89
use rustc_middle::ty;
910
use rustc_session::declare_lint_pass;
11+
use rustc_span::source_map::Spanned;
1012
use std::ops::ControlFlow;
1113

1214
declare_clippy_lint! {
1315
/// ### What it does
14-
/// Detects manual zero checks before dividing unsigned integers, such as `if x != 0 { y / x }`.
16+
/// Detects manual zero checks before dividing integers, such as `if x != 0 { y / x }`.
1517
///
1618
/// ### Why is this bad?
1719
/// `checked_div` already handles the zero case and makes the intent clearer while avoiding a
@@ -35,7 +37,7 @@ declare_clippy_lint! {
3537
#[clippy::version = "1.93.0"]
3638
pub MANUAL_CHECKED_DIV,
3739
nursery,
38-
"manual zero checks before dividing unsigned integers"
40+
"manual zero checks before dividing integers"
3941
}
4042
declare_lint_pass!(ManualCheckedDiv => [MANUAL_CHECKED_DIV]);
4143

@@ -47,44 +49,68 @@ enum NonZeroBranch {
4749

4850
impl LateLintPass<'_> for ManualCheckedDiv {
4951
fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) {
50-
if expr.span.from_expansion() {
51-
return;
52-
}
53-
5452
if let ExprKind::If(cond, then, r#else) = expr.kind
53+
&& !expr.span.from_expansion()
5554
&& let Some((divisor, branch)) = divisor_from_condition(cond)
56-
&& is_unsigned(cx, divisor)
55+
&& is_integer(cx, divisor)
56+
&& let Some(block) = branch_block(then, r#else, branch)
5757
{
58-
let Some(block) = branch_block(then, r#else, branch) else {
59-
return;
60-
};
6158
let mut eq = SpanlessEq::new(cx);
59+
let mut applicability = Applicability::MaybeIncorrect;
60+
let mut divisions = Vec::new();
6261

6362
for_each_expr_without_closures(block, |e| {
6463
if let ExprKind::Binary(binop, lhs, rhs) = e.kind
6564
&& binop.node == BinOpKind::Div
6665
&& eq.eq_expr(rhs, divisor)
67-
&& is_unsigned(cx, lhs)
66+
&& is_integer(cx, lhs)
6867
{
69-
let mut applicability = Applicability::MaybeIncorrect;
70-
let lhs_snip = Sugg::hir_with_applicability(cx, lhs, "..", &mut applicability);
71-
let rhs_snip = Sugg::hir_with_applicability(cx, rhs, "..", &mut applicability);
72-
73-
span_lint_and_sugg(
74-
cx,
75-
MANUAL_CHECKED_DIV,
76-
e.span,
77-
"manual checked division",
78-
"consider using `checked_div`",
79-
format!("{}.checked_div({})", lhs_snip.maybe_paren(), rhs_snip),
80-
applicability,
81-
);
68+
let lhs_snip = Sugg::hir_with_applicability(cx, lhs, "_", &mut applicability);
69+
let rhs_snip = Sugg::hir_with_applicability(cx, rhs, "_", &mut applicability);
70+
let lhs_ty = cx.typeck_results().expr_ty(lhs);
71+
let lhs_sugg = lhs_snip.maybe_paren().to_string();
72+
let type_suffix = if matches!(
73+
lhs.kind,
74+
ExprKind::Lit(Spanned {
75+
node: LitKind::Int(_, LitIntType::Unsuffixed),
76+
..
77+
}) | ExprKind::Unary(
78+
UnOp::Neg,
79+
Expr {
80+
kind: ExprKind::Lit(Spanned {
81+
node: LitKind::Int(_, LitIntType::Unsuffixed),
82+
..
83+
}),
84+
..
85+
}
86+
)
87+
) {
88+
format!("_{lhs_ty}")
89+
} else {
90+
String::new()
91+
};
92+
let suggestion = format!("{lhs_sugg}{type_suffix}.checked_div({rhs_snip})");
93+
divisions.push((e.span, suggestion));
8294

8395
ControlFlow::<(), _>::Continue(Descend::No)
8496
} else {
8597
ControlFlow::<(), _>::Continue(Descend::Yes)
8698
}
8799
});
100+
101+
if !divisions.is_empty() {
102+
let mut spans: Vec<_> = divisions.iter().map(|(span, _)| *span).collect();
103+
spans.push(cond.span);
104+
span_lint_and_then(
105+
cx,
106+
MANUAL_CHECKED_DIV,
107+
MultiSpan::from_spans(spans),
108+
"manual checked division",
109+
|diag| {
110+
diag.multipart_suggestion("consider using `checked_div`", divisions, applicability);
111+
},
112+
);
113+
}
88114
}
89115
}
90116
}
@@ -109,15 +135,12 @@ fn branch_block<'tcx>(
109135
branch: NonZeroBranch,
110136
) -> Option<&'tcx Block<'tcx>> {
111137
match branch {
112-
NonZeroBranch::Then => {
113-
if let ExprKind::Block(block, _) = then.kind {
114-
Some(block)
115-
} else {
116-
None
117-
}
138+
NonZeroBranch::Then => match then.kind {
139+
ExprKind::Block(block, _) => Some(block),
140+
_ => None,
118141
},
119-
NonZeroBranch::Else => match r#else.map(|expr| &expr.kind) {
120-
Some(ExprKind::Block(block, _)) => Some(block),
142+
NonZeroBranch::Else => match r#else?.kind {
143+
ExprKind::Block(block, _) => Some(block),
121144
_ => None,
122145
},
123146
}
@@ -127,6 +150,9 @@ fn is_zero(expr: &Expr<'_>) -> bool {
127150
is_integer_literal(expr, 0)
128151
}
129152

130-
fn is_unsigned(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
131-
matches!(cx.typeck_results().expr_ty(expr).peel_refs().kind(), ty::Uint(_))
153+
fn is_integer(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
154+
matches!(
155+
cx.typeck_results().expr_ty(expr).peel_refs().kind(),
156+
ty::Uint(_) | ty::Int(_)
157+
)
132158
}

‎tests/ui/manual_checked_div.fixed‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,30 +6,31 @@ fn main() {
66

77
// Should trigger lint
88
if b != 0 {
9-
let _result = a.checked_div(b);
109
//~^ manual_checked_div
10+
let _result = a.checked_div(b);
11+
let _another = (a + 1).checked_div(b);
1112
}
1213

1314
if b > 0 {
14-
let _result = a.checked_div(b);
1515
//~^ manual_checked_div
16+
let _result = a.checked_div(b);
1617
}
1718

1819
if b == 0 {
20+
//~^ manual_checked_div
1921
println!("zero");
2022
} else {
2123
let _result = a.checked_div(b);
22-
//~^ manual_checked_div
2324
}
2425

2526
// Should NOT trigger (already using checked_div)
2627
if let Some(result) = b.checked_div(a) {
2728
println!("{result}");
2829
}
2930

30-
// Should NOT trigger (signed integers)
3131
let c = -5i32;
3232
if c != 0 {
33-
let _result = 10 / c;
33+
//~^ manual_checked_div
34+
let _result = 10_i32.checked_div(c);
3435
}
3536
}

‎tests/ui/manual_checked_div.rs‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,30 +6,31 @@ fn main() {
66

77
// Should trigger lint
88
if b != 0 {
9-
let _result = a / b;
109
//~^ manual_checked_div
10+
let _result = a / b;
11+
let _another = (a + 1) / b;
1112
}
1213

1314
if b > 0 {
14-
let _result = a / b;
1515
//~^ manual_checked_div
16+
let _result = a / b;
1617
}
1718

1819
if b == 0 {
20+
//~^ manual_checked_div
1921
println!("zero");
2022
} else {
2123
let _result = a / b;
22-
//~^ manual_checked_div
2324
}
2425

2526
// Should NOT trigger (already using checked_div)
2627
if let Some(result) = b.checked_div(a) {
2728
println!("{result}");
2829
}
2930

30-
// Should NOT trigger (signed integers)
3131
let c = -5i32;
3232
if c != 0 {
33+
//~^ manual_checked_div
3334
let _result = 10 / c;
3435
}
3536
}

‎tests/ui/manual_checked_div.stderr‎

Lines changed: 30 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,23 +1,48 @@
11
error: manual checked division
2-
--> tests/ui/manual_checked_div.rs:9:23
2+
--> tests/ui/manual_checked_div.rs:8:8
33
|
4+
LL | if b != 0 {
5+
| ^^^^^^
6+
LL |
47
LL | let _result = a / b;
5-
| ^^^^^ help: consider using `checked_div`: `a.checked_div(b)`
8+
| ^^^^^
9+
LL | let _another = (a + 1) / b;
10+
| ^^^^^^^^^^^
611
|
712
= note: `-D clippy::manual-checked-div` implied by `-D warnings`
813
= help: to override `-D warnings` add `#[allow(clippy::manual_checked_div)]`
14+
help: consider using `checked_div`
15+
|
16+
LL ~ let _result = a.checked_div(b);
17+
LL ~ let _another = (a + 1).checked_div(b);
18+
|
919

1020
error: manual checked division
11-
--> tests/ui/manual_checked_div.rs:14:23
21+
--> tests/ui/manual_checked_div.rs:14:8
1222
|
23+
LL | if b > 0 {
24+
| ^^^^^
25+
LL |
1326
LL | let _result = a / b;
1427
| ^^^^^ help: consider using `checked_div`: `a.checked_div(b)`
1528

1629
error: manual checked division
17-
--> tests/ui/manual_checked_div.rs:21:23
30+
--> tests/ui/manual_checked_div.rs:19:8
1831
|
32+
LL | if b == 0 {
33+
| ^^^^^^
34+
...
1935
LL | let _result = a / b;
2036
| ^^^^^ help: consider using `checked_div`: `a.checked_div(b)`
2137

22-
error: aborting due to 3 previous errors
38+
error: manual checked division
39+
--> tests/ui/manual_checked_div.rs:32:8
40+
|
41+
LL | if c != 0 {
42+
| ^^^^^^
43+
LL |
44+
LL | let _result = 10 / c;
45+
| ^^^^^^ help: consider using `checked_div`: `10_i32.checked_div(c)`
46+
47+
error: aborting due to 4 previous errors
2348

0 commit comments

Comments
 (0)