Skip to content

[FP] identity_op in front of if #8730

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 3, 2022
Merged

[FP] identity_op in front of if #8730

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 59 additions & 23 deletions clippy_lints/src/identity_op.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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};
Expand All @@ -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,
Expand All @@ -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()
Expand Down
30 changes: 30 additions & 0 deletions tests/ui/identity_op.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to see a testcase like let _c = 0 + if b { 1 } else { 2 };
Maybe also a function like I wrote in #8724:

pub fn decide(a: bool, b: bool) -> u32 {
    0 + if a { 1 } else { 2 } + if b { 3 } else { 5 }
}

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 };
Comment on lines +95 to +96
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a false negative in here, in contexts where it's unambiguously an expression it is fine to reduce these, e.g.

let b = true;

f(0 + if b { 1 } else { 2 } + 3);
let _ = 0 + if b { 1 } else { 2 } + 3;
const _: i32 = 0 + if b { 1 } else { 2 } + 3;

Perhaps instead of get_parent_expr it should be a get_parent_node, checking the node's variant

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.
But I think it seems to be enough to use get_parent_expr.
Is there need to check node variants, using get_parent_node?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those cases don't lint currently but they could do is what I mean, as in

// doesn't trigger the lint
f(0 + if b { 1 } else { 2 } + 3);
// but it could, as the following is valid
f(if b { 1 } else { 2 } + 3);

It'd be also fine to leave it as is and make a note of that under a Known problems section in the lint description

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't aware of this case. I will leave it as it is and add documents.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added documents


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
}
92 changes: 91 additions & 1 deletion tests/ui/identity_op.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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