Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6530,6 +6530,7 @@ Released 2018-09-13
[`manual_async_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_async_fn
[`manual_bits`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_bits
[`manual_c_str_literals`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_c_str_literals
[`manual_checked_div`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_checked_div
[`manual_clamp`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_clamp
[`manual_contains`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_contains
[`manual_dangling_ptr`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_dangling_ptr
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
crate::manual_assert::MANUAL_ASSERT_INFO,
crate::manual_async_fn::MANUAL_ASYNC_FN_INFO,
crate::manual_bits::MANUAL_BITS_INFO,
crate::manual_checked_div::MANUAL_CHECKED_DIV_INFO,
crate::manual_clamp::MANUAL_CLAMP_INFO,
crate::manual_float_methods::MANUAL_IS_FINITE_INFO,
crate::manual_float_methods::MANUAL_IS_INFINITE_INFO,
Expand Down
2 changes: 2 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,7 @@ mod manual_abs_diff;
mod manual_assert;
mod manual_async_fn;
mod manual_bits;
mod manual_checked_div;
mod manual_clamp;
mod manual_float_methods;
mod manual_hash_one;
Expand Down Expand Up @@ -848,6 +849,7 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co
Box::new(|_| Box::new(toplevel_ref_arg::ToplevelRefArg)),
Box::new(|_| Box::new(volatile_composites::VolatileComposites)),
Box::new(|_| Box::<replace_box::ReplaceBox>::default()),
Box::new(|_| Box::new(manual_checked_div::ManualCheckedDiv)),
// add late passes here, used by `cargo dev new_lint`
];
store.late_passes.extend(late_lints);
Expand Down
132 changes: 132 additions & 0 deletions clippy_lints/src/manual_checked_div.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::sugg::Sugg;
use clippy_utils::visitors::{Descend, for_each_expr_without_closures};
use clippy_utils::{SpanlessEq, is_integer_literal};
use rustc_errors::Applicability;
use rustc_hir::{BinOpKind, Block, Expr, ExprKind};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty;
use rustc_session::declare_lint_pass;
use std::ops::ControlFlow;

declare_clippy_lint! {
/// ### What it does
/// Detects manual zero checks before dividing unsigned integers, such as `if x != 0 { y / x }`.
///
/// ### Why is this bad?
/// `checked_div` already handles the zero case and makes the intent clearer while avoiding a
/// panic from a manual division.
///
/// ### Example
/// ```no_run
/// # let (a, b) = (10u32, 5u32);
/// if b != 0 {
/// let result = a / b;
/// println!("{result}");
/// }
/// ```
/// Use instead:
/// ```no_run
/// # let (a, b) = (10u32, 5u32);
/// if let Some(result) = a.checked_div(b) {
/// println!("{result}");
/// }
/// ```
#[clippy::version = "1.93.0"]
pub MANUAL_CHECKED_DIV,
nursery,
"manual zero checks before dividing unsigned integers"
}
declare_lint_pass!(ManualCheckedDiv => [MANUAL_CHECKED_DIV]);

#[derive(Copy, Clone)]
enum NonZeroBranch {
Then,
Else,
}

impl LateLintPass<'_> for ManualCheckedDiv {
fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) {
if expr.span.from_expansion() {
return;
}
Comment on lines +50 to +52
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be moved into the let-chain as well


if let ExprKind::If(cond, then, r#else) = expr.kind
&& let Some((divisor, branch)) = divisor_from_condition(cond)
&& is_unsigned(cx, divisor)
{
let Some(block) = branch_block(then, r#else, branch) else {
return;
};
Comment on lines +58 to +60
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be moved into the let-chain as well

let mut eq = SpanlessEq::new(cx);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine to inline


for_each_expr_without_closures(block, |e| {
if let ExprKind::Binary(binop, lhs, rhs) = e.kind
&& binop.node == BinOpKind::Div
&& eq.eq_expr(rhs, divisor)
&& is_unsigned(cx, lhs)
{
let mut applicability = Applicability::MaybeIncorrect;
let lhs_snip = Sugg::hir_with_applicability(cx, lhs, "..", &mut applicability);
let rhs_snip = Sugg::hir_with_applicability(cx, rhs, "..", &mut applicability);
Comment on lines +70 to +71
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For single expressions, the preferred placeholder is "_"

Suggested change
let lhs_snip = Sugg::hir_with_applicability(cx, lhs, "..", &mut applicability);
let rhs_snip = Sugg::hir_with_applicability(cx, rhs, "..", &mut applicability);
let lhs_snip = Sugg::hir_with_applicability(cx, lhs, "_", &mut applicability);
let rhs_snip = Sugg::hir_with_applicability(cx, rhs, "_", &mut applicability);


span_lint_and_sugg(
cx,
MANUAL_CHECKED_DIV,
e.span,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be nice to also highlight the span where the variable (let's call it b) is checked for being non-zero, which you could do by passing MultiSpan::new(vec![cond.span, e.span]).

But with that, if there were multiple instances of a / b (however unlikely that is), each firing of the lint would show the said checking place, which would be suboptimal. Because of this, a better solution could be to first collect all the instances of a / b (where each one might have a different a!), then lint on all of them at once (if there are any, of course). Now that I've written all this, it does sound like a bit too much work -- if you want to give it a try, feel free, but otherwise just highlighting both the condition and the division would be a good start.

"manual checked division",
"consider using `checked_div`",
format!("{}.checked_div({})", lhs_snip.maybe_paren(), rhs_snip),
applicability,
);

ControlFlow::<(), _>::Continue(Descend::No)
} else {
ControlFlow::<(), _>::Continue(Descend::Yes)
}
});
}
}
}

fn divisor_from_condition<'tcx>(cond: &'tcx Expr<'tcx>) -> Option<(&'tcx Expr<'tcx>, NonZeroBranch)> {
let ExprKind::Binary(binop, lhs, rhs) = cond.kind else {
return None;
};

match binop.node {
BinOpKind::Ne | BinOpKind::Lt if is_zero(lhs) => Some((rhs, NonZeroBranch::Then)),
BinOpKind::Ne | BinOpKind::Gt if is_zero(rhs) => Some((lhs, NonZeroBranch::Then)),
BinOpKind::Eq if is_zero(lhs) => Some((rhs, NonZeroBranch::Else)),
BinOpKind::Eq if is_zero(rhs) => Some((lhs, NonZeroBranch::Else)),
_ => None,
}
}

