From a0b7681a6f5e0522a4e8255961523370256bfd7d Mon Sep 17 00:00:00 2001 From: TheSlapstickDictator <6685366+TheSlapstickDictator@users.noreply.github.com> Date: Sat, 2 Nov 2024 19:47:11 -0700 Subject: [PATCH 1/4] fix: `identity_op` suggestions use correct parenthesis The `identity_op` lint was suggesting code fixes that resulted in incorrect or broken code, due to missing parenthesis in the fix that changed the semantics of the code. For a binary expression, `left op right`, if the `left` was redundant, it would check if the right side needed parenthesis, but if the `right` was redundant, it would just assume that the left side did not need parenthesis. This can result in either rustfix generating broken code and failing, or code that has different behavior than before the fix. e.g. `-(x + y + 0)` would turn into `-x + y`, changing the behavior, and `1u64 + (x + y + 0i32) as u64` where `x: i32` and `y: i32` would turn into `1u64 + x + y as u64`, creating broken code where `x` cannot be added to the other values, as it was never cast to `u64`. This commit fixes both of these cases by always checking the non-redundant child of a binary expression for needed parenthesis, and makes it so if we need parenthesis, but they already exist, we don't add any redundant ones. Fixes #13470 --- clippy_lints/src/operators/identity_op.rs | 211 +++++++++++----------- tests/ui/identity_op.fixed | 42 ++++- tests/ui/identity_op.rs | 40 ++++ tests/ui/identity_op.stderr | 64 ++++++- 4 files changed, 252 insertions(+), 105 deletions(-) diff --git a/clippy_lints/src/operators/identity_op.rs b/clippy_lints/src/operators/identity_op.rs index 830be50c8ba4..d8430f6492d2 100644 --- a/clippy_lints/src/operators/identity_op.rs +++ b/clippy_lints/src/operators/identity_op.rs @@ -42,83 +42,53 @@ pub(crate) fn check<'tcx>( match op { BinOpKind::Add | BinOpKind::BitOr | BinOpKind::BitXor => { - let _ = check_op( - cx, - left, - 0, - expr.span, - peeled_right_span, - needs_parenthesis(cx, expr, right), - right_is_coerced_to_value, - ) || check_op( - cx, - right, - 0, - expr.span, - peeled_left_span, - Parens::Unneeded, - left_is_coerced_to_value, - ); + if is_redundant_op(cx, left, 0) { + let paren = needs_parenthesis(cx, expr, right); + span_ineffective_operation(cx, expr.span, peeled_right_span, paren, right_is_coerced_to_value); + } else if is_redundant_op(cx, right, 0) { + let paren = needs_parenthesis(cx, expr, left); + span_ineffective_operation(cx, expr.span, peeled_left_span, paren, left_is_coerced_to_value); + } }, BinOpKind::Shl | BinOpKind::Shr | BinOpKind::Sub => { - let _ = check_op( - cx, - right, - 0, - expr.span, - peeled_left_span, - Parens::Unneeded, - left_is_coerced_to_value, - ); + if is_redundant_op(cx, right, 0) { + span_ineffective_operation( + cx, + expr.span, + peeled_left_span, + Parens::Unneeded, + left_is_coerced_to_value, + ); + } }, BinOpKind::Mul => { - let _ = check_op( - cx, - left, - 1, - expr.span, - peeled_right_span, - needs_parenthesis(cx, expr, right), - right_is_coerced_to_value, - ) || check_op( - cx, - right, - 1, - expr.span, - peeled_left_span, - Parens::Unneeded, - left_is_coerced_to_value, - ); + if is_redundant_op(cx, left, 1) { + let paren = needs_parenthesis(cx, expr, right); + span_ineffective_operation(cx, expr.span, peeled_right_span, paren, right_is_coerced_to_value); + } else if is_redundant_op(cx, right, 1) { + let paren = needs_parenthesis(cx, expr, left); + span_ineffective_operation(cx, expr.span, peeled_left_span, paren, left_is_coerced_to_value); + } }, BinOpKind::Div => { - let _ = check_op( - cx, - right, - 1, - expr.span, - peeled_left_span, - Parens::Unneeded, - left_is_coerced_to_value, - ); + if is_redundant_op(cx, right, 1) { + span_ineffective_operation( + cx, + expr.span, + peeled_left_span, + Parens::Unneeded, + left_is_coerced_to_value, + ); + } }, BinOpKind::BitAnd => { - let _ = check_op( - cx, - left, - -1, - expr.span, - peeled_right_span, - needs_parenthesis(cx, expr, right), - right_is_coerced_to_value, - ) || check_op( - cx, - right, - -1, - expr.span, - peeled_left_span, - Parens::Unneeded, - left_is_coerced_to_value, - ); + if is_redundant_op(cx, left, -1) { + let paren = needs_parenthesis(cx, expr, right); + span_ineffective_operation(cx, expr.span, peeled_right_span, paren, right_is_coerced_to_value); + } else if is_redundant_op(cx, right, -1) { + let paren = needs_parenthesis(cx, expr, left); + span_ineffective_operation(cx, expr.span, peeled_left_span, paren, left_is_coerced_to_value); + } }, BinOpKind::Rem => check_remainder(cx, left, right, expr.span, left.span), _ => (), @@ -138,43 +108,75 @@ enum Parens { Unneeded, } -/// Checks if `left op right` needs parenthesis when reduced to `right` +/// Checks if a binary expression needs parenthesis when reduced to just its +/// right or left child. +/// +/// e.g. `-(x + y + 0)` cannot be reduced to `-x + y`, as the behavior changes silently. +/// e.g. `1u64 + ((x + y + 0i32) as u64)` cannot be reduced to `1u64 + x + y as u64`, since +/// the the cast expression will not apply to the same expression. /// 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 +/// interpreted as a statement. The same behavior happens for `match`, `loop`, +/// and blocks. +/// e.g. `2 * (0 + { a })` can be reduced to `2 * { a }` without the need for parenthesis, +/// but `1 * ({ a } + 4)` cannot be reduced to `{ a } + 4`, as a block at the start of a line +/// will be interpreted as a statement instead of an expression. /// -/// See #8724 -fn needs_parenthesis(cx: &LateContext<'_>, binary: &Expr<'_>, right: &Expr<'_>) -> Parens { - match right.kind { +/// See #8724, #13470 +fn needs_parenthesis(cx: &LateContext<'_>, binary: &Expr<'_>, child: &Expr<'_>) -> Parens { + match child.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); + // For casts and binary expressions, we want to add parenthesis if + // the parent HIR node is an expression with a different precedence, + // or if the parent HIR node is a Block or Stmt, and the new left hand side + // would be treated as a statement rather than an expression. + if let Some((_, parent)) = cx.tcx.hir().parent_iter(binary.hir_id).next() { + if let Node::Expr(expr) = parent { + if child.precedence().order() != expr.precedence().order() { + return Parens::Needed; + } + return Parens::Unneeded; + } + match parent { + Node::Block(_) | Node::Stmt(_) => { + // ensure we're checking against the leftmost expression of `child` + // + // ~~~~~~~~~~~ `binary` + // ~~~ `lhs` + // 0 + {4} * 2 + // ~~~~~~~ `child` + return needs_parenthesis(cx, binary, lhs); + }, + _ => return Parens::Unneeded, + } + } }, - ExprKind::If(..) | ExprKind::Match(..) | ExprKind::Block(..) | ExprKind::Loop(..) => {}, - _ => return Parens::Unneeded, - } + ExprKind::If(..) | ExprKind::Match(..) | ExprKind::Block(..) | ExprKind::Loop(..) => { + // For if, match, block, and loop expressions, we want to add parenthesis if + // the closest ancestor node that is not an expression is a block or statement. + // This would mean that the rustfix suggestion will appear at the start of a line, which causes + // these expressions to be interpreted as statements if they do not have parenthesis. + let mut prev_id = binary.hir_id; + for (_, parent) in cx.tcx.hir().parent_iter(binary.hir_id) { + if let Node::Expr(expr) = parent + && let ExprKind::Binary(_, lhs, _) | ExprKind::Cast(lhs, _) | ExprKind::Unary(_, 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; + } - 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, - }; + match parent { + Node::Block(_) | Node::Stmt(_) => return Parens::Needed, + _ => return Parens::Unneeded, + }; + } + }, + _ => { + return Parens::Unneeded; + }, } - Parens::Needed } @@ -199,7 +201,7 @@ fn check_remainder(cx: &LateContext<'_>, left: &Expr<'_>, right: &Expr<'_>, span } } -fn check_op(cx: &LateContext<'_>, e: &Expr<'_>, m: i8, span: Span, arg: Span, parens: Parens, is_erased: bool) -> bool { +fn is_redundant_op(cx: &LateContext<'_>, e: &Expr<'_>, m: i8) -> bool { if let Some(Constant::Int(v)) = ConstEvalCtxt::new(cx).eval_simple(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), @@ -212,7 +214,6 @@ fn check_op(cx: &LateContext<'_>, e: &Expr<'_>, m: i8, span: Span, arg: Span, pa 1 => v == 1, _ => unreachable!(), } { - span_ineffective_operation(cx, span, arg, parens, is_erased); return true; } } @@ -234,7 +235,13 @@ fn span_ineffective_operation( expr_snippet.into_owned() }; let suggestion = match parens { - Parens::Needed => format!("({expr_snippet})"), + Parens::Needed => { + if !expr_snippet.starts_with('(') && !expr_snippet.ends_with(')') { + format!("({expr_snippet})") + } else { + expr_snippet + } + }, Parens::Unneeded => expr_snippet, }; diff --git a/tests/ui/identity_op.fixed b/tests/ui/identity_op.fixed index b18d8560f6f8..18b7401768df 100644 --- a/tests/ui/identity_op.fixed +++ b/tests/ui/identity_op.fixed @@ -149,7 +149,7 @@ fn main() { 2 * { a }; //~^ ERROR: this operation has no effect - (({ a } + 4)); + ({ a } + 4); //~^ ERROR: this operation has no effect 1; //~^ ERROR: this operation has no effect @@ -212,3 +212,43 @@ fn issue_12050() { //~^ ERROR: this operation has no effect } } + +fn issue_13470() { + let x = 1i32; + let y = 1i32; + // Removes the + 0i32 while keeping the parentheses around x + y so the cast operation works + let _: u64 = (x + y) as u64; + //~^ ERROR: this operation has no effect + // both of the next two lines should look the same after rustfix + let _: u64 = 1u64 & (x + y) as u64; + //~^ ERROR: this operation has no effect + // Same as above, but with extra redundant parenthesis + let _: u64 = 1u64 & (x + y) as u64; + //~^ ERROR: this operation has no effect + // Should maintain parenthesis even if the surrounding expr has the same precedence + let _: u64 = 5u64 + (x + y) as u64; + //~^ ERROR: this operation has no effect + + // If we don't maintain the parens here, the behavior changes + let _ = -(x + y); + //~^ ERROR: this operation has no effect + // Maintain parenthesis if the parent expr is of higher precedence + let _ = 2i32 * (x + y); + //~^ ERROR: this operation has no effect + // No need for parenthesis if the parent expr is of equal precedence + let _ = 2i32 + x + y; + //~^ ERROR: this operation has no effect + // But make sure that inner parens still exist + let z = 1i32; + let _ = 2 + x + (y * z); + //~^ ERROR: this operation has no effect + // Maintain parenthesis if the parent expr is of lower precedence + // This is for clarity, and clippy will not warn on these being unnecessary + let _ = 2i32 + (x * y); + //~^ ERROR: this operation has no effect + + let x = 1i16; + let y = 1i16; + let _: u64 = 1u64 + (x as i32 + y as i32) as u64; + //~^ ERROR: this operation has no effect +} diff --git a/tests/ui/identity_op.rs b/tests/ui/identity_op.rs index f1f01b424478..55483f4e5fb3 100644 --- a/tests/ui/identity_op.rs +++ b/tests/ui/identity_op.rs @@ -212,3 +212,43 @@ fn issue_12050() { //~^ ERROR: this operation has no effect } } + +fn issue_13470() { + let x = 1i32; + let y = 1i32; + // Removes the + 0i32 while keeping the parentheses around x + y so the cast operation works + let _: u64 = (x + y + 0i32) as u64; + //~^ ERROR: this operation has no effect + // both of the next two lines should look the same after rustfix + let _: u64 = 1u64 & (x + y + 0i32) as u64; + //~^ ERROR: this operation has no effect + // Same as above, but with extra redundant parenthesis + let _: u64 = 1u64 & ((x + y) + 0i32) as u64; + //~^ ERROR: this operation has no effect + // Should maintain parenthesis even if the surrounding expr has the same precedence + let _: u64 = 5u64 + ((x + y) + 0i32) as u64; + //~^ ERROR: this operation has no effect + + // If we don't maintain the parens here, the behavior changes + let _ = -(x + y + 0i32); + //~^ ERROR: this operation has no effect + // Maintain parenthesis if the parent expr is of higher precedence + let _ = 2i32 * (x + y + 0i32); + //~^ ERROR: this operation has no effect + // No need for parenthesis if the parent expr is of equal precedence + let _ = 2i32 + (x + y + 0i32); + //~^ ERROR: this operation has no effect + // But make sure that inner parens still exist + let z = 1i32; + let _ = 2 + (x + (y * z) + 0); + //~^ ERROR: this operation has no effect + // Maintain parenthesis if the parent expr is of lower precedence + // This is for clarity, and clippy will not warn on these being unnecessary + let _ = 2i32 + (x * y * 1i32); + //~^ ERROR: this operation has no effect + + let x = 1i16; + let y = 1i16; + let _: u64 = 1u64 + ((x as i32 + y as i32) as u64 + 0u64); + //~^ ERROR: this operation has no effect +} diff --git a/tests/ui/identity_op.stderr b/tests/ui/identity_op.stderr index 9fff86b86f9f..856bebfa1ffc 100644 --- a/tests/ui/identity_op.stderr +++ b/tests/ui/identity_op.stderr @@ -221,7 +221,7 @@ error: this operation has no effect --> tests/ui/identity_op.rs:152:5 | LL | 1 * ({ a } + 4); - | ^^^^^^^^^^^^^^^ help: consider reducing it to: `(({ a } + 4))` + | ^^^^^^^^^^^^^^^ help: consider reducing it to: `({ a } + 4)` error: this operation has no effect --> tests/ui/identity_op.rs:154:5 @@ -313,5 +313,65 @@ error: this operation has no effect LL | let _: i32 = **&&*&x + 0; | ^^^^^^^^^^^ help: consider reducing it to: `***&&*&x` -error: aborting due to 52 previous errors +error: this operation has no effect + --> tests/ui/identity_op.rs:220:18 + | +LL | let _: u64 = (x + y + 0i32) as u64; + | ^^^^^^^^^^^^^^ help: consider reducing it to: `(x + y)` + +error: this operation has no effect + --> tests/ui/identity_op.rs:223:25 + | +LL | let _: u64 = 1u64 & (x + y + 0i32) as u64; + | ^^^^^^^^^^^^^^ help: consider reducing it to: `(x + y)` + +error: this operation has no effect + --> tests/ui/identity_op.rs:226:25 + | +LL | let _: u64 = 1u64 & ((x + y) + 0i32) as u64; + | ^^^^^^^^^^^^^^^^ help: consider reducing it to: `(x + y)` + +error: this operation has no effect + --> tests/ui/identity_op.rs:229:25 + | +LL | let _: u64 = 5u64 + ((x + y) + 0i32) as u64; + | ^^^^^^^^^^^^^^^^ help: consider reducing it to: `(x + y)` + +error: this operation has no effect + --> tests/ui/identity_op.rs:233:14 + | +LL | let _ = -(x + y + 0i32); + | ^^^^^^^^^^^^^^ help: consider reducing it to: `(x + y)` + +error: this operation has no effect + --> tests/ui/identity_op.rs:236:20 + | +LL | let _ = 2i32 * (x + y + 0i32); + | ^^^^^^^^^^^^^^ help: consider reducing it to: `(x + y)` + +error: this operation has no effect + --> tests/ui/identity_op.rs:239:20 + | +LL | let _ = 2i32 + (x + y + 0i32); + | ^^^^^^^^^^^^^^ help: consider reducing it to: `x + y` + +error: this operation has no effect + --> tests/ui/identity_op.rs:243:17 + | +LL | let _ = 2 + (x + (y * z) + 0); + | ^^^^^^^^^^^^^^^^^ help: consider reducing it to: `x + (y * z)` + +error: this operation has no effect + --> tests/ui/identity_op.rs:247:20 + | +LL | let _ = 2i32 + (x * y * 1i32); + | ^^^^^^^^^^^^^^ help: consider reducing it to: `(x * y)` + +error: this operation has no effect + --> tests/ui/identity_op.rs:252:25 + | +LL | let _: u64 = 1u64 + ((x as i32 + y as i32) as u64 + 0u64); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `(x as i32 + y as i32) as u64` + +error: aborting due to 62 previous errors From 6d738f6bc032ca539d08a7d59674f1e74e539674 Mon Sep 17 00:00:00 2001 From: TheSlapstickDictator <6685366+TheSlapstickDictator@users.noreply.github.com> Date: Sun, 3 Nov 2024 09:23:03 -0800 Subject: [PATCH 2/4] Fix parens getting removed for non-associative operators --- clippy_lints/src/operators/identity_op.rs | 22 ++++++---------------- tests/ui/identity_op.fixed | 5 +++-- tests/ui/identity_op.rs | 5 +++-- tests/ui/identity_op.stderr | 12 ++++++------ 4 files changed, 18 insertions(+), 26 deletions(-) diff --git a/clippy_lints/src/operators/identity_op.rs b/clippy_lints/src/operators/identity_op.rs index d8430f6492d2..e3ce1ae9427d 100644 --- a/clippy_lints/src/operators/identity_op.rs +++ b/clippy_lints/src/operators/identity_op.rs @@ -52,13 +52,8 @@ pub(crate) fn check<'tcx>( }, BinOpKind::Shl | BinOpKind::Shr | BinOpKind::Sub => { if is_redundant_op(cx, right, 0) { - span_ineffective_operation( - cx, - expr.span, - peeled_left_span, - Parens::Unneeded, - left_is_coerced_to_value, - ); + let paren = needs_parenthesis(cx, expr, left); + span_ineffective_operation(cx, expr.span, peeled_left_span, paren, left_is_coerced_to_value); } }, BinOpKind::Mul => { @@ -127,17 +122,12 @@ fn needs_parenthesis(cx: &LateContext<'_>, binary: &Expr<'_>, child: &Expr<'_>) match child.kind { ExprKind::Binary(_, lhs, _) | ExprKind::Cast(lhs, _) => { // For casts and binary expressions, we want to add parenthesis if - // the parent HIR node is an expression with a different precedence, - // or if the parent HIR node is a Block or Stmt, and the new left hand side - // would be treated as a statement rather than an expression. + // the parent HIR node is an expression, or if the parent HIR node + // is a Block or Stmt, and the new left hand side would need + // parenthesis be treated as a statement rather than an expression. if let Some((_, parent)) = cx.tcx.hir().parent_iter(binary.hir_id).next() { - if let Node::Expr(expr) = parent { - if child.precedence().order() != expr.precedence().order() { - return Parens::Needed; - } - return Parens::Unneeded; - } match parent { + Node::Expr(_) => return Parens::Needed, Node::Block(_) | Node::Stmt(_) => { // ensure we're checking against the leftmost expression of `child` // diff --git a/tests/ui/identity_op.fixed b/tests/ui/identity_op.fixed index 18b7401768df..927142d5f802 100644 --- a/tests/ui/identity_op.fixed +++ b/tests/ui/identity_op.fixed @@ -235,8 +235,9 @@ fn issue_13470() { // Maintain parenthesis if the parent expr is of higher precedence let _ = 2i32 * (x + y); //~^ ERROR: this operation has no effect - // No need for parenthesis if the parent expr is of equal precedence - let _ = 2i32 + x + y; + // Maintain parenthesis if the parent expr is the same precedence + // as not all operations are associative + let _ = 2i32 - (x - y); //~^ ERROR: this operation has no effect // But make sure that inner parens still exist let z = 1i32; diff --git a/tests/ui/identity_op.rs b/tests/ui/identity_op.rs index 55483f4e5fb3..a50546929a82 100644 --- a/tests/ui/identity_op.rs +++ b/tests/ui/identity_op.rs @@ -235,8 +235,9 @@ fn issue_13470() { // Maintain parenthesis if the parent expr is of higher precedence let _ = 2i32 * (x + y + 0i32); //~^ ERROR: this operation has no effect - // No need for parenthesis if the parent expr is of equal precedence - let _ = 2i32 + (x + y + 0i32); + // Maintain parenthesis if the parent expr is the same precedence + // as not all operations are associative + let _ = 2i32 - (x - y - 0i32); //~^ ERROR: this operation has no effect // But make sure that inner parens still exist let z = 1i32; diff --git a/tests/ui/identity_op.stderr b/tests/ui/identity_op.stderr index 856bebfa1ffc..79d1a7bd81c3 100644 --- a/tests/ui/identity_op.stderr +++ b/tests/ui/identity_op.stderr @@ -350,25 +350,25 @@ LL | let _ = 2i32 * (x + y + 0i32); | ^^^^^^^^^^^^^^ help: consider reducing it to: `(x + y)` error: this operation has no effect - --> tests/ui/identity_op.rs:239:20 + --> tests/ui/identity_op.rs:240:20 | -LL | let _ = 2i32 + (x + y + 0i32); - | ^^^^^^^^^^^^^^ help: consider reducing it to: `x + y` +LL | let _ = 2i32 - (x - y - 0i32); + | ^^^^^^^^^^^^^^ help: consider reducing it to: `(x - y)` error: this operation has no effect - --> tests/ui/identity_op.rs:243:17 + --> tests/ui/identity_op.rs:244:17 | LL | let _ = 2 + (x + (y * z) + 0); | ^^^^^^^^^^^^^^^^^ help: consider reducing it to: `x + (y * z)` error: this operation has no effect - --> tests/ui/identity_op.rs:247:20 + --> tests/ui/identity_op.rs:248:20 | LL | let _ = 2i32 + (x * y * 1i32); | ^^^^^^^^^^^^^^ help: consider reducing it to: `(x * y)` error: this operation has no effect - --> tests/ui/identity_op.rs:252:25 + --> tests/ui/identity_op.rs:253:25 | LL | let _: u64 = 1u64 + ((x as i32 + y as i32) as u64 + 0u64); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `(x as i32 + y as i32) as u64` From 1b7239d9548d5d6672ac05ba1f48019a29b9790d Mon Sep 17 00:00:00 2001 From: TheSlapstickDictator <6685366+TheSlapstickDictator@users.noreply.github.com> Date: Mon, 4 Nov 2024 19:19:58 -0800 Subject: [PATCH 3/4] Fixing a missed check for needs_parenthesis in division --- clippy_lints/src/operators/identity_op.rs | 9 ++------- tests/ui/identity_op.fixed | 3 +++ tests/ui/identity_op.rs | 3 +++ tests/ui/identity_op.stderr | 18 ++++++++++++------ 4 files changed, 20 insertions(+), 13 deletions(-) diff --git a/clippy_lints/src/operators/identity_op.rs b/clippy_lints/src/operators/identity_op.rs index e3ce1ae9427d..769937857276 100644 --- a/clippy_lints/src/operators/identity_op.rs +++ b/clippy_lints/src/operators/identity_op.rs @@ -67,13 +67,8 @@ pub(crate) fn check<'tcx>( }, BinOpKind::Div => { if is_redundant_op(cx, right, 1) { - span_ineffective_operation( - cx, - expr.span, - peeled_left_span, - Parens::Unneeded, - left_is_coerced_to_value, - ); + let paren = needs_parenthesis(cx, expr, left); + span_ineffective_operation(cx, expr.span, peeled_left_span, paren, left_is_coerced_to_value); } }, BinOpKind::BitAnd => { diff --git a/tests/ui/identity_op.fixed b/tests/ui/identity_op.fixed index 927142d5f802..b80ba701c3ad 100644 --- a/tests/ui/identity_op.fixed +++ b/tests/ui/identity_op.fixed @@ -232,6 +232,9 @@ fn issue_13470() { // If we don't maintain the parens here, the behavior changes let _ = -(x + y); //~^ ERROR: this operation has no effect + // Similarly, we need to maintain parens here + let _ = -(x / y); + //~^ ERROR: this operation has no effect // Maintain parenthesis if the parent expr is of higher precedence let _ = 2i32 * (x + y); //~^ ERROR: this operation has no effect diff --git a/tests/ui/identity_op.rs b/tests/ui/identity_op.rs index a50546929a82..3e20fa6f2b89 100644 --- a/tests/ui/identity_op.rs +++ b/tests/ui/identity_op.rs @@ -232,6 +232,9 @@ fn issue_13470() { // If we don't maintain the parens here, the behavior changes let _ = -(x + y + 0i32); //~^ ERROR: this operation has no effect + // Similarly, we need to maintain parens here + let _ = -(x / y / 1i32); + //~^ ERROR: this operation has no effect // Maintain parenthesis if the parent expr is of higher precedence let _ = 2i32 * (x + y + 0i32); //~^ ERROR: this operation has no effect diff --git a/tests/ui/identity_op.stderr b/tests/ui/identity_op.stderr index 79d1a7bd81c3..3f5cab5404d0 100644 --- a/tests/ui/identity_op.stderr +++ b/tests/ui/identity_op.stderr @@ -344,34 +344,40 @@ LL | let _ = -(x + y + 0i32); | ^^^^^^^^^^^^^^ help: consider reducing it to: `(x + y)` error: this operation has no effect - --> tests/ui/identity_op.rs:236:20 + --> tests/ui/identity_op.rs:236:14 + | +LL | let _ = -(x / y / 1i32); + | ^^^^^^^^^^^^^^ help: consider reducing it to: `(x / y)` + +error: this operation has no effect + --> tests/ui/identity_op.rs:239:20 | LL | let _ = 2i32 * (x + y + 0i32); | ^^^^^^^^^^^^^^ help: consider reducing it to: `(x + y)` error: this operation has no effect - --> tests/ui/identity_op.rs:240:20 + --> tests/ui/identity_op.rs:243:20 | LL | let _ = 2i32 - (x - y - 0i32); | ^^^^^^^^^^^^^^ help: consider reducing it to: `(x - y)` error: this operation has no effect - --> tests/ui/identity_op.rs:244:17 + --> tests/ui/identity_op.rs:247:17 | LL | let _ = 2 + (x + (y * z) + 0); | ^^^^^^^^^^^^^^^^^ help: consider reducing it to: `x + (y * z)` error: this operation has no effect - --> tests/ui/identity_op.rs:248:20 + --> tests/ui/identity_op.rs:251:20 | LL | let _ = 2i32 + (x * y * 1i32); | ^^^^^^^^^^^^^^ help: consider reducing it to: `(x * y)` error: this operation has no effect - --> tests/ui/identity_op.rs:253:25 + --> tests/ui/identity_op.rs:256:25 | LL | let _: u64 = 1u64 + ((x as i32 + y as i32) as u64 + 0u64); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `(x as i32 + y as i32) as u64` -error: aborting due to 62 previous errors +error: aborting due to 63 previous errors From ef0f1cad593c24bc80f0b5d2255fafc561a47030 Mon Sep 17 00:00:00 2001 From: TheSlapstickDictator <6685366+TheSlapstickDictator@users.noreply.github.com> Date: Thu, 7 Nov 2024 20:45:15 -0800 Subject: [PATCH 4/4] Remove check for duplicate parenthesis It was an incorrect, and could lead to behavior changes in the suggested code --- clippy_lints/src/operators/identity_op.rs | 8 +------- tests/ui/identity_op.fixed | 12 ++++++------ tests/ui/identity_op.stderr | 12 ++++++------ 3 files changed, 13 insertions(+), 19 deletions(-) diff --git a/clippy_lints/src/operators/identity_op.rs b/clippy_lints/src/operators/identity_op.rs index 769937857276..1c2d6e90fc95 100644 --- a/clippy_lints/src/operators/identity_op.rs +++ b/clippy_lints/src/operators/identity_op.rs @@ -220,13 +220,7 @@ fn span_ineffective_operation( expr_snippet.into_owned() }; let suggestion = match parens { - Parens::Needed => { - if !expr_snippet.starts_with('(') && !expr_snippet.ends_with(')') { - format!("({expr_snippet})") - } else { - expr_snippet - } - }, + Parens::Needed => format!("({expr_snippet})"), Parens::Unneeded => expr_snippet, }; diff --git a/tests/ui/identity_op.fixed b/tests/ui/identity_op.fixed index b80ba701c3ad..2e8e2366de6f 100644 --- a/tests/ui/identity_op.fixed +++ b/tests/ui/identity_op.fixed @@ -116,7 +116,7 @@ fn main() { //~^ ERROR: this operation has no effect (match a { 0 => 10, _ => 20 }) + if b { 3 } else { 4 }; //~^ ERROR: this operation has no effect - (if b { 1 } else { 2 }); + ((if b { 1 } else { 2 })); //~^ ERROR: this operation has no effect ({ a }) + 3; @@ -149,7 +149,7 @@ fn main() { 2 * { a }; //~^ ERROR: this operation has no effect - ({ a } + 4); + (({ a } + 4)); //~^ ERROR: this operation has no effect 1; //~^ ERROR: this operation has no effect @@ -223,10 +223,10 @@ fn issue_13470() { let _: u64 = 1u64 & (x + y) as u64; //~^ ERROR: this operation has no effect // Same as above, but with extra redundant parenthesis - let _: u64 = 1u64 & (x + y) as u64; + let _: u64 = 1u64 & ((x + y)) as u64; //~^ ERROR: this operation has no effect // Should maintain parenthesis even if the surrounding expr has the same precedence - let _: u64 = 5u64 + (x + y) as u64; + let _: u64 = 5u64 + ((x + y)) as u64; //~^ ERROR: this operation has no effect // If we don't maintain the parens here, the behavior changes @@ -244,7 +244,7 @@ fn issue_13470() { //~^ ERROR: this operation has no effect // But make sure that inner parens still exist let z = 1i32; - let _ = 2 + x + (y * z); + let _ = 2 + (x + (y * z)); //~^ ERROR: this operation has no effect // Maintain parenthesis if the parent expr is of lower precedence // This is for clarity, and clippy will not warn on these being unnecessary @@ -253,6 +253,6 @@ fn issue_13470() { let x = 1i16; let y = 1i16; - let _: u64 = 1u64 + (x as i32 + y as i32) as u64; + let _: u64 = 1u64 + ((x as i32 + y as i32) as u64); //~^ ERROR: this operation has no effect } diff --git a/tests/ui/identity_op.stderr b/tests/ui/identity_op.stderr index 3f5cab5404d0..99a58ef2c1b5 100644 --- a/tests/ui/identity_op.stderr +++ b/tests/ui/identity_op.stderr @@ -149,7 +149,7 @@ error: this operation has no effect --> tests/ui/identity_op.rs:119:5 | LL | (if b { 1 } else { 2 }) + 0; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `(if b { 1 } else { 2 })` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `((if b { 1 } else { 2 }))` error: this operation has no effect --> tests/ui/identity_op.rs:122:5 @@ -221,7 +221,7 @@ error: this operation has no effect --> tests/ui/identity_op.rs:152:5 | LL | 1 * ({ a } + 4); - | ^^^^^^^^^^^^^^^ help: consider reducing it to: `({ a } + 4)` + | ^^^^^^^^^^^^^^^ help: consider reducing it to: `(({ a } + 4))` error: this operation has no effect --> tests/ui/identity_op.rs:154:5 @@ -329,13 +329,13 @@ error: this operation has no effect --> tests/ui/identity_op.rs:226:25 | LL | let _: u64 = 1u64 & ((x + y) + 0i32) as u64; - | ^^^^^^^^^^^^^^^^ help: consider reducing it to: `(x + y)` + | ^^^^^^^^^^^^^^^^ help: consider reducing it to: `((x + y))` error: this operation has no effect --> tests/ui/identity_op.rs:229:25 | LL | let _: u64 = 5u64 + ((x + y) + 0i32) as u64; - | ^^^^^^^^^^^^^^^^ help: consider reducing it to: `(x + y)` + | ^^^^^^^^^^^^^^^^ help: consider reducing it to: `((x + y))` error: this operation has no effect --> tests/ui/identity_op.rs:233:14 @@ -365,7 +365,7 @@ error: this operation has no effect --> tests/ui/identity_op.rs:247:17 | LL | let _ = 2 + (x + (y * z) + 0); - | ^^^^^^^^^^^^^^^^^ help: consider reducing it to: `x + (y * z)` + | ^^^^^^^^^^^^^^^^^ help: consider reducing it to: `(x + (y * z))` error: this operation has no effect --> tests/ui/identity_op.rs:251:20 @@ -377,7 +377,7 @@ error: this operation has no effect --> tests/ui/identity_op.rs:256:25 | LL | let _: u64 = 1u64 + ((x as i32 + y as i32) as u64 + 0u64); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `(x as i32 + y as i32) as u64` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider reducing it to: `((x as i32 + y as i32) as u64)` error: aborting due to 63 previous errors