identity_op: add parenthesis to suggestions where required

Author: Alexendoo

clippy_lints/src/identity_op.rs

@@ -1,15 +1,14 @@
-use clippy_utils::get_parent_expr;
-use clippy_utils::source::snippet;
-use rustc_hir::{BinOp, BinOpKind, Expr, ExprKind};
+use clippy_utils::consts::{constant_full_int, constant_simple, Constant, FullInt};
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::source::snippet_with_applicability;
+use clippy_utils::{clip, unsext};
+use rustc_errors::Applicability;
+use rustc_hir::{BinOp, BinOpKind, Expr, ExprKind, Node};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::ty;
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 use rustc_span::source_map::Span;
 
-use clippy_utils::consts::{constant_full_int, constant_simple, Constant, FullInt};
-use clippy_utils::diagnostics::span_lint;
-use clippy_utils::{clip, unsext};
-
 declare_clippy_lint! {
     /// ### What it does
     /// Checks for identity operations, e.g., `x + 0`.
@@ -23,11 +22,6 @@ 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,
@@ -45,56 +39,73 @@ impl<'tcx> LateLintPass<'tcx> for IdentityOp {
             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);
+                        check(cx, left, 0, expr.span, right.span, needs_parenthesis(cx, expr, right));
+                        check(cx, right, 0, expr.span, left.span, Parens::Unneeded);
                     },
                     BinOpKind::Shl | BinOpKind::Shr | BinOpKind::Sub => {
-                        check(cx, right, 0, expr.span, left.span);
+                        check(cx, right, 0, expr.span, left.span, Parens::Unneeded);
                     },
                     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);
+                        check(cx, left, 1, expr.span, right.span, needs_parenthesis(cx, expr, right));
+                        check(cx, right, 1, expr.span, left.span, Parens::Unneeded);
                     },
-                    BinOpKind::Div => check(cx, right, 1, expr.span, left.span),
+                    BinOpKind::Div => check(cx, right, 1, expr.span, left.span, Parens::Unneeded),
                     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);
+                        check(cx, left, -1, expr.span, right.span, needs_parenthesis(cx, expr, right));
+                        check(cx, right, -1, expr.span, left.span, Parens::Unneeded);
                     },
+                    BinOpKind::Rem => 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 }`
+#[derive(Copy, Clone)]
+enum Parens {
+    Needed,
+    Unneeded,
+}
+
+/// Checks if `left op right` needs parenthesis when 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 }` where the `if` could be
+/// interpreted as a statement
+///
 /// 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 needs_parenthesis(cx: &LateContext<'_>, binary: &Expr<'_>, right: &Expr<'_>) -> Parens {
+    match right.kind {
+        ExprKind::Binary(_, lhs, _) | ExprKind::Cast(lhs, _) => {
+            // ensure we're checking against the leftmost expression of `right`
+            //
+            //     ~~~ `lhs`
+            // 0 + {4} * 2
+            //     ~~~~~~~ `right`
+            return needs_parenthesis(cx, binary, lhs);
+        },
+        ExprKind::If(..) | ExprKind::Match(..) | ExprKind::Block(..) | ExprKind::Loop(..) => {},
+        _ => return Parens::Unneeded,
     }
-}
 
-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
+    let mut prev_id = binary.hir_id;
+    for (_, node) in cx.tcx.hir().parent_iter(binary.hir_id) {
+        if let Node::Expr(expr) = node
+            && let ExprKind::Binary(_, lhs, _) | ExprKind::Cast(lhs, _) = expr.kind
+            && lhs.hir_id == prev_id
+        {
+            // keep going until we find a node that encompasses left of `binary`
+            prev_id = expr.hir_id;
+            continue;
+        }
+
+        match node {
+            Node::Block(_) | Node::Stmt(_) => break,
+            _ => return Parens::Unneeded,
+        };
     }
