feat: manual_rotate also recognize non-consts

clippy_lints/src/manual_rotate.rs

@@ -56,20 +56,14 @@ impl Display for ShiftDirection {
     }
 }
 
-fn parse_shift<'tcx>(
-    cx: &LateContext<'tcx>,
-    expr: &'tcx Expr<'tcx>,
-) -> Option<(ShiftDirection, u128, &'tcx Expr<'tcx>)> {
+fn parse_shift<'tcx>(expr: &'tcx Expr<'tcx>) -> Option<(ShiftDirection, &'tcx Expr<'tcx>, &'tcx Expr<'tcx>)> {
     if let ExprKind::Binary(op, l, r) = expr.kind {
         let dir = match op.node {
             BinOpKind::Shl => ShiftDirection::Left,
             BinOpKind::Shr => ShiftDirection::Right,
             _ => return None,
         };
-        let const_expr = ConstEvalCtxt::new(cx).eval(r)?;
-        if let Constant::Int(shift) = const_expr {
-            return Some((dir, shift, l));
-        }
+        return Some((dir, l, r));
     }
     None
 }
@@ -78,40 +72,60 @@ impl LateLintPass<'_> for ManualRotate {
     fn check_expr<'tcx>(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
         if let ExprKind::Binary(op, l, r) = expr.kind
             && let BinOpKind::Add | BinOpKind::BitOr = op.node
-            && let Some((l_shift_dir, l_amount, l_expr)) = parse_shift(cx, l)
-            && let Some((r_shift_dir, r_amount, r_expr)) = parse_shift(cx, r)
-        {
-            if l_shift_dir == r_shift_dir {
-                return;
-            }
-            if !clippy_utils::eq_expr_value(cx, l_expr, r_expr) {
-                return;
-            }
-            let Some(bit_width) = (match cx.typeck_results().expr_ty(expr).kind() {
+            && let Some((l_shift_dir, l_expr, l_amount)) = parse_shift(l)
+            && let Some((r_shift_dir, r_expr, r_amount)) = parse_shift(r)
+            && l_shift_dir != r_shift_dir
+            && clippy_utils::eq_expr_value(cx, l_expr, r_expr)
+            && let Some(bit_width) = match cx.typeck_results().expr_ty(expr).kind() {
                 ty::Int(itype) => itype.bit_width(),
                 ty::Uint(itype) => itype.bit_width(),
                 _ => return,
-            }) else {
-                return;
-            };
-            if l_amount + r_amount == u128::from(bit_width) {
-                let (shift_function, amount) = if l_amount < r_amount {
+            }
+        {
+            let const_eval = ConstEvalCtxt::new(cx);
+
+            let (shift_function, amount) = if let Some(Constant::Int(l_amount_val)) = const_eval.eval(l_amount)
+                && let Some(Constant::Int(r_amount_val)) = const_eval.eval(r_amount)
+                && l_amount_val + r_amount_val == u128::from(bit_width)
+            {
+                if l_amount_val < r_amount_val {
                     (l_shift_dir, l_amount)
                 } else {
                     (r_shift_dir, r_amount)
+                }
+            } else {
+                let (amount1, binop, minuend, amount2, shift_direction) = match (l_amount.kind, r_amount.kind) {
+                    (_, ExprKind::Binary(binop, minuend, other)) => (l_amount, binop, minuend, other, l_shift_dir),
+                    (ExprKind::Binary(binop, minuend, other), _) => (r_amount, binop, minuend, other, r_shift_dir),
+                    _ => return,
                 };
-                let mut applicability = Applicability::MachineApplicable;
-                let expr_sugg = sugg::Sugg::hir_with_applicability(cx, l_expr, "_", &mut applicability).maybe_paren();
-                span_lint_and_sugg(
-                    cx,
-                    MANUAL_ROTATE,
-                    expr.span,
-                    "there is no need to manually implement bit rotation",
-                    "this expression can be rewritten as",
-                    format!("{expr_sugg}.{shift_function}({amount})"),
-                    Applicability::MachineApplicable,
-                );
-            }
+
+                if let Some(Constant::Int(minuend)) = const_eval.eval_simple(minuend)
+                    && clippy_utils::eq_expr_value(cx, amount1, amount2)
+                    // (x << s) | (x >> bit_width - s)
+                    && ((binop.node == BinOpKind::Sub && u128::from(bit_width) == minuend)
+                        // (x << s) | (x >> (bit_width - 1) ^ s)
+                        || (binop.node == BinOpKind::BitXor && u128::from(bit_width).checked_sub(minuend) == Some(1)))
+                {
+                    // NOTE: we take these from the side that _doesn't_ have the binop, since it's probably simpler
+                    (shift_direction, amount1)
+                } else {
+                    return;
+                }
+            };
+
+            let mut applicability = Applicability::MachineApplicable;
+            let expr_sugg = sugg::Sugg::hir_with_applicability(cx, l_expr, "_", &mut applicability).maybe_paren();
+            let amount = sugg::Sugg::hir_with_applicability(cx, amount, "_", &mut applicability);
+            span_lint_and_sugg(
+                cx,
+                MANUAL_ROTATE,
+                expr.span,
+                "there is no need to manually implement bit rotation",
+                "this expression can be rewritten as",
+                format!("{expr_sugg}.{shift_function}({amount})"),
+                Applicability::MachineApplicable,
+            );
         }
     }
 }

