From ba70842c625abed2a61aa84dd0a7bd4b7b874063 Mon Sep 17 00:00:00 2001 From: Alex Ozdemir Date: Mon, 27 Dec 2021 16:06:27 -0800 Subject: [PATCH 1/3] Limit the identity_op lint to integral operands. If operands are being applied to non-integers, then the lint is likely wrong. --- clippy_lints/src/identity_op.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/identity_op.rs b/clippy_lints/src/identity_op.rs index b4e7bbc76713..a833e17917eb 100644 --- a/clippy_lints/src/identity_op.rs +++ b/clippy_lints/src/identity_op.rs @@ -61,10 +61,13 @@ impl<'tcx> LateLintPass<'tcx> for IdentityOp { } fn is_allowed(cx: &LateContext<'_>, cmp: BinOp, left: &Expr<'_>, right: &Expr<'_>) -> bool { - // `1 << 0` is a common pattern in bit manipulation code - cmp.node == BinOpKind::Shl - && constant_simple(cx, cx.typeck_results(), right) == Some(Constant::Int(0)) - && constant_simple(cx, cx.typeck_results(), left) == Some(Constant::Int(1)) + // This lint applies to integers + !cx.typeck_results().expr_ty(left).is_integral() + || !cx.typeck_results().expr_ty(right).is_integral() + // `1 << 0` is a common pattern in bit manipulation code + || (cmp.node == BinOpKind::Shl + && constant_simple(cx, cx.typeck_results(), right) == Some(Constant::Int(0)) + && constant_simple(cx, cx.typeck_results(), left) == Some(Constant::Int(1))) } fn check(cx: &LateContext<'_>, e: &Expr<'_>, m: i8, span: Span, arg: Span) { From bc0579f5bf10c28de8ca75127d97b884170471ef Mon Sep 17 00:00:00 2001 From: Alex Ozdemir Date: Tue, 28 Dec 2021 08:19:47 -0800 Subject: [PATCH 2/3] test --- clippy_lints/src/identity_op.rs | 4 ++-- tests/ui/identity_op.rs | 14 ++++++++++++++ tests/ui/identity_op.stderr | 30 ++++++++++++++++++------------ 3 files changed, 34 insertions(+), 14 deletions(-) diff --git a/clippy_lints/src/identity_op.rs b/clippy_lints/src/identity_op.rs index a833e17917eb..de60e284cad1 100644 --- a/clippy_lints/src/identity_op.rs +++ b/clippy_lints/src/identity_op.rs @@ -62,8 +62,8 @@ impl<'tcx> LateLintPass<'tcx> for IdentityOp { fn is_allowed(cx: &LateContext<'_>, cmp: BinOp, left: &Expr<'_>, right: &Expr<'_>) -> bool { // This lint applies to integers - !cx.typeck_results().expr_ty(left).is_integral() - || !cx.typeck_results().expr_ty(right).is_integral() + !cx.typeck_results().expr_ty(left).peel_refs().is_integral() + || !cx.typeck_results().expr_ty(right).peel_refs().is_integral() // `1 << 0` is a common pattern in bit manipulation code || (cmp.node == BinOpKind::Shl && constant_simple(cx, cx.typeck_results(), right) == Some(Constant::Int(0)) diff --git a/tests/ui/identity_op.rs b/tests/ui/identity_op.rs index ceaacaaf6bd7..a3985b68fbbb 100644 --- a/tests/ui/identity_op.rs +++ b/tests/ui/identity_op.rs @@ -2,6 +2,16 @@ const ONE: i64 = 1; const NEG_ONE: i64 = -1; const ZERO: i64 = 0; +struct A(String); + +impl std::ops::Shl for A { + type Output = A; + fn shl(mut self, other: i32) -> Self { + self.0.push_str(&format!("{}", other)); + self + } +} + #[allow( clippy::eq_op, clippy::no_effect, @@ -38,4 +48,8 @@ fn main() { 42 << 0; 1 >> 0; 42 >> 0; + &x >> 0; + + let mut a = A("".into()); + let b = a << 0; // no error: non-integer } diff --git a/tests/ui/identity_op.stderr b/tests/ui/identity_op.stderr index d8d44a74f9ab..5b63eb9cc4fc 100644 --- a/tests/ui/identity_op.stderr +++ b/tests/ui/identity_op.stderr @@ -1,5 +1,5 @@ error: the operation is ineffective. Consider reducing it to `x` - --> $DIR/identity_op.rs:16:5 + --> $DIR/identity_op.rs:26:5 | LL | x + 0; | ^^^^^ @@ -7,64 +7,70 @@ LL | x + 0; = note: `-D clippy::identity-op` implied by `-D warnings` error: the operation is ineffective. Consider reducing it to `x` - --> $DIR/identity_op.rs:17:5 + --> $DIR/identity_op.rs:27:5 | LL | x + (1 - 1); | ^^^^^^^^^^^ error: the operation is ineffective. Consider reducing it to `x` - --> $DIR/identity_op.rs:19:5 + --> $DIR/identity_op.rs:29:5 | LL | 0 + x; | ^^^^^ error: the operation is ineffective. Consider reducing it to `x` - --> $DIR/identity_op.rs:22:5 + --> $DIR/identity_op.rs:32:5 | LL | x | (0); | ^^^^^^^ error: the operation is ineffective. Consider reducing it to `x` - --> $DIR/identity_op.rs:25:5 + --> $DIR/identity_op.rs:35:5 | LL | x * 1; | ^^^^^ error: the operation is ineffective. Consider reducing it to `x` - --> $DIR/identity_op.rs:26:5 + --> $DIR/identity_op.rs:36:5 | LL | 1 * x; | ^^^^^ error: the operation is ineffective. Consider reducing it to `x` - --> $DIR/identity_op.rs:32:5 + --> $DIR/identity_op.rs:42:5 | LL | -1 & x; | ^^^^^^ error: the operation is ineffective. Consider reducing it to `u` - --> $DIR/identity_op.rs:35:5 + --> $DIR/identity_op.rs:45:5 | LL | u & 255; | ^^^^^^^ error: the operation is ineffective. Consider reducing it to `42` - --> $DIR/identity_op.rs:38:5 + --> $DIR/identity_op.rs:48:5 | LL | 42 << 0; | ^^^^^^^ error: the operation is ineffective. Consider reducing it to `1` - --> $DIR/identity_op.rs:39:5 + --> $DIR/identity_op.rs:49:5 | LL | 1 >> 0; | ^^^^^^ error: the operation is ineffective. Consider reducing it to `42` - --> $DIR/identity_op.rs:40:5 + --> $DIR/identity_op.rs:50:5 | LL | 42 >> 0; | ^^^^^^^ -error: aborting due to 11 previous errors +error: the operation is ineffective. Consider reducing it to `&x` + --> $DIR/identity_op.rs:51:5 + | +LL | &x >> 0; + | ^^^^^^^ + +error: aborting due to 12 previous errors From ee6d5c5cdae8b7cb3198800a615855169fc0b3de Mon Sep 17 00:00:00 2001 From: Alex Ozdemir Date: Tue, 28 Dec 2021 08:32:55 -0800 Subject: [PATCH 3/3] contants peel_refs to catch `x << &0` --- clippy_lints/src/identity_op.rs | 4 ++-- clippy_utils/src/consts.rs | 8 ++++++++ tests/ui/identity_op.rs | 3 ++- tests/ui/identity_op.stderr | 8 +++++++- 4 files changed, 19 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/identity_op.rs b/clippy_lints/src/identity_op.rs index de60e284cad1..f824f20ca40a 100644 --- a/clippy_lints/src/identity_op.rs +++ b/clippy_lints/src/identity_op.rs @@ -71,8 +71,8 @@ fn is_allowed(cx: &LateContext<'_>, cmp: BinOp, left: &Expr<'_>, right: &Expr<'_ } fn check(cx: &LateContext<'_>, e: &Expr<'_>, m: i8, span: Span, arg: Span) { - if let Some(Constant::Int(v)) = constant_simple(cx, cx.typeck_results(), e) { - let check = match *cx.typeck_results().expr_ty(e).kind() { + 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), ty::Uint(uty) => clip(cx.tcx, !0, uty), _ => return, diff --git a/clippy_utils/src/consts.rs b/clippy_utils/src/consts.rs index 04347672e0fb..1cf8f9721b58 100644 --- a/clippy_utils/src/consts.rs +++ b/clippy_utils/src/consts.rs @@ -168,6 +168,14 @@ impl Constant { None } } + + #[must_use] + pub fn peel_refs(mut self) -> Self { + while let Constant::Ref(r) = self { + self = *r; + } + self + } } /// Parses a `LitKind` to a `Constant`. diff --git a/tests/ui/identity_op.rs b/tests/ui/identity_op.rs index a3985b68fbbb..2ed4b5db574d 100644 --- a/tests/ui/identity_op.rs +++ b/tests/ui/identity_op.rs @@ -11,11 +11,11 @@ impl std::ops::Shl for A { self } } - #[allow( clippy::eq_op, clippy::no_effect, clippy::unnecessary_operation, + clippy::op_ref, clippy::double_parens )] #[warn(clippy::identity_op)] @@ -49,6 +49,7 @@ fn main() { 1 >> 0; 42 >> 0; &x >> 0; + x >> &0; let mut a = A("".into()); let b = a << 0; // no error: non-integer diff --git a/tests/ui/identity_op.stderr b/tests/ui/identity_op.stderr index 5b63eb9cc4fc..ff34b38db015 100644 --- a/tests/ui/identity_op.stderr +++ b/tests/ui/identity_op.stderr @@ -72,5 +72,11 @@ error: the operation is ineffective. Consider reducing it to `&x` LL | &x >> 0; | ^^^^^^^ -error: aborting due to 12 previous errors +error: the operation is ineffective. Consider reducing it to `x` + --> $DIR/identity_op.rs:52:5 + | +LL | x >> &0; + | ^^^^^^^ + +error: aborting due to 13 previous errors