Auto merge of #8884 - evantypanski:manual_range_contains_multiple, r=Manishearth

Fix `manual_range_contains` false negative with chains of `&&` and `||`

Fixes #8745

Since the precedence for `&&` is the same as itself the HIR for a chain of `&&` ends up with a right skewed tree like:

```
     &&
    /  \
  &&   c2
 /  \
... c1
```

So only the leftmost `&&` was actually "fully" checked, the top level was just `c2` and `&&` so the `manual_range_contains` lint won't apply. This change makes it also check `c2` with `c1`.

There's a bit of a hacky solution in the [second commit](257f097) to check if the number of open/closing parens in the snippet match. This is to prevent a case like `((x % 2 == 0) || (x < 0)) || (x >= 10)` from offering a suggestion like `((x % 2 == 0) || !(0..10).contains(&x)` which now won't compile.

Any suggestions for that paren hack welcome, kinda new to working on this so not too sure about possible solutions :) it's weird because I don't know how else to check for parens in HIR considering they're removed when lowering AST.

changelog: Fix [`manual_range_contains`] false negative with chains of `&&` and `||`

Author: bors

clippy_lints/src/ranges.rs

@@ -194,7 +194,7 @@ impl<'tcx> LateLintPass<'tcx> for Ranges {
             },
             ExprKind::Binary(ref op, l, r) => {
                 if meets_msrv(self.msrv, msrvs::RANGE_CONTAINS) {
-                    check_possible_range_contains(cx, op.node, l, r, expr);
+                    check_possible_range_contains(cx, op.node, l, r, expr, expr.span);
                 }
             },
             _ => {},
@@ -213,12 +213,12 @@ fn check_possible_range_contains(
     left: &Expr<'_>,
     right: &Expr<'_>,
     expr: &Expr<'_>,
+    span: Span,
 ) {
     if in_constant(cx, expr.hir_id) {
         return;
     }
 
-    let span = expr.span;
     let combine_and = match op {
         BinOpKind::And | BinOpKind::BitAnd => true,
         BinOpKind::Or | BinOpKind::BitOr => false,
@@ -294,6 +294,20 @@ fn check_possible_range_contains(
             );
         }
     }
+
+    // If the LHS is the same operator, we have to recurse to get the "real" RHS, since they have
+    // the same operator precedence
+    if_chain! {
+        if let ExprKind::Binary(ref lhs_op, _left, new_lhs) = left.kind;
+        if op == lhs_op.node;
+        let new_span = Span::new(new_lhs.span.lo(), right.span.hi(), expr.span.ctxt(), expr.span.parent());
+        if let Some(snip) = &snippet_opt(cx, new_span);
+        // Do not continue if we have mismatched number of parens, otherwise the suggestion is wrong
+        if snip.matches('(').count() == snip.matches(')').count();
+        then {
+            check_possible_range_contains(cx, op, new_lhs, right, expr, new_span);
+        }
+    }
 }
 
 struct RangeBounds<'a> {

tests/ui/range_contains.fixed

@@ -49,6 +49,13 @@ fn main() {
     x >= 10 && x <= -10;
     (-3. ..=3.).contains(&y);
     y >= 3. && y <= -3.;
+
+    // Fix #8745
+    let z = 42;
+    (0..=10).contains(&x) && (0..=10).contains(&z);
+    !(0..10).contains(&x) || !(0..10).contains(&z);
+    // Make sure operators in parens don't give a breaking suggestion
+    ((x % 2 == 0) || (x < 0)) || (x >= 10);
 }
 
 // Fix #6373

tests/ui/range_contains.rs

@@ -49,6 +49,13 @@ fn main() {
     x >= 10 && x <= -10;
     y >= -3. && y <= 3.;
     y >= 3. && y <= -3.;
+
+    // Fix #8745
+    let z = 42;
+    (x >= 0) && (x <= 10) && (z >= 0) && (z <= 10);
+    (x < 0) || (x >= 10) || (z < 0) || (z >= 10);
+    // Make sure operators in parens don't give a breaking suggestion
+    ((x % 2 == 0) || (x < 0)) || (x >= 10);
 }
 
 // Fix #6373

tests/ui/range_contains.stderr

@@ -96,5 +96,29 @@ error: manual `RangeInclusive::contains` implementation
 LL |     y >= -3. && y <= 3.;
    |     ^^^^^^^^^^^^^^^^^^^ help: use: `(-3. ..=3.).contains(&y)`
 
-error: aborting due to 16 previous errors
+error: manual `RangeInclusive::contains` implementation
+  --> $DIR/range_contains.rs:55:30
+   |
+LL |     (x >= 0) && (x <= 10) && (z >= 0) && (z <= 10);
+   |                              ^^^^^^^^^^^^^^^^^^^^^ help: use: `(0..=10).contains(&z)`
+
+error: manual `RangeInclusive::contains` implementation
+  --> $DIR/range_contains.rs:55:5
+   |
+LL |     (x >= 0) && (x <= 10) && (z >= 0) && (z <= 10);
+   |     ^^^^^^^^^^^^^^^^^^^^^ help: use: `(0..=10).contains(&x)`
+
+error: manual `!Range::contains` implementation
+  --> $DIR/range_contains.rs:56:29
+   |
+LL |     (x < 0) || (x >= 10) || (z < 0) || (z >= 10);
+   |                             ^^^^^^^^^^^^^^^^^^^^ help: use: `!(0..10).contains(&z)`
+
+error: manual `!Range::contains` implementation
+  --> $DIR/range_contains.rs:56:5
+   |
+LL |     (x < 0) || (x >= 10) || (z < 0) || (z >= 10);
+   |     ^^^^^^^^^^^^^^^^^^^^ help: use: `!(0..10).contains(&x)`
+
+error: aborting due to 20 previous errors