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.

[1tgr]

I am seeing a similar set of false positives from this lint after a toolchain upgrade, all relating to n * n terms:

warning: this sequence of operators looks suspiciously like a bug
 --> src/lib.rs:2:5
  |
2 |     vol * vol * (b - c) / 2.0 / mr
  |     ^^^^^^^^^ help: did you mean: `b * vol`
  |
  = 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

I see the benefit of this lint for logical operators (per the example code, self.x == other.y && self.y == other.y && self.z == other.z). But I think it's too ambitious to extend it to arithmetic operators like this.

[Ryan1729]

Original author of this lint here, again.

When I originally wrote the lint, my life circumstances were such that I had more time to spend on things like this. I thought that circumstances might change back fairly soon, but it appears that will not be the case, at least for a while.

Given that, here's some options I considered for addressing the false positive issues:

• I've already marked certain ways that a lint can be emitted as MaybeIncorrect via the Applicability enum. Someone could fairly easily add a check to only emit lints when the value of applicability is MachineApplicable in the local emit_suggestion function. The expected output of the tests would need to be updated, since that change to the code would stop some true positives from being found.
• Redesign it so there are fewer false positives. For example, if something like https://github.com/rust-lang/rust-clippy/pull/3875 were implemented, one could probably define very strict patterns that should always be flagged.

As the author of the lint, I have a mild preference for it to used. But I will not be offended if anyone makes changes to the lint, including things like making it disabled by default, on the off-chance that was what was stopping anyone from making a PR. :slightly_smiling_face:

[uazu]

This one hit me too. Unless clippy can learn mathematics, it seems too ambitious to get it to recommend changes to my code calculating derivatives of a function. I mean, my maths might be wrong, I accept that -- but it seems clear that following clippy's suggestions would only make it worse. At least get it to do some basic dimensional analysis!