Auto merge of rust-lang#12983 - frp:manual_rotate, r=llogiq

Implement a lint to replace manual bit rotations with rotate_left/rot…

Fixes rust-lang#6861

r? `@llogiq`

---

changelog: add [`manual_rotate`] lint

CHANGELOG.md

@@ -5539,6 +5539,7 @@ Released 2018-09-13
 [`manual_range_patterns`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_patterns
 [`manual_rem_euclid`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_rem_euclid
 [`manual_retain`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_retain
+[`manual_rotate`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_rotate
 [`manual_saturating_arithmetic`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_saturating_arithmetic
 [`manual_slice_size_calculation`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_slice_size_calculation
 [`manual_split_once`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_split_once

clippy_lints/src/declared_lints.rs

@@ -313,6 +313,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::manual_range_patterns::MANUAL_RANGE_PATTERNS_INFO,
     crate::manual_rem_euclid::MANUAL_REM_EUCLID_INFO,
     crate::manual_retain::MANUAL_RETAIN_INFO,
+    crate::manual_rotate::MANUAL_ROTATE_INFO,
     crate::manual_slice_size_calculation::MANUAL_SLICE_SIZE_CALCULATION_INFO,
     crate::manual_string_new::MANUAL_STRING_NEW_INFO,
     crate::manual_strip::MANUAL_STRIP_INFO,

clippy_lints/src/lib.rs

@@ -211,6 +211,7 @@ mod manual_non_exhaustive;
 mod manual_range_patterns;
 mod manual_rem_euclid;
 mod manual_retain;
+mod manual_rotate;
 mod manual_slice_size_calculation;
 mod manual_string_new;
 mod manual_strip;
@@ -1023,6 +1024,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
     store.register_late_pass(|_| Box::new(default_instead_of_iter_empty::DefaultIterEmpty));
     store.register_late_pass(move |_| Box::new(manual_rem_euclid::ManualRemEuclid::new(msrv())));
     store.register_late_pass(move |_| Box::new(manual_retain::ManualRetain::new(msrv())));
+    store.register_late_pass(move |_| Box::new(manual_rotate::ManualRotate));
     store.register_late_pass(move |_| {
         Box::new(operators::Operators::new(
             verbose_bit_mask_threshold,

clippy_lints/src/manual_rotate.rs

@@ -0,0 +1,117 @@
+use std::fmt::Display;
+
+use clippy_utils::consts::{constant, Constant};
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::sugg;
+use rustc_errors::Applicability;
+use rustc_hir::{BinOpKind, Expr, ExprKind};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::ty;
+use rustc_session::declare_lint_pass;
+
+declare_clippy_lint! {
+    /// ### What it does
+    ///
+    /// It detects manual bit rotations that could be rewritten using standard
+    /// functions `rotate_left` or `rotate_right`.
+    ///
+    /// ### Why is this bad?
+    ///
+    /// Calling the function better conveys the intent.
+    ///
+    /// ### Known issues
+    ///
+    /// Currently, the lint only catches shifts by constant amount.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// let x = 12345678_u32;
+    /// let _ = (x >> 8) | (x << 24);
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// let x = 12345678_u32;
+    /// let _ = x.rotate_right(8);
+    /// ```
+    #[clippy::version = "1.81.0"]
+    pub MANUAL_ROTATE,
+    style,
+    "using bit shifts to rotate integers"
+}
+
+declare_lint_pass!(ManualRotate => [MANUAL_ROTATE]);
+
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+enum ShiftDirection {
+    Left,
+    Right,
+}
+
+impl Display for ShiftDirection {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.write_str(match self {
+            Self::Left => "rotate_left",
+            Self::Right => "rotate_right",
+        })
+    }
+}
+
+fn parse_shift<'tcx>(
+    cx: &LateContext<'tcx>,
+    expr: &'tcx Expr<'tcx>,
+) -> Option<(ShiftDirection, u128, &'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 = constant(cx, cx.typeck_results(), r)?;
+        if let Constant::Int(shift) = const_expr {
+            return Some((dir, shift, l));
+        }
+    }
+    None
+}
+
+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() {
+                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 {
+                    (l_shift_dir, l_amount)
+                } else {
+                    (r_shift_dir, r_amount)
+                };
+                let mut applicability = Applicability::MachineApplicable;
+                let expr_sugg = sugg::Sugg::hir_with_applicability(cx, l_expr, "_", &mut applicability).maybe_par();
+                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

@@ -0,0 +1,31 @@
+#![warn(clippy::manual_rotate)]
+#![allow(unused)]
+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;
+    // True positives
+    let y_u8 = x_u8.rotate_right(3);
+    let y_u16 = x_u16.rotate_right(7);
+    let y_u32 = x_u32.rotate_right(8);
+    let y_u64 = x_u64.rotate_right(9);
+    let y_i8 = x_i8.rotate_right(3);
+    let y_i16 = x_i16.rotate_right(7);
+    let y_i32 = x_i32.rotate_right(8);
+    let y_i64 = x_i64.rotate_right(9);
+    // Plus also works instead of |
+    let y_u32_plus = x_u32.rotate_right(8);
+    // Complex expression
+    let y_u32_complex = (x_u32 | 3256).rotate_right(8);
+    let y_u64_as = (x_u32 as u64).rotate_right(8);
+
+    // False positives - can't be replaced with a rotation
+    let y_u8_false = (x_u8 >> 6) | (x_u8 << 3);
+    let y_u32_false = (x_u32 >> 8) | (x_u32 >> 24);
+    let y_u64_false2 = (x_u64 >> 9) & (x_u64 << 55);
+    // Variable mismatch
+    let y_u32_wrong_vars = (x_u32 >> 8) | (a_u32 << 24);
+    // Has side effects and therefore should not be matched
+    let mut l = vec![12_u8, 34];
+    let y = (l.pop().unwrap() << 3) + (l.pop().unwrap() >> 5);
+}

tests/ui/manual_rotate.rs

@@ -0,0 +1,31 @@
+#![warn(clippy::manual_rotate)]
+#![allow(unused)]
+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;
+    // True positives
+    let y_u8 = (x_u8 >> 3) | (x_u8 << 5);
+    let y_u16 = (x_u16 >> 7) | (x_u16 << 9);
+    let y_u32 = (x_u32 >> 8) | (x_u32 << 24);
+    let y_u64 = (x_u64 >> 9) | (x_u64 << 55);
+    let y_i8 = (x_i8 >> 3) | (x_i8 << 5);
+    let y_i16 = (x_i16 >> 7) | (x_i16 << 9);
+    let y_i32 = (x_i32 >> 8) | (x_i32 << 24);
+    let y_i64 = (x_i64 >> 9) | (x_i64 << 55);
+    // Plus also works instead of |
+    let y_u32_plus = (x_u32 >> 8) + (x_u32 << 24);
+    // Complex expression
+    let y_u32_complex = ((x_u32 | 3256) >> 8) | ((x_u32 | 3256) << 24);
+    let y_u64_as = (x_u32 as u64 >> 8) | ((x_u32 as u64) << 56);
+
+    // False positives - can't be replaced with a rotation
+    let y_u8_false = (x_u8 >> 6) | (x_u8 << 3);
+    let y_u32_false = (x_u32 >> 8) | (x_u32 >> 24);
+    let y_u64_false2 = (x_u64 >> 9) & (x_u64 << 55);
+    // Variable mismatch
+    let y_u32_wrong_vars = (x_u32 >> 8) | (a_u32 << 24);
+    // Has side effects and therefore should not be matched
+    let mut l = vec![12_u8, 34];
+    let y = (l.pop().unwrap() << 3) + (l.pop().unwrap() >> 5);
+}

tests/ui/manual_rotate.stderr

@@ -0,0 +1,71 @@
+error: there is no need to manually implement bit rotation
+  --> tests/ui/manual_rotate.rs:8:16
+   |
+LL |     let y_u8 = (x_u8 >> 3) | (x_u8 << 5);
+   |                ^^^^^^^^^^^^^^^^^^^^^^^^^ help: this expression can be rewritten as: `x_u8.rotate_right(3)`
+   |
+   = note: `-D clippy::manual-rotate` implied by `-D warnings`
+   = 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:9: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:10: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:11: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:12: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:13: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:14: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:15: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:17: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:19: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:20: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
+