tests/ui/manual_rotate.fixed

@@ -4,6 +4,7 @@ fn main() {
     let (x_u8, x_u16, x_u32, x_u64) = (1u8, 1u16, 1u32, 1u64);
     let (x_i8, x_i16, x_i32, x_i64) = (1i8, 1i16, 1i32, 1i64);
     let a_u32 = 1u32;
+    const N: u32 = 5;
     // True positives
     let y_u8 = x_u8.rotate_right(3);
     //~^ manual_rotate
@@ -29,6 +30,9 @@ fn main() {
     //~^ manual_rotate
     let y_u64_as = (x_u32 as u64).rotate_right(8);
     //~^ manual_rotate
+    // shift by a const
+    let _ = x_i64.rotate_right(N);
+    //~^ manual_rotate
 
     // False positives - can't be replaced with a rotation
     let y_u8_false = (x_u8 >> 6) | (x_u8 << 3);
@@ -40,3 +44,20 @@ fn main() {
     let mut l = vec![12_u8, 34];
     let y = (l.pop().unwrap() << 3) + (l.pop().unwrap() >> 5);
 }
+
+fn issue13028() {
+    let s = 5;
+    let u = 5;
+    let x: u32 = 123456;
+
+    let _ = x.rotate_left(s);
+    //~^ manual_rotate
+    let _ = x.rotate_left(s);
+    //~^ manual_rotate
+    // still works with consts
+    let _ = x.rotate_right(9);
+    //~^ manual_rotate
+
+    // don't lint, because `s` and `u` are different variables, albeit with the same value
+    let _ = (x << s) | (x >> (32 - u));
+}

tests/ui/manual_rotate.rs

@@ -4,6 +4,7 @@ fn main() {
     let (x_u8, x_u16, x_u32, x_u64) = (1u8, 1u16, 1u32, 1u64);
     let (x_i8, x_i16, x_i32, x_i64) = (1i8, 1i16, 1i32, 1i64);
     let a_u32 = 1u32;
+    const N: u32 = 5;
     // True positives
     let y_u8 = (x_u8 >> 3) | (x_u8 << 5);
     //~^ manual_rotate
@@ -29,6 +30,9 @@ fn main() {
     //~^ manual_rotate
     let y_u64_as = (x_u32 as u64 >> 8) | ((x_u32 as u64) << 56);
     //~^ manual_rotate
+    // shift by a const
+    let _ = (x_i64 >> N) | (x_i64 << (64 - N));
+    //~^ manual_rotate
 
     // False positives - can't be replaced with a rotation
     let y_u8_false = (x_u8 >> 6) | (x_u8 << 3);
@@ -40,3 +44,20 @@ fn main() {
     let mut l = vec![12_u8, 34];
     let y = (l.pop().unwrap() << 3) + (l.pop().unwrap() >> 5);
 }
+
+fn issue13028() {
+    let s = 5;
+    let u = 5;
+    let x: u32 = 123456;
+
+    let _ = (x << s) | (x >> (32 - s));
+    //~^ manual_rotate
+    let _ = (x << s) | (x >> (31 ^ s));
+    //~^ manual_rotate
+    // still works with consts
+    let _ = (x >> 9) | (x << (32 - 9));
+    //~^ manual_rotate
+
+    // don't lint, because `s` and `u` are different variables, albeit with the same value
+    let _ = (x << s) | (x >> (32 - u));
+}

tests/ui/manual_rotate.stderr

@@ -1,5 +1,5 @@
 error: there is no need to manually implement bit rotation
-  --> tests/ui/manual_rotate.rs:8:16
+  --> tests/ui/manual_rotate.rs:9:16
    |
 LL |     let y_u8 = (x_u8 >> 3) | (x_u8 << 5);
    |                ^^^^^^^^^^^^^^^^^^^^^^^^^ help: this expression can be rewritten as: `x_u8.rotate_right(3)`
@@ -8,64 +8,88 @@ LL |     let y_u8 = (x_u8 >> 3) | (x_u8 << 5);
    = help: to override `-D warnings` add `#[allow(clippy::manual_rotate)]`
 
 error: there is no need to manually implement bit rotation
-  --> tests/ui/manual_rotate.rs:10:17
+  --> tests/ui/manual_rotate.rs:11:17
    |
 LL |     let y_u16 = (x_u16 >> 7) | (x_u16 << 9);
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: this expression can be rewritten as: `x_u16.rotate_right(7)`
 
 error: there is no need to manually implement bit rotation
-  --> tests/ui/manual_rotate.rs:12:17
+  --> tests/ui/manual_rotate.rs:13:17
    |
 LL |     let y_u32 = (x_u32 >> 8) | (x_u32 << 24);
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: this expression can be rewritten as: `x_u32.rotate_right(8)`
 
 error: there is no need to manually implement bit rotation
-  --> tests/ui/manual_rotate.rs:14:17
+  --> tests/ui/manual_rotate.rs:15:17
    |
 LL |     let y_u64 = (x_u64 >> 9) | (x_u64 << 55);
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: this expression can be rewritten as: `x_u64.rotate_right(9)`
 
 error: there is no need to manually implement bit rotation
-  --> tests/ui/manual_rotate.rs:16:16
+  --> tests/ui/manual_rotate.rs:17:16
    |
 LL |     let y_i8 = (x_i8 >> 3) | (x_i8 << 5);
    |                ^^^^^^^^^^^^^^^^^^^^^^^^^ help: this expression can be rewritten as: `x_i8.rotate_right(3)`
 
 error: there is no need to manually implement bit rotation
-  --> tests/ui/manual_rotate.rs:18:17
+  --> tests/ui/manual_rotate.rs:19:17
    |
 LL |     let y_i16 = (x_i16 >> 7) | (x_i16 << 9);
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: this expression can be rewritten as: `x_i16.rotate_right(7)`
 
 error: there is no need to manually implement bit rotation
-  --> tests/ui/manual_rotate.rs:20:17
+  --> tests/ui/manual_rotate.rs:21:17
    |
 LL |     let y_i32 = (x_i32 >> 8) | (x_i32 << 24);
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: this expression can be rewritten as: `x_i32.rotate_right(8)`
 
 error: there is no need to manually implement bit rotation
-  --> tests/ui/manual_rotate.rs:22:17
+  --> tests/ui/manual_rotate.rs:23:17
    |
 LL |     let y_i64 = (x_i64 >> 9) | (x_i64 << 55);
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: this expression can be rewritten as: `x_i64.rotate_right(9)`
 
 error: there is no need to manually implement bit rotation
-  --> tests/ui/manual_rotate.rs:25:22
+  --> tests/ui/manual_rotate.rs:26:22
    |
 LL |     let y_u32_plus = (x_u32 >> 8) + (x_u32 << 24);
    |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: this expression can be rewritten as: `x_u32.rotate_right(8)`
 
 error: there is no need to manually implement bit rotation
-  --> tests/ui/manual_rotate.rs:28:25
+  --> tests/ui/manual_rotate.rs:29:25
    |
 LL |     let y_u32_complex = ((x_u32 | 3256) >> 8) | ((x_u32 | 3256) << 24);
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: this expression can be rewritten as: `(x_u32 | 3256).rotate_right(8)`
 
 error: there is no need to manually implement bit rotation
-  --> tests/ui/manual_rotate.rs:30:20
+  --> tests/ui/manual_rotate.rs:31:20
    |
 LL |     let y_u64_as = (x_u32 as u64 >> 8) | ((x_u32 as u64) << 56);
    |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: this expression can be rewritten as: `(x_u32 as u64).rotate_right(8)`
 
-error: aborting due to 11 previous errors
+error: there is no need to manually implement bit rotation
+  --> tests/ui/manual_rotate.rs:34:13
+   |
+LL |     let _ = (x_i64 >> N) | (x_i64 << (64 - N));
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: this expression can be rewritten as: `x_i64.rotate_right(N)`
+
+error: there is no need to manually implement bit rotation
+  --> tests/ui/manual_rotate.rs:53:13
+   |
+LL |     let _ = (x << s) | (x >> (32 - s));
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: this expression can be rewritten as: `x.rotate_left(s)`
+
+error: there is no need to manually implement bit rotation
+  --> tests/ui/manual_rotate.rs:55:13
+   |
+LL |     let _ = (x << s) | (x >> (31 ^ s));
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: this expression can be rewritten as: `x.rotate_left(s)`
+
+error: there is no need to manually implement bit rotation
+  --> tests/ui/manual_rotate.rs:58:13
+   |
+LL |     let _ = (x >> 9) | (x << (32 - 9));
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: this expression can be rewritten as: `x.rotate_right(9)`
+
+error: aborting due to 15 previous errors