spurious suspicious_arithmetic_impl

[apoelstra]

Example https://gist.github.com/apoelstra/d44c7b0244c1a4f87560a4b0e5c17f40 gives

error: Suspicious use of binary operator in `Div` impl
   --> src/main.rs:137:33
    |
137 |         let mut shift = my_bits - your_bits;
    |                                 ^
    |
    = note: #[deny(suspicious_arithmetic_impl)] on by default
    = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#suspicious_arithmetic_impl

error: Suspicious use of binary operator in `Div` impl
   --> src/main.rs:138:33
    |
138 |         shift_copy = shift_copy << shift;
    |                                 ^^
    |
    = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#suspicious_arithmetic_impl

error: Suspicious use of binary operator in `Div` impl
   --> src/main.rs:142:37
    |
142 |                 sub_copy = sub_copy - shift_copy;
    |                                     ^
    |
    = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#suspicious_arithmetic_impl

error: Suspicious use of binary operator in `Div` impl
   --> src/main.rs:144:37
    |
144 |             shift_copy = shift_copy >> 1;
    |                                     ^^
    |
    = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#suspicious_arithmetic_impl

It is possible to implement division using only division (at the very least, alongside #2545 you can use <= and / to extract bits and do boolean logic on them, so you can do anything ;)), but it isn't really reasonable. Even a more efficient implementation of division would need to use subtraction at least.

[flip1995]

Playground example for people who want to test this by just clicking one button 😉

If the arithmetic impl is that complicated, I think this is a case where you should just put an allow(clippy::suspicious_arithmetic_impl) on the impl.

Nonetheless this is a false positive. If someone has an idea how to detect cases like this, feel free to improve this lint!

[apoelstra]

Fair enough, I'm happy to just disable the lint. I won't be offended if somebody closes this issue (but on the other hand, if there's a way to detect and avoid the false positive, that'd be even better :)).

[cbiffle]

Here's a simpler example showing the false positive. I was implementing a binary counter with a limited number of bits and default-wrapping addition.

    fn add(self, other: Self) -> Self {
        Addr((self.0.wrapping_add(other.0)) & P::addr_mask(), PhantomData)
    }

Clippy is upset by the presence of & in there, even though it's in the context of an addition expression.

[ChrisJefferson]

I also got this, by using + in a Mul implementation. I think this warning should only be given at the most when operators are being applied to self, other, or members of those. In general, an implementation of Mul is just a general Rust function, and for interesting types can do all kinds of things. Here (for example) is my implementation, where the size + 1 was flagged.

impl std::ops::Mul<&Permutation> for &Permutation {
    type Output = Permutation;

    fn mul(self, other: &Permutation) -> Self::Output {
        if self.is_id() {
            other.clone()
        } else if other.is_id() {
            self.clone()
        } else {
            let size = max(self.lmp().unwrap_or(0), other.lmp().unwrap_or(0));
            debug_assert!(size > 0);
            let mut v = vec![0; size + 1];
            for i in 0..size + 1 {
                v[i] = other[self[i]];
            }
            Permutation::from_vec(v)
        }
    }
}

[flip1995]

operators are being applied to self, other, or members of those

That is a great idea!

[Serentty]

This whole lint seems like it's likely to cause more problems than it solves to me. If you're implementing arithmetic operations yourself, chances are that you'll have to do more than simply use the operators that you're trying to implement. It seems like it's trying to fix a very specific bug in a very specific case, and doesn't seem like a good fit for being deny by default.

[l0calh05t]

or members of those

Even that will fail (result in a false positive) for simple cases such as a complex number in polar representation where multiplication requires multiplying the magnitudes and adding the polar components (and optionally applying %). Having run into it yesterday, I agree with @Serentty that it seems quite narrow for a general deny (only seems to make sense for "forwarding" implementations).

@llogiq suggested looking at the types the addition is being applied to yesterday, i.e. are they related (potentially via a later from, into or as), but I am not sure if this wouldn't result in similar issues as to checking if the operator is being applied to members of self/other.

Another option might be to check if it is the only binary operator (other function calls, lets can still be applied) being applied within the function. Wouldn't that cover the original use case while avoiding a majority of false positives?

[bodil]

This just started breaking the nightly builds on my ring buffer implementation because I have a logical index type which implements AddAssign and needs subtraction to be able to map to the physical index. It's much too overeager as it stands, imo.