Various fixes for manual_is_power_of_two (#14463)

Fix #14461:

- insert parentheses as required in suggestion
- check MSRV before suggesting fix in `const` context
- do not lint macro expansion result

Commits have been logically separated to facilitate review, and start
with a refactoring (and simplification) of the existing code.

changelog: [`manual_is_power_of_two`]: insert parentheses as required in
suggestion, check MSRV before suggesting fix in `const` context, do not
lint macro expansion results

Author: blyxyas

book/src/lint_configuration.md

@@ -805,6 +805,7 @@ The minimum rust version that the project supports. Defaults to the `rust-versio
 * [`manual_flatten`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_flatten)
 * [`manual_hash_one`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_hash_one)
 * [`manual_is_ascii_check`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_is_ascii_check)
+* [`manual_is_power_of_two`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_is_power_of_two)
 * [`manual_let_else`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_let_else)
 * [`manual_midpoint`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_midpoint)
 * [`manual_non_exhaustive`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_non_exhaustive)

clippy_config/src/conf.rs

@@ -722,6 +722,7 @@ define_Conf! {
         manual_flatten,
         manual_hash_one,
         manual_is_ascii_check,
+        manual_is_power_of_two,
         manual_let_else,
         manual_midpoint,
         manual_non_exhaustive,

clippy_lints/src/lib.rs

@@ -971,7 +971,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
     store.register_late_pass(|_| Box::new(zombie_processes::ZombieProcesses));
     store.register_late_pass(|_| Box::new(pointers_in_nomem_asm_block::PointersInNomemAsmBlock));
     store.register_late_pass(move |_| Box::new(manual_div_ceil::ManualDivCeil::new(conf)));
-    store.register_late_pass(|_| Box::new(manual_is_power_of_two::ManualIsPowerOfTwo));
+    store.register_late_pass(move |_| Box::new(manual_is_power_of_two::ManualIsPowerOfTwo::new(conf)));
     store.register_late_pass(|_| Box::new(non_zero_suggestions::NonZeroSuggestions));
     store.register_late_pass(|_| Box::new(literal_string_with_formatting_args::LiteralStringWithFormattingArg));
     store.register_late_pass(move |_| Box::new(unused_trait_names::UnusedTraitNames::new(conf)));

clippy_lints/src/manual_is_power_of_two.rs

@@ -1,13 +1,14 @@
-use clippy_utils::SpanlessEq;
+use clippy_config::Conf;
 use clippy_utils::diagnostics::span_lint_and_sugg;
-use clippy_utils::source::snippet_with_applicability;
-use rustc_ast::LitKind;
-use rustc_data_structures::packed::Pu128;
+use clippy_utils::msrvs::{self, Msrv};
+use clippy_utils::sugg::Sugg;
+use clippy_utils::ty::ty_from_hir_ty;
+use clippy_utils::{SpanlessEq, is_in_const_context, is_integer_literal};
 use rustc_errors::Applicability;
-use rustc_hir::{BinOpKind, Expr, ExprKind};
+use rustc_hir::{BinOpKind, Expr, ExprKind, QPath};
 use rustc_lint::{LateContext, LateLintPass};
-use rustc_middle::ty::Uint;
-use rustc_session::declare_lint_pass;
+use rustc_middle::ty;
+use rustc_session::impl_lint_pass;
 
 declare_clippy_lint! {
     /// ### What it does
@@ -33,112 +34,111 @@ declare_clippy_lint! {
     "manually reimplementing `is_power_of_two`"
 }
 
-declare_lint_pass!(ManualIsPowerOfTwo => [MANUAL_IS_POWER_OF_TWO]);
+pub struct ManualIsPowerOfTwo {
+    msrv: Msrv,
+}
 
-impl LateLintPass<'_> for ManualIsPowerOfTwo {
-    fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) {
-        let mut applicability = Applicability::MachineApplicable;
+impl_lint_pass!(ManualIsPowerOfTwo => [MANUAL_IS_POWER_OF_TWO]);
 
-        if let ExprKind::Binary(bin_op, left, right) = expr.kind
-            && bin_op.node == BinOpKind::Eq
-        {
-            // a.count_ones() == 1
-            if let ExprKind::MethodCall(method_name, receiver, [], _) = left.kind
-                && method_name.ident.as_str() == "count_ones"
-                && let &Uint(_) = cx.typeck_results().expr_ty(receiver).kind()
-                && check_lit(right, 1)
-            {
-                build_sugg(cx, expr, receiver, &mut applicability);
-            }
+impl ManualIsPowerOfTwo {
+    pub fn new(conf: &'static Conf) -> Self {
+        Self { msrv: conf.msrv }
+    }
 
-            // 1 == a.count_ones()
-            if let ExprKind::MethodCall(method_name, receiver, [], _) = right.kind
-                && method_name.ident.as_str() == "count_ones"
-                && let &Uint(_) = cx.typeck_results().expr_ty(receiver).kind()
-                && check_lit(left, 1)
-            {
-                build_sugg(cx, expr, receiver, &mut applicability);
-            }
+    fn build_sugg(&self, cx: &LateContext<'_>, expr: &Expr<'_>, receiver: &Expr<'_>) {
+        if is_in_const_context(cx) && !self.msrv.meets(cx, msrvs::CONST_IS_POWER_OF_TWO) {
+            return;
+        }
 
-            // a & (a - 1) == 0
-            if let ExprKind::Binary(op1, left1, right1) = left.kind
-                && op1.node == BinOpKind::BitAnd
-                && let ExprKind::Binary(op2, left2, right2) = right1.kind
-                && op2.node == BinOpKind::Sub
-                && check_eq_expr(cx, left1, left2)
-                && let &Uint(_) = cx.typeck_results().expr_ty(left1).kind()
-                && check_lit(right2, 1)
-                && check_lit(right, 0)
-            {
-                build_sugg(cx, expr, left1, &mut applicability);
-            }
+        let mut applicability = Applicability::MachineApplicable;
+        let snippet = Sugg::hir_with_applicability(cx, receiver, "_", &mut applicability);
 
-            // (a - 1) & a == 0;
-            if let ExprKind::Binary(op1, left1, right1) = left.kind
-                && op1.node == BinOpKind::BitAnd
-                && let ExprKind::Binary(op2, left2, right2) = left1.kind
-                && op2.node == BinOpKind::Sub
-                && check_eq_expr(cx, right1, left2)
-                && let &Uint(_) = cx.typeck_results().expr_ty(right1).kind()
-                && check_lit(right2, 1)
-                && check_lit(right, 0)
-            {
-                build_sugg(cx, expr, right1, &mut applicability);
-            }
+        span_lint_and_sugg(
+            cx,
+            MANUAL_IS_POWER_OF_TWO,
+            expr.span,
+            "manually reimplementing `is_power_of_two`",
+            "consider using `.is_power_of_two()`",
+            format!("{}.is_power_of_two()", snippet.maybe_paren()),
+            applicability,
+        );
+    }
+}
 
-            // 0 == a & (a - 1);
-            if let ExprKind::Binary(op1, left1, right1) = right.kind
-                && op1.node == BinOpKind::BitAnd
-                && let ExprKind::Binary(op2, left2, right2) = right1.kind
-                && op2.node == BinOpKind::Sub
-                && check_eq_expr(cx, left1, left2)
-                && let &Uint(_) = cx.typeck_results().expr_ty(left1).kind()
-                && check_lit(right2, 1)
-                && check_lit(left, 0)
+impl<'tcx> LateLintPass<'tcx> for ManualIsPowerOfTwo {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
+        if !expr.span.from_expansion()
+            && let Some((lhs, rhs)) = unexpanded_binop_operands(expr, BinOpKind::Eq)
+        {
+            if let Some(a) = count_ones_receiver(cx, lhs)
+                && is_integer_literal(rhs, 1)
             {
-                build_sugg(cx, expr, left1, &mut applicability);
-            }
-
-            // 0 == (a - 1) & a
-            if let ExprKind::Binary(op1, left1, right1) = right.kind
-                && op1.node == BinOpKind::BitAnd
-                && let ExprKind::Binary(op2, left2, right2) = left1.kind
-                && op2.node == BinOpKind::Sub
-                && check_eq_expr(cx, right1, left2)
-                && let &Uint(_) = cx.typeck_results().expr_ty(right1).kind()
-                && check_lit(right2, 1)
-                && check_lit(left, 0)
+                self.build_sugg(cx, expr, a);
+            } else if let Some(a) = count_ones_receiver(cx, rhs)
+                && is_integer_literal(lhs, 1)
+            {
+                self.build_sugg(cx, expr, a);
+            } else if is_integer_literal(rhs, 0)
+                && let Some(a) = is_and_minus_one(cx, lhs)
+            {
+                self.build_sugg(cx, expr, a);
+            } else if is_integer_literal(lhs, 0)
+                && let Some(a) = is_and_minus_one(cx, rhs)
             {
-                build_sugg(cx, expr, right1, &mut applicability);
+                self.build_sugg(cx, expr, a);
             }
         }
     }
 }
 
-fn build_sugg(cx: &LateContext<'_>, expr: &Expr<'_>, receiver: &Expr<'_>, applicability: &mut Applicability) {
-    let snippet = snippet_with_applicability(cx, receiver.span, "..", applicability);
-
-    span_lint_and_sugg(
-        cx,
-        MANUAL_IS_POWER_OF_TWO,
-        expr.span,
-        "manually reimplementing `is_power_of_two`",
-        "consider using `.is_power_of_two()`",
-        format!("{snippet}.is_power_of_two()"),
-        *applicability,
-    );
+/// Return the unsigned integer receiver of `.count_ones()` or the argument of
+/// `<int-type>::count_ones(…)`.
+fn count_ones_receiver<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) -> Option<&'tcx Expr<'tcx>> {
+    let (method, ty, receiver) = if let ExprKind::MethodCall(method_name, receiver, [], _) = expr.kind {
+        (method_name, cx.typeck_results().expr_ty_adjusted(receiver), receiver)
+    } else if let ExprKind::Call(func, [arg]) = expr.kind
+        && let ExprKind::Path(QPath::TypeRelative(ty, func_name)) = func.kind
+    {
+        (func_name, ty_from_hir_ty(cx, ty), arg)
+    } else {
+        return None;
+    };
+    (method.ident.as_str() == "count_ones" && matches!(ty.kind(), ty::Uint(_))).then_some(receiver)
 }
 
-fn check_lit(expr: &Expr<'_>, expected_num: u128) -> bool {
-    if let ExprKind::Lit(lit) = expr.kind
-        && let LitKind::Int(Pu128(num), _) = lit.node
-        && num == expected_num
+/// Return `greater` if `smaller == greater - 1`
+fn is_one_less<'tcx>(
+    cx: &LateContext<'tcx>,
+    greater: &'tcx Expr<'tcx>,
+    smaller: &Expr<'tcx>,
+) -> Option<&'tcx Expr<'tcx>> {
+    if let Some((lhs, rhs)) = unexpanded_binop_operands(smaller, BinOpKind::Sub)
+        && SpanlessEq::new(cx).eq_expr(greater, lhs)
+        && is_integer_literal(rhs, 1)
+        && matches!(cx.typeck_results().expr_ty_adjusted(greater).kind(), ty::Uint(_))
     {
-        return true;
+        Some(greater)
+    } else {
+        None
     }
-    false
 }
 
-fn check_eq_expr(cx: &LateContext<'_>, lhs: &Expr<'_>, rhs: &Expr<'_>) -> bool {
-    SpanlessEq::new(cx).eq_expr(lhs, rhs)
+/// Return `v` if `expr` is `v & (v - 1)` or `(v - 1) & v`
+fn is_and_minus_one<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) -> Option<&'tcx Expr<'tcx>> {
+    let (lhs, rhs) = unexpanded_binop_operands(expr, BinOpKind::BitAnd)?;
+    is_one_less(cx, lhs, rhs).or_else(|| is_one_less(cx, rhs, lhs))
+}
+
+/// Return the operands of the `expr` binary operation if the operator is `op` and none of the
+/// operands come from expansion.
+fn unexpanded_binop_operands<'hir>(expr: &Expr<'hir>, op: BinOpKind) -> Option<(&'hir Expr<'hir>, &'hir Expr<'hir>)> {
+    if let ExprKind::Binary(binop, lhs, rhs) = expr.kind
+        && binop.node == op
+        && !lhs.span.from_expansion()
+        && !rhs.span.from_expansion()
+    {
+        Some((lhs, rhs))
+    } else {
+        None
+    }
 }

clippy_utils/src/msrvs.rs

@@ -64,6 +64,7 @@ msrv_aliases! {
     1,35,0 { OPTION_COPIED, RANGE_CONTAINS }
     1,34,0 { TRY_FROM }
     1,33,0 { UNDERSCORE_IMPORTS }
+    1,32,0 { CONST_IS_POWER_OF_TWO }
     1,31,0 { OPTION_REPLACE }
     1,30,0 { ITERATOR_FIND_MAP, TOOL_ATTRIBUTES }
     1,29,0 { ITER_FLATTEN }

tests/ui/manual_is_power_of_two.fixed

@@ -1,4 +1,17 @@
 #![warn(clippy::manual_is_power_of_two)]
+#![allow(clippy::precedence)]
+
+macro_rules! binop {
+    ($a: expr, equal, $b: expr) => {
+        $a == $b
+    };
+    ($a: expr, and, $b: expr) => {
+        $a & $b
+    };
+    ($a: expr, minus, $b: expr) => {
+        $a - $b
+    };
+}
 
 fn main() {
     let a = 16_u64;
@@ -7,6 +20,8 @@ fn main() {
     //~^ manual_is_power_of_two
     let _ = a.is_power_of_two();
     //~^ manual_is_power_of_two
+    let _ = a.is_power_of_two();
+    //~^ manual_is_power_of_two
 
     // Test different orders of expression
     let _ = a.is_power_of_two();
@@ -23,4 +38,23 @@ fn main() {
     // is_power_of_two only works for unsigned integers
     let _ = b.count_ones() == 1;
     let _ = b & (b - 1) == 0;
+
+    let i: i32 = 3;
+    let _ = (i as u32).is_power_of_two();
+    //~^ manual_is_power_of_two
+
+    let _ = binop!(a.count_ones(), equal, 1);
+    let _ = binop!(a, and, a - 1) == 0;
+    let _ = a & binop!(a, minus, 1) == 0;
+}
+
+#[clippy::msrv = "1.31"]
+const fn low_msrv(a: u32) -> bool {
+    a & (a - 1) == 0
+}
+
+#[clippy::msrv = "1.32"]
+const fn high_msrv(a: u32) -> bool {
+    a.is_power_of_two()
+    //~^ manual_is_power_of_two
 }

tests/ui/manual_is_power_of_two.rs

@@ -1,10 +1,25 @@
 #![warn(clippy::manual_is_power_of_two)]
+#![allow(clippy::precedence)]
+
+macro_rules! binop {
+    ($a: expr, equal, $b: expr) => {
+        $a == $b
+    };
+    ($a: expr, and, $b: expr) => {
+        $a & $b
+    };
+    ($a: expr, minus, $b: expr) => {
+        $a - $b
+    };
+}
 
 fn main() {
     let a = 16_u64;
 
     let _ = a.count_ones() == 1;
     //~^ manual_is_power_of_two
+    let _ = u64::count_ones(a) == 1;
+    //~^ manual_is_power_of_two
     let _ = a & (a - 1) == 0;
     //~^ manual_is_power_of_two
 
@@ -23,4 +38,23 @@ fn main() {
     // is_power_of_two only works for unsigned integers
     let _ = b.count_ones() == 1;
     let _ = b & (b - 1) == 0;
+
+    let i: i32 = 3;
+    let _ = i as u32 & (i as u32 - 1) == 0;
+    //~^ manual_is_power_of_two
+
+    let _ = binop!(a.count_ones(), equal, 1);
+    let _ = binop!(a, and, a - 1) == 0;
+    let _ = a & binop!(a, minus, 1) == 0;
+}
+
+#[clippy::msrv = "1.31"]
+const fn low_msrv(a: u32) -> bool {
+    a & (a - 1) == 0
+}
+
+#[clippy::msrv = "1.32"]
+const fn high_msrv(a: u32) -> bool {
+    a & (a - 1) == 0
+    //~^ manual_is_power_of_two
 }