Auto merge of #8730 - tamaroning:fix8724, r=Alexendoo

bors · bors · commit 0ceacbe18546 · 2022-05-03T07:05:51.000Z
[FP] identity_op in front of if fix #8724 changelog: FP: [`identity_op`]: is now allowed in front of if statements, blocks and other expressions where the suggestion would be invalid. Resolved simular problems with blocks, mathces, and loops. identity_op always does NOT suggest reducing `0 + if b { 1 } else { 2 } + 3` into `if b { 1 } else { 2 } + 3` even in the case that the expression is in `f(expr)` or `let x = expr;` for now.
diff --git a/clippy_lints/src/identity_op.rs b/clippy_lints/src/identity_op.rs
@@ -1,3 +1,4 @@
+use clippy_utils::get_parent_expr;
 use clippy_utils::source::snippet;
 use rustc_hir::{BinOp, BinOpKind, Expr, ExprKind};
 use rustc_lint::{LateContext, LateLintPass};
@@ -22,6 +23,11 @@ declare_clippy_lint! {
     /// # let x = 1;
     /// x / 1 + 0 * 1 - 0 | 0;
     /// ```
+    ///
+    /// ### Known problems
+    /// False negatives: `f(0 + if b { 1 } else { 2 } + 3);` is reducible to
+    /// `f(if b { 1 } else { 2 } + 3);`. But the lint doesn't trigger for the code.
+    /// See [#8724](https://github.com/rust-lang/rust-clippy/issues/8724)
     #[clippy::version = "pre 1.29.0"]
     pub IDENTITY_OP,
     complexity,
@@ -31,36 +37,66 @@ declare_clippy_lint! {
 declare_lint_pass!(IdentityOp => [IDENTITY_OP]);
 
 impl<'tcx> LateLintPass<'tcx> for IdentityOp {
-    fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) {
-        if e.span.from_expansion() {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
+        if expr.span.from_expansion() {
             return;
         }
-        if let ExprKind::Binary(cmp, left, right) = e.kind {
-            if is_allowed(cx, cmp, left, right) {
-                return;
-            }
-            match cmp.node {
-                BinOpKind::Add | BinOpKind::BitOr | BinOpKind::BitXor => {
-                    check(cx, left, 0, e.span, right.span);
-                    check(cx, right, 0, e.span, left.span);
-                },
-                BinOpKind::Shl | BinOpKind::Shr | BinOpKind::Sub => check(cx, right, 0, e.span, left.span),
-                BinOpKind::Mul => {
-                    check(cx, left, 1, e.span, right.span);
-                    check(cx, right, 1, e.span, left.span);
-                },
-                BinOpKind::Div => check(cx, right, 1, e.span, left.span),
-                BinOpKind::BitAnd => {
-                    check(cx, left, -1, e.span, right.span);
-                    check(cx, right, -1, e.span, left.span);
-                },
-                BinOpKind::Rem => check_remainder(cx, left, right, e.span, left.span),
-                _ => (),
+        if let ExprKind::Binary(cmp, left, right) = &expr.kind {
+            if !is_allowed(cx, *cmp, left, right) {
+                match cmp.node {
+                    BinOpKind::Add | BinOpKind::BitOr | BinOpKind::BitXor => {
+                        if reducible_to_right(cx, expr, right) {
+                            check(cx, left, 0, expr.span, right.span);
+                        }
+                        check(cx, right, 0, expr.span, left.span);
+                    },
+                    BinOpKind::Shl | BinOpKind::Shr | BinOpKind::Sub => {
+                        check(cx, right, 0, expr.span, left.span);
+                    },
+                    BinOpKind::Mul => {
+                        if reducible_to_right(cx, expr, right) {
+                            check(cx, left, 1, expr.span, right.span);
+                        }
+                        check(cx, right, 1, expr.span, left.span);
+                    },
+                    BinOpKind::Div => check(cx, right, 1, expr.span, left.span),
+                    BinOpKind::BitAnd => {
+                        if reducible_to_right(cx, expr, right) {
+                            check(cx, left, -1, expr.span, right.span);
+                        }
+                        check(cx, right, -1, expr.span, left.span);
+                    },
+                    BinOpKind::Rem => {
+                        // Don't call reducible_to_right because N % N is always reducible to 1
+                        check_remainder(cx, left, right, expr.span, left.span);
+                    },
+                    _ => (),
+                }
             }
         }
     }
 }
 
+/// Checks if `left op ..right` can be actually reduced to `right`
+/// e.g. `0 + if b { 1 } else { 2 } + if b { 3 } else { 4 }`
+/// cannot be reduced to `if b { 1 } else { 2 } +  if b { 3 } else { 4 }`
+/// See #8724
+fn reducible_to_right(cx: &LateContext<'_>, binary: &Expr<'_>, right: &Expr<'_>) -> bool {
+    if let ExprKind::If(..) | ExprKind::Match(..) | ExprKind::Block(..) | ExprKind::Loop(..) = right.kind {
+        is_toplevel_binary(cx, binary)
+    } else {
+        true
+    }
+}
+
+fn is_toplevel_binary(cx: &LateContext<'_>, must_be_binary: &Expr<'_>) -> bool {
+    if let Some(parent) = get_parent_expr(cx, must_be_binary) && let ExprKind::Binary(..) = &parent.kind {
+        false
+    } else {
+        true
+    }
+}
+
 fn is_allowed(cx: &LateContext<'_>, cmp: BinOp, left: &Expr<'_>, right: &Expr<'_>) -> bool {
     // This lint applies to integers
     !cx.typeck_results().expr_ty(left).peel_refs().is_integral()
diff --git a/tests/ui/identity_op.rs b/tests/ui/identity_op.rs
@@ -77,4 +77,34 @@ fn main() {
     (x + 1) % 3; // no error
     4 % 3; // no error
     4 % -3; // no error
+
+    // See #8724
+    let a = 0;
+    let b = true;
+    0 + if b { 1 } else { 2 };
+    0 + if b { 1 } else { 2 } + if b { 3 } else { 4 }; // no error
+    0 + match a { 0 => 10, _ => 20 };
+    0 + match a { 0 => 10, _ => 20 } + match a { 0 => 30, _ => 40 }; // no error
+    0 + if b { 1 } else { 2 } + match a { 0 => 30, _ => 40 }; // no error
+    0 + match a { 0 => 10, _ => 20 } + if b { 3 } else { 4 }; // no error
+    
+    0 + if b { 0 + 1 } else { 2 };
+    0 + match a { 0 =>  0 + 10, _ => 20 };
+    0 + if b { 0 + 1 } else { 2 } + match a { 0 => 0 + 30, _ => 40 };
+
+    let _ = 0 + if 0 + 1 > 0 { 1 } else { 2 } + if 0 + 1 > 0 { 3 } else { 4 };
+    let _ = 0 + match 0 + 1 { 0 => 10, _ => 20 } + match 0 + 1  { 0 => 30, _ => 40 };
+
+    0 + if b { 1 } else { 2 } + if b { 3 } else { 4 } + 0;
+    
+    0 + { a } + 3; // no error
+    0 + loop { let mut c = 0; if c == 10 { break c; } c += 1; } + { a * 2 }; // no error
+    
+    fn f(_: i32) {
+        todo!();
+    }
+    f(1 * a + { 8 * 5 });
+    f(0 + if b { 1 } else { 2 } + 3); // no error
+    const _: i32 = { 2 * 4 } + 0 + 3;
+    const _: i32 = 0 + { 1 + 2 * 3 } + 3; // no error
 }
diff --git a/tests/ui/identity_op.stderr b/tests/ui/identity_op.stderr
@@ -108,5 +108,95 @@ error: the operation is ineffective. Consider reducing it to `1`
 LL |     x + 1 % 3;
    |         ^^^^^
 
-error: aborting due to 18 previous errors
+error: the operation is ineffective. Consider reducing it to `if b { 1 } else { 2 }`
+  --> $DIR/identity_op.rs:84:5
+   |
+LL |     0 + if b { 1 } else { 2 };
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: the operation is ineffective. Consider reducing it to `match a { 0 => 10, _ => 20 }`
+  --> $DIR/identity_op.rs:86:5
+   |
+LL |     0 + match a { 0 => 10, _ => 20 };
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: the operation is ineffective. Consider reducing it to `if b { 0 + 1 } else { 2 }`
+  --> $DIR/identity_op.rs:91:5
+   |
+LL |     0 + if b { 0 + 1 } else { 2 };
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: the operation is ineffective. Consider reducing it to `1`
+  --> $DIR/identity_op.rs:91:16
+   |
+LL |     0 + if b { 0 + 1 } else { 2 };
+   |                ^^^^^
+
+error: the operation is ineffective. Consider reducing it to `match a { 0 =>  0 + 10, _ => 20 }`
+  --> $DIR/identity_op.rs:92:5
+   |
+LL |     0 + match a { 0 =>  0 + 10, _ => 20 };
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: the operation is ineffective. Consider reducing it to `10`
+  --> $DIR/identity_op.rs:92:25
+   |
+LL |     0 + match a { 0 =>  0 + 10, _ => 20 };
+   |                         ^^^^^^
+
+error: the operation is ineffective. Consider reducing it to `1`
+  --> $DIR/identity_op.rs:93:16
+   |
+LL |     0 + if b { 0 + 1 } else { 2 } + match a { 0 => 0 + 30, _ => 40 };
+   |                ^^^^^
+
+error: the operation is ineffective. Consider reducing it to `30`
+  --> $DIR/identity_op.rs:93:52
+   |
+LL |     0 + if b { 0 + 1 } else { 2 } + match a { 0 => 0 + 30, _ => 40 };
+   |                                                    ^^^^^^
+
+error: the operation is ineffective. Consider reducing it to `1`
+  --> $DIR/identity_op.rs:95:20
+   |
+LL |     let _ = 0 + if 0 + 1 > 0 { 1 } else { 2 } + if 0 + 1 > 0 { 3 } else { 4 };
+   |                    ^^^^^
+
+error: the operation is ineffective. Consider reducing it to `1`
+  --> $DIR/identity_op.rs:95:52
+   |
+LL |     let _ = 0 + if 0 + 1 > 0 { 1 } else { 2 } + if 0 + 1 > 0 { 3 } else { 4 };
+   |                                                    ^^^^^
+
+error: the operation is ineffective. Consider reducing it to `1`
+  --> $DIR/identity_op.rs:96:23
+   |
+LL |     let _ = 0 + match 0 + 1 { 0 => 10, _ => 20 } + match 0 + 1  { 0 => 30, _ => 40 };
+   |                       ^^^^^
+
+error: the operation is ineffective. Consider reducing it to `1`
+  --> $DIR/identity_op.rs:96:58
+   |
+LL |     let _ = 0 + match 0 + 1 { 0 => 10, _ => 20 } + match 0 + 1  { 0 => 30, _ => 40 };
+   |                                                          ^^^^^
+
+error: the operation is ineffective. Consider reducing it to `0 + if b { 1 } else { 2 } + if b { 3 } else { 4 }`
+  --> $DIR/identity_op.rs:98:5
+   |
+LL |     0 + if b { 1 } else { 2 } + if b { 3 } else { 4 } + 0;
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: the operation is ineffective. Consider reducing it to `a`
+  --> $DIR/identity_op.rs:106:7
+   |
+LL |     f(1 * a + { 8 * 5 });
+   |       ^^^^^
+
+error: the operation is ineffective. Consider reducing it to `{ 2 * 4 }`
+  --> $DIR/identity_op.rs:108:20
+   |
+LL |     const _: i32 = { 2 * 4 } + 0 + 3;
+   |                    ^^^^^^^^^^^^^
+
+error: aborting due to 33 previous errors
 
