suspicious-operation-groupings false positive

[emilk]

Lint name: suspicious-operation-groupings

I tried this code:

pub fn truncated_cone_volume([r0, r1]: [f32; 2], height: f32) -> f32 {
    use std::f32::consts::PI;
    PI * height * (r0 * r0 + r0 * r1 + r1 * r1) / 3.0
}

Latest clippy gives me this:

warning: This sequence of operators looks suspiciously like a bug.
 --> src/lib.rs:3:40
  |
3 |     PI * height * (r0 * r0 + r0 * r1 + r1 * r1) / 3.0
  |                                        ^^^^^^^ help: I think you meant: `PI * r1`
  |
  = note: `#[warn(clippy::suspicious_operation_groupings)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_operation_groupings

(playground link: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=4dbce29a2cf5bed0efc48214538bea53)

---

Minimal reproduce:

pub fn false_positive(r0: f32, r1: f32) -> f32 {
    r0 * r0 + r0 * r1 + r1 * r1
}

2 |     r0 * r0 + r0 * r1 + r1 * r1
  |                         ^^^^^^^ help: I think you meant: `r0 * r1`

---

I don't know how this lint is implemented, but maybe it should only trigger if the number of variables involved is the same as the number of binary expressions?

For instance, this is a true positive:

pub fn true_positive(r0: f32, r1: f32, r2: f32) -> f32 {
    r0 * r0 + r1 * r1 + r2 * r1
}

…although the suggestion is probably not the best:

2 |     r0 * r0 + r1 * r1 + r2 * r1
  |               ^^^^^^^ help: I think you meant: `r0 * r1`

[soniasingla]

I am working on this issue 🎤

[soniasingla]

@rustbot claim

[warycat]

I have similar issues.

error: This sequence of operators looks suspiciously like a bug.
  --> rustgym/src/leetcode/d16/_1620_coordinate_with_maxium_network_quality.rs:15:68
   |
15 |                     if (x - xi) * (x - xi) + (y - yi) * (y - yi) > radius * radius {
   |                                                                    ^^^^^^^^^^^^^^^ help: I think you meant: `x * radius`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_operation_groupings

[warycat]

  --> rustgym/src/leetcode/d0/_97_interleaving_string.rs:36:20
   |
36 |                 && s1[i] == s3[k]
   |                    ^^^^^^^^^^^^^^ help: I think you meant: `s1[i] == s3[i]`
   |

[sanbox-irl]

Agreed, similar issue here:

    fn aabb_collision(bb: Vec4, coord: Vec2) -> bool {
        !(coord.x < bb.x || coord.y < bb.y || coord.x >= bb.z || coord.y >= bb.w)
    }

Wants to change coord.x to coord.z (which ofc won't compile)

[Ryan1729]

Original author of the suspicious_operation_groupings lint here.

@soniasingla, Are you still working on this?

I know that writing this lint was more difficult than I initially expected it to be, so I suspect that fixing this issue would also be more difficult than one might expect from the issue description!

If you're still working on it, then please let me know if you'd like any help understanding the code I wrote.

[jmerdich]

Here's another false positive to add to the list. This one was emitted twice at the same location as well (only one shown for brevity).

warning: this sequence of operators looks suspiciously like a bug
  --> src\main.rs:89:29
   |
89 |         let discriminant = (half_b * half_b) - (a * c);
   |                             ^^^^^^^^^^^^^^^ help: did you mean: `a * half_b`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_operation_groupings

warning: 2 warnings emitted

    Finished dev [unoptimized + debuginfo] target(s) in 0.30s
PS C:\Users\jmerdich\workspace\rt_weekend> cargo clippy -V
clippy 0.1.52 (07e0e2ec 2021-03-24)

While I can see this lint being useful for some cases, this conceptually seems like it could be very error-prone, and I am surprised it's enabled by default. I imagine it works well for basic data structures (eg, impl Eq for Foo), but any non-trivial number crunching would probably trip this somewhere.