Mirrors/rust-clippy
2
0
Fork 0
mirror of https://github.com/rust-lang/rust-clippy synced 2024-11-21 20:23:12 +00:00
Code Issues Projects Releases Packages Wiki Activity

fix: identity_op suggestions use correct parenthesis (#13647)

Browse source 
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 rustfix generating broken code and failing, or
generating 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 an error where `x` cannot be added
to the other values, as it was never cast to `u64`.

This commit fixes both of these problems by always checking the
non-redundant child of a binary expression for needed parenthesis.

fixes #13470

changelog: [`identity_op`]: Fix suggested code that is broken or has
changed behavior
This commit is contained in:
Jason Newcomb 2024-11-09 05:03:10 +00:00 committed by GitHub
commit 8cfb95959b
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 245 additions and 105 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

190
clippy_lints/src/operators/identity_op.rs
View file

@ -42,83 +42,43 @@ 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) {
let paren = needs_parenthesis(cx, expr, left);
span_ineffective_operation(cx, expr.span, peeled_left_span, paren, 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) {
let paren = needs_parenthesis(cx, expr, left);
span_ineffective_operation(cx, expr.span, peeled_left_span, paren, 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 +98,70 @@ 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, 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() {
match parent {
Node::Expr(_) => return Parens::Needed,
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(..) => {
// 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;
}
match parent {
Node::Block(_) | Node::Stmt(_) => return Parens::Needed,
_ => return Parens::Unneeded,
};
}
},
_ => {
return Parens::Unneeded;
},
ExprKind::If(..) | ExprKind::Match(..) | ExprKind::Block(..) | ExprKind::Loop(..) => {},
_ => return Parens::Unneeded,
}
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
}
@ -199,7 +186,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 +199,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;
}
}

46
tests/ui/identity_op.fixed
View file

@ -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;
@ -212,3 +212,47 @@ 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
// 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
// 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;
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
}

44
tests/ui/identity_op.rs
View file

@ -212,3 +212,47 @@ 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
// 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
// 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;
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
}

70
tests/ui/identity_op.stderr
View file

@ -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
@ -313,5 +313,71 @@ 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: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: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: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: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: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 63 previous errors