Add manual_checked_div lint

[amerikrainian]

changelog: [manual_checked_div]: new lint suggesting checked_div instead of manual zero checks before unsigned division

Implements the manual checked div from #12894. I'm relatively new to Rust and a complete newbie at Clippy, so my apologies if I forgot anything. I looked through the linked issue and could not find anything talking about the above lint.

[rustbot]

r? @samueltardieu

rustbot has assigned @samueltardieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[github-actions]

No changes for c9dc19d

[ada4a]

Extremely impressive for a "complete newbie at Clippy"! Left a couple starting comments

View changes since this review

[samueltardieu]

@amerikrainian Could you look at, and probably apply, @ada4a's suggestions?
@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[amerikrainian]

@amerikrainian Could you look at, and probably apply, @ada4a's suggestions? @rustbot author

Yes. I'm sorry. I'm a student and we're in the finals season, so I am deep in the trenches until tomorrow; will do my best to get this done before Friday.

[samueltardieu]

No hurry, I just wanted to make sure you noticed them. Take your time.

[amerikrainian]

Applied @ada4a’s suggestions and rebased.

I also went ahead and gave multiple divisions implementation a shot; it turned out to be rather straightforward, although I'm not sure if the current form is what was intended.

I promise future feedback won't take as long to be applied.

@rustbot ready

[samueltardieu]

This is a good initial shot. Some risks of false positives need to be fixed though.

As a general rule, dividend() / divisor() can be linted only if you can prove that divisor() is identical between the check and the division, and if divisor() has no side-effect. For example, the following code should not be linted as it would not give the same result:

fn counter() -> u32 { /* Some code which increments and return a counter */ }

if counter() > 0 {
    let a = 32 / counter();
    println!("{}", a);  // This divides by the value of the counter as incremented twice
}

Also, if the divisor is used separately from the division, it might not be easy to refactor with .checked_div(), as in:

    if b > 0 {
        do_something_with(b);
        f(a / b);
    }

Another, more tortuous, example, would be, with a mutable b:

    if b > 0 {
        g(inc_and_return_value(&mut b), a / b);
    }

In the latest example, the value of b may have changed while evaluating the first argument of g(), so computing the division first using .checked_div() might not be appropriate.

In short, I think you should trigger the lint only if the division is the first use of the divisor in the "then" part of the if, and if the divisor is an expression that cannot change between the check and the division (checking for the absence of function calls should be fine).

Also, it would be great if /= could be checked the same way. And maybe % and %=, with checked_rem(), since the same logic applies.

View changes since this review

[amerikrainian]

I'm happy to update this for the /=, %, and %= cases, I just wanted to ensure that this is what you had in mind. I updated the tests with the listed negative examples and enforced first use/purity of divisor expressions since I think it's more trickier and extension to other ops should be mechanical and straightforward. Is this what you intended?

@rustbot ready

[Jarcho]

You can't lint signed and unsigned values with the same values since MIN / -1 also returns None in checked_div.

[amerikrainian]

So I gave this some thought, and we can make it so that for signed, only accept conditions of the form b > 0 or 0 < b (depending on which side zero is on). We would then skip linting if the condition is b != 0, b == 0, or anything else. Again, this is for signed. Is this what you had in mind? Any other cases I should be aware of?

[Jarcho]

The only additional case returning None is MIN / -1 so you'll have to check for that in addition to the rhs being zero. You can't change the meaning of the code for a lint like this if it's enabled by default.

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[samueltardieu]

I am concerned about the suggestions here. They change the type, and only happen to compile because you don't do anything with the result and you don't use explicit types on the variables holding the result of the division.

I think the lint should hint at using .checked_div() as a help, not as a suggestion which wouldn't compile.

Can you please add the following two tests?

fn arbitrary_signed(lhs: i32, rhs: i32) -> i32 {
    if rhs != 0 {
        lhs / rhs
    } else {
        lhs
    }
}

fn arbitrary_unsigned(lhs: u32, rhs: u32) -> u32 {
    if rhs != 0 {
        //~^ manual_checked_div
        lhs / rhs
    } else {
        lhs
    }
}

View changes since this review

[amerikrainian]

I am concerned about the suggestions here. They change the type, and only happen to compile because you don't do anything with the result and you don't use explicit types on the variables holding the result of the division.

I think the lint should hint at using .checked_div() as a help, not as a suggestion which wouldn't compile.

Can you please add the following two tests?

fn arbitrary_signed(lhs: i32, rhs: i32) -> i32 {
    if rhs != 0 {
        lhs / rhs
    } else {
        lhs
    }
}

fn arbitrary_unsigned(lhs: u32, rhs: u32) -> u32 {
    if rhs != 0 {
        //~^ manual_checked_div
        lhs / rhs
    } else {
        lhs
    }
}

View changes since this review