+
+    Parens::Needed
 }
 
 fn is_allowed(cx: &LateContext<'_>, cmp: BinOp, left: &Expr<'_>, right: &Expr<'_>) -> bool {
@@ -115,11 +126,11 @@ fn check_remainder(cx: &LateContext<'_>, left: &Expr<'_>, right: &Expr<'_>, span
         (Some(FullInt::U(lv)), Some(FullInt::U(rv))) => lv < rv,
         _ => return,
     } {
-        span_ineffective_operation(cx, span, arg);
+        span_ineffective_operation(cx, span, arg, Parens::Unneeded);
     }
 }
 
-fn check(cx: &LateContext<'_>, e: &Expr<'_>, m: i8, span: Span, arg: Span) {
+fn check(cx: &LateContext<'_>, e: &Expr<'_>, m: i8, span: Span, arg: Span, parens: Parens) {
     if let Some(Constant::Int(v)) = constant_simple(cx, cx.typeck_results(), e).map(Constant::peel_refs) {
         let check = match *cx.typeck_results().expr_ty(e).peel_refs().kind() {
             ty::Int(ity) => unsext(cx.tcx, -1_i128, ity),
@@ -132,19 +143,27 @@ fn check(cx: &LateContext<'_>, e: &Expr<'_>, m: i8, span: Span, arg: Span) {
             1 => v == 1,
             _ => unreachable!(),
         } {
-            span_ineffective_operation(cx, span, arg);
+            span_ineffective_operation(cx, span, arg, parens);
         }
     }
 }
 
-fn span_ineffective_operation(cx: &LateContext<'_>, span: Span, arg: Span) {
-    span_lint(
+fn span_ineffective_operation(cx: &LateContext<'_>, span: Span, arg: Span, parens: Parens) {
+    let mut applicability = Applicability::MachineApplicable;
+    let expr_snippet = snippet_with_applicability(cx, arg, "..", &mut applicability);
+
+    let suggestion = match parens {
+        Parens::Needed => format!("({expr_snippet})"),
+        Parens::Unneeded => expr_snippet.into_owned(),
+    };
+
+    span_lint_and_sugg(
         cx,
         IDENTITY_OP,
         span,
-        &format!(
-            "the operation is ineffective. Consider reducing it to `{}`",
-            snippet(cx, arg, "..")
-        ),
+        "this operation has no effect",
+        "consider reducing it to",
+        suggestion,
+        applicability,
     );
 }

tests/ui/identity_op.fixed

@@ -0,0 +1,119 @@
+// run-rustfix
+
+#![warn(clippy::identity_op)]
+#![allow(
+    clippy::eq_op,
+    clippy::no_effect,
+    clippy::unnecessary_operation,
+    clippy::op_ref,
+    clippy::double_parens,
+    unused
+)]
+
+use std::fmt::Write as _;
+
+const ONE: i64 = 1;
+const NEG_ONE: i64 = -1;
+const ZERO: i64 = 0;
+
+struct A(String);
+
+impl std::ops::Shl<i32> for A {
+    type Output = A;
+    fn shl(mut self, other: i32) -> Self {
+        let _ = write!(self.0, "{}", other);
+        self
+    }
+}
+
+struct Length(u8);
+struct Meter;
+
+impl core::ops::Mul<Meter> for u8 {
+    type Output = Length;
+    fn mul(self, _: Meter) -> Length {
+        Length(self)
+    }
+}
+
+#[rustfmt::skip]
+fn main() {
+    let x = 0;
+
+    x;
+    x;
+    x + 1;
+    x;
+    1 + x;
+    x - ZERO; //no error, as we skip lookups (for now)
+    x;
+    ((ZERO)) | x; //no error, as we skip lookups (for now)
+
+    x;
+    x;
+    x / ONE; //no error, as we skip lookups (for now)
+
+    x / 2; //no false positive
+
+    x & NEG_ONE; //no error, as we skip lookups (for now)
+    x;
+
+    let u: u8 = 0;
+    u;
+
+    1 << 0; // no error, this case is allowed, see issue 3430
+    42;
+    1;
+    42;
+    &x;
+    x;
+
+    let mut a = A("".into());
+    let b = a << 0; // no error: non-integer
+
+    1 * Meter; // no error: non-integer
+
+    2;
+    -2;
+    2 + x;
+    -2 + x;
+    x + 1;
+    (x + 1) % 3; // no error
+    4 % 3; // no error
+    4 % -3; // no error
+
+    // See #8724
+    let a = 0;
+    let b = true;
+    (if b { 1 } else { 2 });
+    (if b { 1 } else { 2 }) + if b { 3 } else { 4 };
+    (match a { 0 => 10, _ => 20 });
+    (match a { 0 => 10, _ => 20 }) + match a { 0 => 30, _ => 40 };
+    (if b { 1 } else { 2 }) + match a { 0 => 30, _ => 40 };
+    (match a { 0 => 10, _ => 20 }) + if b { 3 } else { 4 };
+    (if b { 1 } else { 2 });
+
+    ({ a }) + 3;
+    ({ a } * 2);
+    (loop { let mut c = 0; if c == 10 { break c; } c += 1; }) + { a * 2 };
+
+    fn f(_: i32) {
+        todo!();
+    }
+    f(a + { 8 * 5 });
+    f(if b { 1 } else { 2 } + 3);
+    const _: i32 = { 2 * 4 } + 3;
+    const _: i32 = { 1 + 2 * 3 } + 3;
+
+    a as usize;
+    let _ = a as usize;
+    ({ a } as usize);
+
+    2 * { a };
+    (({ a } + 4));
+    1;
+}
+
+pub fn decide(a: bool, b: bool) -> u32 {
+    (if a { 1 } else { 2 }) + if b { 3 } else { 5 }
+}

tests/ui/identity_op.rs

@@ -1,3 +1,15 @@
+// run-rustfix
+
+#![warn(clippy::identity_op)]
+#![allow(
+    clippy::eq_op,
+    clippy::no_effect,
+    clippy::unnecessary_operation,
+    clippy::op_ref,
+    clippy::double_parens,
+    unused
+)]
+
 use std::fmt::Write as _;
 
 const ONE: i64 = 1;
