integer_arithmetic rule is too simplistic and is triggered even when arithmetic operations are applied to constants/literals that cannot cause overflow or panic

[mimoo]

Hello, the following code triggers the integer-arithmetic rule:

warning: integer arithmetic detected
  --> client/json-rpc/src/blocking/client.rs:89:44
   |
89 |                 if expiration_time_secs <= state.timestamp_usecs / 1_000_000 {
   |                                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: requested on the command line with `-W clippy::integer-arithmetic`
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#integer_arithmetic

I'm not sure how useful it is, as there is no danger here IIUC. The only edge-case is division by 0 right?

[ebroto]

According to the Reference

Using / or %, where the left-hand argument is the smallest integer of a signed integer type and the right-hand argument is -1.

So this will overflow

fn main() {
    let _ = std::i32::MIN / -1;
}

(playground)

We could special case though if the RHS of a binary operation / or % is a literal/constant that is not 0 or -1, there should be no risk of overflow/panic.

[mimoo]

yeah, I don't think it's a good idea to ask people to use checked_div and checked_rem when we know that there's no chance of bug there : o

[mlegner]

This would also apply to other operations when both sides are literals/constants, right? For example, there are also no issues when writing 1+1 (which currently triggers the lint). So this issue could be rephrased to "integer-arithmetic rule is too simplistic and is triggered even when arithmetic operations are applied to constants/literals that cannot cause overflow or panic".

Ideally, the lint would check if one or both operands are literals and, if yes, check if they cause overflow/panic.

[ebroto]

This would also apply to other operations when both sides are literals/constants, right?

Indeed, and we could improve that too, but I think it will be a bit more involved than the cases with division and remainder. I'm going to add the E-medium tag for this second potential enhancement.

[mlegner]

After looking into this some more, I noticed that rustc already raises errors whenever there is definitely an overflow because the right operand (in case of division/remainder by 0 and left/right shift) or both operands (division by -1, addition, subtraction, multiplication) are constants/literals. So we could simply not trigger the lint in all these cases in addition to restricting it for division/remainder.

For multiplication, addition, subtraction, both operands need to be known to rule out overflows; except if one is known to be 1 (multiplication) or 0 (addition, subtraction), which anyways trigger the identity_op lint.

For the potential overflow of division and remainder in case the left-hand argument is the smallest integer of a signed integer type and the right-hand argument is -1, either one of the operands can potentially rule out the overflow. (Side note/question: Why is there an overflow when calculating the remainder with -1 anyway? The result is always 0, right?)

[mlegner]

@ebroto If you agree with my assessment and suggestions, I'm happy to raise a PR fixing this.

[mimoo]

If that helps, I also got this kind of false positive:

warning: integer arithmetic detected
   --> diem-node/src/lib.rs:231:40
    |
231 |         std::time::Duration::from_secs(60 * 60),
    |                                        ^^^^^^^
    |
    = note: requested on the command line with `-W clippy::integer-arithmetic`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#integer_arithmetic

[c410-f3r]

This issue can be closed now