Just to ensure I'm reading this right: the lint shouldn't just be "fixable" because we possibly conflate types? I.e, the lint should just now give text like, "consider using checked_div and handling the Option result" and do not offer any actual fixes. In retrospect this makes sense, but I want to make sure I'm understanding you correctly.

[samueltardieu]

I am concerned about the suggestions here. They change the type, and only happen to compile because you don't do anything with the result and you don't use explicit types on the variables holding the result of the division.
I think the lint should hint at using .checked_div() as a help, not as a suggestion which wouldn't compile.
Can you please add the following two tests?

fn arbitrary_signed(lhs: i32, rhs: i32) -> i32 {
    if rhs != 0 {
        lhs / rhs
    } else {
        lhs
    }
}

fn arbitrary_unsigned(lhs: u32, rhs: u32) -> u32 {
    if rhs != 0 {
        //~^ manual_checked_div
        lhs / rhs
    } else {
        lhs
    }
}

View changes since this review

Just to ensure I'm reading this right: the lint shouldn't just be "fixable" because we possibly conflate types? I.e, the lint should just now give text like, "consider using checked_div and handling the Option result" and do not offer any actual fixes. In retrospect this makes sense, but I want to make sure I'm understanding you correctly.

Yes, unless you can find a way to give a proper fix. For example, if the result of the division is assigned to a new binding, then you could suggest replacing

if rhs != 0 {
   let d = lhs / rhs;
   do_something_with(d);
}

by

if let Some(d) = lhs.checked_div(rhs) {
    do_something_with(d);
}

But those are probably rare cases, as we usually don't want to invent new binding names as this is not easy to do.

Also, thinking more about signed types, I'm not sure we want to transform (or suggest transforming)

if rhs > 0 {
   let d = lhs / rhs;
   do_something_with(d);
}

into code using .checked_div(), because if rhs is -2, the code above would not trigger, while it would return Some() with .checked_div(), so getting an equivalent code path may prove difficult. Wouldn't it be safer to only lint unsigned divisions? Not only there would be no MIN/-1 problem, but also the suggested refactoring (even without machine applicable suggestion) would better fit the original code.

[amerikrainian]

I am concerned about the suggestions here. They change the type, and only happen to compile because you don't do anything with the result and you don't use explicit types on the variables holding the result of the division.
I think the lint should hint at using .checked_div() as a help, not as a suggestion which wouldn't compile.
Can you please add the following two tests?

fn arbitrary_signed(lhs: i32, rhs: i32) -> i32 {
    if rhs != 0 {
        lhs / rhs
    } else {
        lhs
    }
}

fn arbitrary_unsigned(lhs: u32, rhs: u32) -> u32 {
    if rhs != 0 {
        //~^ manual_checked_div
        lhs / rhs
    } else {
        lhs
    }
}

View changes since this review

Just to ensure I'm reading this right: the lint shouldn't just be "fixable" because we possibly conflate types? I.e, the lint should just now give text like, "consider using checked_div and handling the Option result" and do not offer any actual fixes. In retrospect this makes sense, but I want to make sure I'm understanding you correctly.

Yes, unless you can find a way to give a proper fix. For example, if the result of the division is assigned to a new binding, then you could suggest replacing

if rhs != 0 {
   let d = lhs / rhs;
   do_something_with(d);
}

by

if let Some(d) = lhs.checked_div(rhs) {
    do_something_with(d);
}

But those are probably rare cases, as we usually don't want to invent new binding names as this is not easy to do.

Also, thinking more about signed types, I'm not sure we want to transform (or suggest transforming)

if rhs > 0 {
   let d = lhs / rhs;
   do_something_with(d);
}

into code using .checked_div(), because if rhs is -2, the code above would not trigger, while it would return Some() with .checked_div(), so getting an equivalent code path may prove difficult. Wouldn't it be safer to only lint unsigned divisions? Not only there would be no MIN/-1 problem, but also the suggested refactoring (even without machine applicable suggestion) would better fit the original code.

I'll see what I can do re, unsigned.

Regarding signed, I originally had it as only unsigned, but @ada4a asked whether signed integers would be reasonable ("I see that the original issue only talks about unsigned integers, but I think signed ones should be fine as well?"). Unless I misunderstood the comment. I'm happy to revert to linting only unsigned as beforehand. If I did misunderstand, could you clarify the meaning of the comment?

Thank you for your patience and time.

[ada4a]

Apologies for the churn, I was indeed the one who brought this up – though tbf I wasn't suggesting to add it, just wondering why it's excluded... but I guess we have the answer to that now.

It might be worth it to put @samueltardieu's explanation as a comment somewhere in the lint code (as I'm not sure it makes sense to put it into the lint description itself)

[amerikrainian]

I've implemented the suggestions as discussed above; please let me know what to fix/edit (still not entirely happy with the explanatory comment but that's all I could come up with without writing paragraphs).

@rustbot ready.

[samueltardieu]

FCP thread opened on Zulip.