@@ -24,14 +36,6 @@ impl core::ops::Mul<Meter> for u8 {
     }
 }
 
-#[allow(
-    clippy::eq_op,
-    clippy::no_effect,
-    clippy::unnecessary_operation,
-    clippy::op_ref,
-    clippy::double_parens
-)]
-#[warn(clippy::identity_op)]
 #[rustfmt::skip]
 fn main() {
     let x = 0;
@@ -82,29 +86,34 @@ fn main() {
     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 + if b { 1 } else { 2 } + if b { 3 } else { 4 };
     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
-    
+    0 + match a { 0 => 10, _ => 20 } + match a { 0 => 30, _ => 40 };
+    0 + if b { 1 } else { 2 } + match a { 0 => 30, _ => 40 };
+    0 + match a { 0 => 10, _ => 20 } + if b { 3 } else { 4 };
+    (if b { 1 } else { 2 }) + 0;
+
+    0 + { a } + 3;
+    0 + { a } * 2;
+    0 + loop { let mut c = 0; if c == 10 { break c; } c += 1; } + { a * 2 };
+
     fn f(_: i32) {
         todo!();
     }
     f(1 * a + { 8 * 5 });
-    f(0 + if b { 1 } else { 2 } + 3); // no error
+    f(0 + if b { 1 } else { 2 } + 3);
     const _: i32 = { 2 * 4 } + 0 + 3;
-    const _: i32 = 0 + { 1 + 2 * 3 } + 3; // no error
+    const _: i32 = 0 + { 1 + 2 * 3 } + 3;
+
+    0 + a as usize;
+    let _ = 0 + a as usize;
+    0 + { a } as usize;
+
+    2 * (0 + { a });
+    1 * ({ a } + 4);
+    1 * 1;
+}
+
+pub fn decide(a: bool, b: bool) -> u32 {
+    0 + if a { 1 } else { 2 } + if b { 3 } else { 5 }
 }