manual_div_ceil: detect unsigned alternate form

[cyphercodes]

Fixes #16940

changelog: [manual_div_ceil]: also detect unsigned (x - 1) / y + 1 forms

Summary

• Detect (x - 1) / y + 1 and 1 + (x - 1) / y as manual_div_ceil for unsigned integers.
• Keep signed integers excluded for this form because it is not equivalent for negative values.

Test plan

• cargo fmt --check
• TESTNAME=manual_div_ceil cargo uibless
• TESTNAME=manual_div_ceil cargo uitest
• git diff --check

[rustbot]

r? @dswij

rustbot has assigned @dswij.
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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: 7 candidates
• 7 candidates expanded to 7 candidates
• Random selection from Jarcho, dswij, llogiq

[Gri-ffin]

nice work trying to catch (a - 1) / b + 1 and 1 + (a - 1) / b forms but there is still a gotcha. aside from the signed ints quirks, this pattern still doesn't reliably match with div_ceil unless a > 0 for unsigned ints, if a == 0, (a - 1) underflows while a.div_ceil(b) returns 0, take this as an example:

use std::hint::black_box;
fn main() {
    let a: u32 = black_box(0);
    let b: u32 = black_box(5);
    // println!("{}", a.div_ceil(b));
    // println!("{}", (a - 1) / b + 1);
}

here a.div_ceil(b) will give us zero but (a - 1) / b + 1 will either fail to compile if the compiler can see it’s zero, or panic in debug at runtime, or wrap to a big num in release at runtime.

[Gri-ffin]

if we truly want to lint on the alternate form, i think this should be at minimum MaybeIncorrect in that case.

View changes since the review

[Gri-ffin]

this assumes x > 0 without proving it.

View changes since the review

[Gri-ffin]

this doesn't account for dividend == 0

View changes since the review

[ronnodas]

aside from the signed ints quirks, this pattern still doesn't reliably match with div_ceil unless a > 0 for unsigned ints, if a == 0, (a - 1) underflows while a.div_ceil(b) returns 0 ...

If you don't ignore overflow, (a + b - 1) / b is also not the same as div_ceil. In both cases it seems exceedingly unlikely that the overflow/panic behavior was the intended one (and allowing the lint is probably the best if that is the case). I'm not sure what the criterion for MaybeIncorrect is, but it should apply equally to both forms.

[Gri-ffin]

that is not really my main point, the existing behavior already takes account for both signed and unsigned integers compared to this, linting for only a > 0 already suggests that the pattern is not a clean equivalent of div_ceil which just undermines the whole point of the lint, now we also need to document "we lint this form but only for unsigned vals and only when a > 0 implicitly, and only if you're okay with the behavior changing from a panic to a return value of 0." which is just a lot of asterisks and basically a disclaimer, i do agree about the overflowing, but this edge case still remain far more unlikely than a == 0, and even then it's actually corrected by the suggestion of the lint unlike this form which might silent return values that hide bugs.

[ronnodas]

I agree that a + b - 1 overflowing is much less likely than a - 1 underflowing, but we should lint even if a could be 0, because it seems likely that the underflow is accidental if the a - 1 is part of such an expression.

It's plausible that the user could want the behavior of:

if a == 0 {
    panic!()
else {
    a.div_ceil(b)
}

But in that case it seems reasonable to recommend explicitness about that (possibly by allowing the lint).

[rustbot]

☔ The latest upstream changes (possibly #15793) made this pull request unmergeable. Please resolve the merge conflicts.