New lint: manual_is_multiple_of

[samueltardieu]

I've added a min_divisor configuration option, default to 4, to not trigger on smaller divisibility operations. I would prefer not to lint if a & 1 == 0 as if a.is_multiple_of(2) by default because the former is very idiomatic in systems (and embedded) programming.

A min_and_mask_size option, defaulting to 3, sets the default bits to be and-masked before this lint triggers; that would be n in v & ((1 << n) - 1) == 0. The form v % 10 == 0 is always linted.

This PR will stay in draft mode until the next rustup which will mark unsigned_is_multiple_of stable for Rust 1.87.0, and the feature flags will be removed.

What should its category be? I've used "complexity", but "pedantic" might be suitable as well.

Close #14289
changelog: [manual_is_multiple_of]: new lint
r? ghost

[joshtriplett]

Having a min_divisor configuration option seems reasonable, but I think that option should default to flagging every instance of this. Large codebases that want to keep the & 1 == 0 pattern can set that option.

[samueltardieu]

Having a min_divisor configuration option seems reasonable, but I think that option should default to flagging every instance of this. Large codebases that want to keep the & 1 == 0 pattern can set that option.

I've added an extra commit to test this with lintcheck. There was only one hit in Clippy sources (except for tests).

[samueltardieu]

@joshtriplett The lintcheck output seems quite reasonable indeed.

[y21]

I agree with linting the a % b == 0 pattern, but I'm unsure about the & one... Something like (x & 0b111) == 0 feels like a very general expression.
I'm a bit concerned that linting here has the opposite effect: emitting warnings on code that has a totally different semantic meaning where x.is_multiple_of(8) would be a very obfuscated way of checking that the last 3 bits aren't set.

[joshtriplett]

First of all, please don't treat that as a blocker; it's not as important as the % N == 0 case.

That said: I've seen many, many codebases which use & (PAGE_SIZE - 1) == 0 and similar instead of % PAGE_SIZE == 0, when they know the value is a power of two.

We may need to tune the heuristic to make sure it doesn't produce false positives, and for instance we may want a different threshold for & vs % (& 1 == 0 might be a flag test), but I think things like & 4095 == 0 or & (4096 - 1) == 0 or & (CONST_EQUAL_TO_4096 - 1) == 0 are pretty clear.

Looking at the lintcheck output, I think I agree with not flagging & 1 == 0 by default.

[samueltardieu]

What about applying the min-divisor option (maybe using another name, like min-one-bits to represent the number of ones) only to the &(2^n-1) form of the lint, with a sensible default such as 5 bits? The % form would be linted unconditionally.

[joshtriplett]

@samueltardieu Sounds reasonable. I would say the default should flag 7 and 15 by default, and maybe 3, but not 1. Because 1 might be a flag, but 3 is definitely two bits.

[samueltardieu]

Done, see the toplevel comment for the updated option.

[joshtriplett]

Looking at the lintcheck output, this looks great!

[samueltardieu]

rustup happened, PR ready.
r? clippy

[samueltardieu]

Rebased.

[samueltardieu]

Rebased

[blyxyas]

Very good lint and initial implementation, some nits in this initial reviewing round (some slip-ups)

[samueltardieu]

@blyxyas Should I reassign?

[blyxyas]

I've opened the FCP
https://rust-lang.zulipchat.com/#narrow/channel/257328-clippy/topic/FCP.20manual_is_multiple_of

[samueltardieu]

Removed the & part as well as the min-and-mask-size option, as discussed in the FCP.

Also updated the clippy version to 1.88.

@rustbot label +S-final-comment-period

[samueltardieu]

Rebased to fix UI test error in iter_kv_map.rs.

[samueltardieu]

Rebased

[samueltardieu]

Rebased and set Clippy version to 1.89.

[samueltardieu]

Rebased

[blyxyas]

LGTM, thanks! ❤️

@flip1995 Does this message interpret as "I'd approve this if it didn't have binops" or as "I want binops removed from this lint, but don't approve it yet".

Binary operations are now removed, and I think it looks all good for approving (I need the second +1)

[folkertdev]

Hey, I'm not sure what your process is, do you have some mechanism to make sure this doesn't just sit here forever?

[samueltardieu]

Apart from pinging @blyxyas and @flip1995 in this case, no.

[blyxyas]

I'll revive the thread on Zulip to see if someone else can take a look at this. We just need a secondary approving look (either from Philipp or from some other maintainer, ideally from him because he raised that request).

[samueltardieu]

Rebased (a new UI test merged in the meantime needed to get the lint applied to it).

[samueltardieu]

@blyxyas @flip1995 has given a +1 on Zulip, you should be able to merge it.

[blyxyas]

LGTM, thanks! ❤️