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 @@ -6607,6 +6607,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 @@ -296,6 +296,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 @@ -198,6 +198,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 @@ -857,6 +858,7 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co
Box::new(|_| Box::<replace_box::ReplaceBox>::default()),
Box::new(move |_| Box::new(manual_ilog2::ManualIlog2::new(conf))),
Box::new(|_| Box::new(same_length_and_capacity::SameLengthAndCapacity)),
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
155 changes: 155 additions & 0 deletions clippy_lints/src/manual_checked_div.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
use clippy_utils::diagnostics::span_lint_and_help;
use clippy_utils::visitors::{Descend, for_each_expr_without_closures};
use clippy_utils::{SpanlessEq, is_integer_literal};
use rustc_errors::MultiSpan;
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 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"]
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
#[clippy::version = "1.93.0"]
#[clippy::version = "1.94.0"]

pub MANUAL_CHECKED_DIV,
nursery,
"manual zero checks before dividing 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 let ExprKind::If(cond, then, r#else) = expr.kind
&& !expr.span.from_expansion()
&& let Some((divisor, branch)) = divisor_from_condition(cond)
&& is_unsigned_integer(cx, divisor)
&& let Some(block) = branch_block(then, r#else, branch)
{
// This lint is intended for unsigned integers only.
//
// For signed integers, the most direct refactor to `checked_div` is often not
// semantically equivalent to the original guard. For example, `rhs > 0` deliberately
// excludes negative divisors, while `checked_div` would return `Some` for `rhs = -2`.
// Also, `checked_div` can return `None` for `MIN / -1`, which requires additional handling beyond
// the zero check.
Comment on lines +55 to +61
Copy link
Contributor

Choose a reason for hiding this comment

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

Nicely explained:) I think putting it directly above the is_unsigned_integer check would be helpful, as that's probably the point where a reader might think to themselves "but why unsigned?"

let mut eq = SpanlessEq::new(cx).deny_side_effects().paths_by_resolution();
if !eq.eq_expr(divisor, divisor) {
return;
}

let mut division_spans = Vec::new();
let mut first_use = None;

let found_early_use = 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_integer(cx, lhs)
{
match first_use {
None => first_use = Some(UseKind::Division),
Some(UseKind::Other) => return ControlFlow::Break(()),
Some(UseKind::Division) => {},
}

division_spans.push(e.span);

ControlFlow::<(), _>::Continue(Descend::No)
} else if eq.eq_expr(e, divisor) {
if first_use.is_none() {
first_use = Some(UseKind::Other);
}
Comment on lines +86 to +88
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, couldn't we early-return in this case? If the first use is not a division, then we should avoid linting just to be on the safer side, no?

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

if found_early_use.is_some() || first_use != Some(UseKind::Division) || division_spans.is_empty() {
return;
}

division_spans.push(cond.span);
span_lint_and_help(
cx,
MANUAL_CHECKED_DIV,
MultiSpan::from_spans(division_spans),
"manual checked division",
None,
"consider using `checked_div`",
);
Comment on lines +99 to +107
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, this generates a diagnostic like this:

error: manual checked division
  --> tests/ui/manual_checked_div.rs:8:8
   |
LL |     if b != 0 {
   |        ^^^^^^
LL |
LL |         let _result = a / b;
   |                       ^^^^^
LL |         let _another = (a + 1) / b;
   |                        ^^^^^^^^^^^
   |
   = help: consider using `checked_div`

Imo, adding some labels here would be nice -- something like this:

error: manual checked division
  --> tests/ui/manual_checked_div.rs:8:8
   |
LL |     if b != 0 {
   |        ^^^^^^ check performed here
LL |
LL |         let _result = a / b;
   |                       ^^^^^ division performed here
LL |         let _another = (a + 1) / b;
   |                        ^^^^^^^^^^^ division performed here (alternatively: "... and here")
   |
   = help: consider using `checked_div`

WDYT @amerikrainian @samueltardieu?

}
}
}

#[derive(Copy, Clone, Eq, PartialEq)]
enum UseKind {
Division,
Other,
}

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 => match then.kind {
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_integer(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
matches!(cx.typeck_results().expr_ty(expr).peel_refs().kind(), ty::Uint(_))
}
80 changes: 80 additions & 0 deletions tests/ui/manual_checked_div.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
#![warn(clippy::manual_checked_div)]

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

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

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

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

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

// Should NOT trigger (signed integers are not linted)
let c = -5i32;
if c != 0 {
let _result = 10 / c;
}

// Should NOT trigger (side effects in divisor)
if counter() > 0 {
let _ = 32 / counter();
}

// Should NOT trigger (divisor used before division)
if b > 0 {
use_value(b);
let _ = a / b;
}

let signed_min = i32::MIN;
let mut signed_div: i32 = -1;

// Should NOT trigger (signed integers are not linted)
if signed_div != 0 {
let _ = signed_min / signed_div;
}

signed_div = 2;

if signed_div > 0 {
let _ = signed_min / signed_div;
}
Comment on lines +48 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.

I think we can now remove these in favor of the signed test above (the one with let c = -5i32;)


// Should NOT trigger (divisor may change during evaluation)
if b > 0 {
g(inc_and_return_value(&mut b), a / b);
}
}

fn counter() -> u32 {
println!("counter");
1
}

fn use_value(_v: u32) {}

fn inc_and_return_value(x: &mut u32) -> u32 {
*x += 1;
*x
}

fn g(_lhs: u32, _rhs: u32) {}
39 changes: 39 additions & 0 deletions tests/ui/manual_checked_div.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
error: manual checked division
--> tests/ui/manual_checked_div.rs:8:8
|
LL | if b != 0 {
| ^^^^^^
LL |
LL | let _result = a / b;
| ^^^^^
LL | let _another = (a + 1) / b;
| ^^^^^^^^^^^
|
= help: consider using `checked_div`
= 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:8
|
LL | if b > 0 {
| ^^^^^
LL |
LL | let _result = a / b;
| ^^^^^
|
= help: consider using `checked_div`

error: manual checked division
--> tests/ui/manual_checked_div.rs:19:8
|
LL | if b == 0 {
| ^^^^^^
...
LL | let _result = a / b;
| ^^^^^
|
= help: consider using `checked_div`

error: aborting due to 3 previous errors