fn branch_block<'tcx>(
then: &'tcx Expr<'tcx>,
r#else: Option<&'tcx Expr<'tcx>>,
branch: NonZeroBranch,
) -> Option<&'tcx Block<'tcx>> {
match branch {
NonZeroBranch::Then => {
if let ExprKind::Block(block, _) = then.kind {
Some(block)
} else {
None
}
},
NonZeroBranch::Else => match r#else.map(|expr| &expr.kind) {
Some(ExprKind::Block(block, _)) => Some(block),
_ => None,
},
Comment on lines +119 to +122
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
NonZeroBranch::Else => match r#else.map(|expr| &expr.kind) {
Some(ExprKind::Block(block, _)) => Some(block),
_ => None,
},
NonZeroBranch::Else => match r#else?.kind {
ExprKind::Block(block, _) => Some(block),
_ => None,
},

}
}

fn is_zero(expr: &Expr<'_>) -> bool {
is_integer_literal(expr, 0)
}

fn is_unsigned(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
matches!(cx.typeck_results().expr_ty(expr).peel_refs().kind(), ty::Uint(_))
}
35 changes: 35 additions & 0 deletions tests/ui/manual_checked_div.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
#![warn(clippy::manual_checked_div)]

fn main() {
let a = 10u32;
let b = 5u32;

// Should trigger lint
if b != 0 {
let _result = a.checked_div(b);
//~^ manual_checked_div
}

if b > 0 {
let _result = a.checked_div(b);
//~^ manual_checked_div
}

if b == 0 {
println!("zero");
} else {
let _result = a.checked_div(b);
//~^ manual_checked_div
}

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

// Should NOT trigger (signed integers)
let c = -5i32;
if c != 0 {
let _result = 10 / c;
}
}
35 changes: 35 additions & 0 deletions tests/ui/manual_checked_div.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
#![warn(clippy::manual_checked_div)]

fn main() {
let a = 10u32;
let b = 5u32;

// Should trigger lint
if b != 0 {
let _result = a / b;
//~^ manual_checked_div
}

if b > 0 {
let _result = a / b;
//~^ manual_checked_div
}

if b == 0 {
println!("zero");
} else {
let _result = a / b;
//~^ manual_checked_div
}

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

// Should NOT trigger (signed integers)
let c = -5i32;
if c != 0 {
let _result = 10 / c;
}
Comment on lines +30 to +34
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, why exactly do we not lint here?... I see that the original issue only talks about unsigned integers, but I think signed ones should be fine as well?

}
23 changes: 23 additions & 0 deletions tests/ui/manual_checked_div.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
error: manual checked division
--> tests/ui/manual_checked_div.rs:9:23
|
LL | let _result = a / b;
| ^^^^^ help: consider using `checked_div`: `a.checked_div(b)`
|
= note: `-D clippy::manual-checked-div` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::manual_checked_div)]`

error: manual checked division
--> tests/ui/manual_checked_div.rs:14:23
|
LL | let _result = a / b;
| ^^^^^ help: consider using `checked_div`: `a.checked_div(b)`

error: manual checked division
--> tests/ui/manual_checked_div.rs:21:23
|
LL | let _result = a / b;
| ^^^^^ help: consider using `checked_div`: `a.checked_div(b)`

error: aborting due to 3 previous errors