From d7996da9dae165c7d6e700f8b734392db30f9697 Mon Sep 17 00:00:00 2001 From: Sour1emon Date: Sat, 31 Aug 2024 16:30:03 -0700 Subject: [PATCH 1/4] Add `manual_is_power_of_two` --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 2 + clippy_lints/src/manual_is_power_of_two.rs | 88 ++++++++++++++++++++++ tests/ui/manual_is_power_of_two.fixed | 14 ++++ tests/ui/manual_is_power_of_two.rs | 14 ++++ tests/ui/manual_is_power_of_two.stderr | 17 +++++ 7 files changed, 137 insertions(+) create mode 100644 clippy_lints/src/manual_is_power_of_two.rs create mode 100644 tests/ui/manual_is_power_of_two.fixed create mode 100644 tests/ui/manual_is_power_of_two.rs create mode 100644 tests/ui/manual_is_power_of_two.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 44a569d1ab54..f1de51c936ea 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5626,6 +5626,7 @@ Released 2018-09-13 [`manual_is_ascii_check`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_is_ascii_check [`manual_is_finite`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_is_finite [`manual_is_infinite`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_is_infinite +[`manual_is_power_of_two`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_is_power_of_two [`manual_is_variant_and`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_is_variant_and [`manual_let_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_let_else [`manual_main_separator_str`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_main_separator_str diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 4804399ef7da..6f468f01b2fd 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -306,6 +306,7 @@ pub static LINTS: &[&crate::LintInfo] = &[ crate::manual_float_methods::MANUAL_IS_INFINITE_INFO, crate::manual_hash_one::MANUAL_HASH_ONE_INFO, crate::manual_is_ascii_check::MANUAL_IS_ASCII_CHECK_INFO, + crate::manual_is_power_of_two::MANUAL_IS_POWER_OF_TWO_INFO, crate::manual_let_else::MANUAL_LET_ELSE_INFO, crate::manual_main_separator_str::MANUAL_MAIN_SEPARATOR_STR_INFO, crate::manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 9514ba8c315d..bc16a3b0c014 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -207,6 +207,7 @@ mod manual_div_ceil; mod manual_float_methods; mod manual_hash_one; mod manual_is_ascii_check; +mod manual_is_power_of_two; mod manual_let_else; mod manual_main_separator_str; mod manual_non_exhaustive; @@ -938,5 +939,6 @@ 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)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/manual_is_power_of_two.rs b/clippy_lints/src/manual_is_power_of_two.rs new file mode 100644 index 000000000000..a65401fc571a --- /dev/null +++ b/clippy_lints/src/manual_is_power_of_two.rs @@ -0,0 +1,88 @@ +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 rustc_errors::Applicability; +use rustc_hir::{BinOpKind, Expr, ExprKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::Uint; +use rustc_session::declare_lint_pass; + +declare_clippy_lint! { + /// ### What it does + /// Checks for expressions like `x.count_ones() == 1` or `x & (x - 1) == 0` which are manual + /// reimplementations of `x.is_power_of_two()`` + /// ### Why is this bad? + /// It's simpler and clearer + /// ### Example + /// ```no_run + /// let x: u32 = 1; + /// let result = x.count_ones() == 1; + /// ``` + /// Use instead: + /// ```no_run + /// let x: u32 = 1; + /// let result = x.is_power_of_two(); + /// ``` + #[clippy::version = "1.82.0"] + pub MANUAL_IS_POWER_OF_TWO, + complexity, + "manually reimplementing `is_power_of_two`" +} + +declare_lint_pass!(ManualIsPowerOfTwo => [MANUAL_IS_POWER_OF_TWO]); + +impl LateLintPass<'_> for ManualIsPowerOfTwo { + fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) { + let mut applicability = Applicability::MachineApplicable; + + // x.count_ones() == 1 + if let ExprKind::Binary(op, left, right) = expr.kind + && BinOpKind::Eq == op.node + && let ExprKind::MethodCall(method_name, reciever, _, _) = left.kind + && method_name.ident.as_str() == "count_ones" + && let ExprKind::Lit(lit) = right.kind + && let LitKind::Int(Pu128(1), _) = lit.node + && let &Uint(_) = cx.typeck_results().expr_ty(reciever).kind() + { + let snippet = snippet_with_applicability(cx, reciever.span, "..", &mut applicability); + let sugg = format!("{snippet}.is_power_of_two()"); + span_lint_and_sugg( + cx, + MANUAL_IS_POWER_OF_TWO, + expr.span, + "manually reimplementing `is_power_of_two`", + "consider using `.is_power_of_two()`", + sugg, + applicability, + ); + } + + // x & (x - 1) == 0 + if let ExprKind::Binary(op, left, right) = expr.kind + && BinOpKind::Eq == op.node + && let ExprKind::Binary(op1, left1, right1) = left.kind + && BinOpKind::BitAnd == op1.node + && let ExprKind::Binary(op2, left2, right2) = right1.kind + && BinOpKind::Sub == op2.node + && left1.span.eq_ctxt(left2.span) + && let &Uint(_) = cx.typeck_results().expr_ty(left1).kind() + && let ExprKind::Lit(lit) = right2.kind + && let LitKind::Int(Pu128(1), _) = lit.node + && let ExprKind::Lit(lit1) = right.kind + && let LitKind::Int(Pu128(0), _) = lit1.node + { + let snippet = snippet_with_applicability(cx, left1.span, "..", &mut applicability); + let sugg = format!("{snippet}.is_power_of_two()"); + span_lint_and_sugg( + cx, + MANUAL_IS_POWER_OF_TWO, + expr.span, + "manually reimplementing `is_power_of_two`", + "consider using `.is_power_of_two()`", + sugg, + applicability, + ); + } + } +} diff --git a/tests/ui/manual_is_power_of_two.fixed b/tests/ui/manual_is_power_of_two.fixed new file mode 100644 index 000000000000..beee2eaf665a --- /dev/null +++ b/tests/ui/manual_is_power_of_two.fixed @@ -0,0 +1,14 @@ +#![warn(clippy::manual_is_power_of_two)] + +fn main() { + let a = 16_u64; + + let _ = a.is_power_of_two(); + let _ = a.is_power_of_two(); + + let b = 4_i64; + + // is_power_of_two only works for unsigned integers + let _ = b.count_ones() == 1; + let _ = b & (b - 1) == 0; +} diff --git a/tests/ui/manual_is_power_of_two.rs b/tests/ui/manual_is_power_of_two.rs new file mode 100644 index 000000000000..0810b4c28da7 --- /dev/null +++ b/tests/ui/manual_is_power_of_two.rs @@ -0,0 +1,14 @@ +#![warn(clippy::manual_is_power_of_two)] + +fn main() { + let a = 16_u64; + + let _ = a.count_ones() == 1; + let _ = a & (a - 1) == 0; + + let b = 4_i64; + + // is_power_of_two only works for unsigned integers + let _ = b.count_ones() == 1; + let _ = b & (b - 1) == 0; +} diff --git a/tests/ui/manual_is_power_of_two.stderr b/tests/ui/manual_is_power_of_two.stderr new file mode 100644 index 000000000000..c7dfe6b11b99 --- /dev/null +++ b/tests/ui/manual_is_power_of_two.stderr @@ -0,0 +1,17 @@ +error: manually reimplementing `is_power_of_two` + --> tests/ui/manual_is_power_of_two.rs:6:13 + | +LL | let _ = a.count_ones() == 1; + | ^^^^^^^^^^^^^^^^^^^ help: consider using `.is_power_of_two()`: `a.is_power_of_two()` + | + = note: `-D clippy::manual-is-power-of-two` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::manual_is_power_of_two)]` + +error: manually reimplementing `is_power_of_two` + --> tests/ui/manual_is_power_of_two.rs:7:13 + | +LL | let _ = a & (a - 1) == 0; + | ^^^^^^^^^^^^^^^^ help: consider using `.is_power_of_two()`: `a.is_power_of_two()` + +error: aborting due to 2 previous errors + From 5515566829f3b7ba3c302c5058e986657c01cc7e Mon Sep 17 00:00:00 2001 From: Isaac Bess Date: Mon, 2 Sep 2024 17:00:32 -0700 Subject: [PATCH 2/4] Improve "Why this is bad" section Co-authored-by: Ruby Lazuli --- clippy_lints/src/manual_is_power_of_two.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/manual_is_power_of_two.rs b/clippy_lints/src/manual_is_power_of_two.rs index a65401fc571a..daa1abf61f1a 100644 --- a/clippy_lints/src/manual_is_power_of_two.rs +++ b/clippy_lints/src/manual_is_power_of_two.rs @@ -13,7 +13,7 @@ declare_clippy_lint! { /// Checks for expressions like `x.count_ones() == 1` or `x & (x - 1) == 0` which are manual /// reimplementations of `x.is_power_of_two()`` /// ### Why is this bad? - /// It's simpler and clearer + /// Manual reimplementations of `is_power_of_two` increase code complexity for little benefit. /// ### Example /// ```no_run /// let x: u32 = 1; From 6ecb48f6d0becb8ff365b787106a8be429b4637a Mon Sep 17 00:00:00 2001 From: Isaac Bess Date: Mon, 2 Sep 2024 17:01:20 -0700 Subject: [PATCH 3/4] Fixed extra backtick Co-authored-by: Ruby Lazuli --- clippy_lints/src/manual_is_power_of_two.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/manual_is_power_of_two.rs b/clippy_lints/src/manual_is_power_of_two.rs index daa1abf61f1a..c7ac668267af 100644 --- a/clippy_lints/src/manual_is_power_of_two.rs +++ b/clippy_lints/src/manual_is_power_of_two.rs @@ -10,8 +10,8 @@ use rustc_session::declare_lint_pass; declare_clippy_lint! { /// ### What it does - /// Checks for expressions like `x.count_ones() == 1` or `x & (x - 1) == 0` which are manual - /// reimplementations of `x.is_power_of_two()`` + /// Checks for expressions like `x.count_ones() == 1` or `x & (x - 1) == 0`, which are manual + /// reimplementations of `x.is_power_of_two()`. /// ### Why is this bad? /// Manual reimplementations of `is_power_of_two` increase code complexity for little benefit. /// ### Example From f994797e4612f97a0f489e794f30c2113ce8d61e Mon Sep 17 00:00:00 2001 From: Sour1emon Date: Thu, 5 Sep 2024 18:23:00 -0700 Subject: [PATCH 4/4] Add support for different orders of expression --- clippy_lints/src/manual_is_power_of_two.rs | 154 ++++++++++++++------- tests/ui/manual_is_power_of_two.fixed | 6 + tests/ui/manual_is_power_of_two.rs | 6 + tests/ui/manual_is_power_of_two.stderr | 26 +++- 4 files changed, 141 insertions(+), 51 deletions(-) diff --git a/clippy_lints/src/manual_is_power_of_two.rs b/clippy_lints/src/manual_is_power_of_two.rs index c7ac668267af..d7879403ebb5 100644 --- a/clippy_lints/src/manual_is_power_of_two.rs +++ b/clippy_lints/src/manual_is_power_of_two.rs @@ -1,5 +1,6 @@ use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::source::snippet_with_applicability; +use clippy_utils::SpanlessEq; use rustc_ast::LitKind; use rustc_data_structures::packed::Pu128; use rustc_errors::Applicability; @@ -10,19 +11,19 @@ use rustc_session::declare_lint_pass; declare_clippy_lint! { /// ### What it does - /// Checks for expressions like `x.count_ones() == 1` or `x & (x - 1) == 0`, which are manual + /// Checks for expressions like `x.count_ones() == 1` or `x & (x - 1) == 0`, with x and unsigned integer, which are manual /// reimplementations of `x.is_power_of_two()`. /// ### Why is this bad? /// Manual reimplementations of `is_power_of_two` increase code complexity for little benefit. /// ### Example /// ```no_run - /// let x: u32 = 1; - /// let result = x.count_ones() == 1; + /// let a: u32 = 4; + /// let result = a.count_ones() == 1; /// ``` /// Use instead: /// ```no_run - /// let x: u32 = 1; - /// let result = x.is_power_of_two(); + /// let a: u32 = 4; + /// let result = a.is_power_of_two(); /// ``` #[clippy::version = "1.82.0"] pub MANUAL_IS_POWER_OF_TWO, @@ -36,53 +37,106 @@ impl LateLintPass<'_> for ManualIsPowerOfTwo { fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) { let mut applicability = Applicability::MachineApplicable; - // x.count_ones() == 1 - if let ExprKind::Binary(op, left, right) = expr.kind - && BinOpKind::Eq == op.node - && let ExprKind::MethodCall(method_name, reciever, _, _) = left.kind - && method_name.ident.as_str() == "count_ones" - && let ExprKind::Lit(lit) = right.kind - && let LitKind::Int(Pu128(1), _) = lit.node - && let &Uint(_) = cx.typeck_results().expr_ty(reciever).kind() + if let ExprKind::Binary(bin_op, left, right) = expr.kind + && bin_op.node == BinOpKind::Eq { - let snippet = snippet_with_applicability(cx, reciever.span, "..", &mut applicability); - let sugg = format!("{snippet}.is_power_of_two()"); - span_lint_and_sugg( - cx, - MANUAL_IS_POWER_OF_TWO, - expr.span, - "manually reimplementing `is_power_of_two`", - "consider using `.is_power_of_two()`", - sugg, - applicability, - ); - } + // a.count_ones() == 1 + if let ExprKind::MethodCall(method_name, reciever, _, _) = left.kind + && method_name.ident.as_str() == "count_ones" + && let &Uint(_) = cx.typeck_results().expr_ty(reciever).kind() + && check_lit(right, 1) + { + build_sugg(cx, expr, reciever, &mut applicability); + } - // x & (x - 1) == 0 - if let ExprKind::Binary(op, left, right) = expr.kind - && BinOpKind::Eq == op.node - && let ExprKind::Binary(op1, left1, right1) = left.kind - && BinOpKind::BitAnd == op1.node - && let ExprKind::Binary(op2, left2, right2) = right1.kind - && BinOpKind::Sub == op2.node - && left1.span.eq_ctxt(left2.span) - && let &Uint(_) = cx.typeck_results().expr_ty(left1).kind() - && let ExprKind::Lit(lit) = right2.kind - && let LitKind::Int(Pu128(1), _) = lit.node - && let ExprKind::Lit(lit1) = right.kind - && let LitKind::Int(Pu128(0), _) = lit1.node - { - let snippet = snippet_with_applicability(cx, left1.span, "..", &mut applicability); - let sugg = format!("{snippet}.is_power_of_two()"); - span_lint_and_sugg( - cx, - MANUAL_IS_POWER_OF_TWO, - expr.span, - "manually reimplementing `is_power_of_two`", - "consider using `.is_power_of_two()`", - sugg, - applicability, - ); + // 1 == a.count_ones() + if let ExprKind::MethodCall(method_name, reciever, _, _) = right.kind + && method_name.ident.as_str() == "count_ones" + && let &Uint(_) = cx.typeck_results().expr_ty(reciever).kind() + && check_lit(left, 1) + { + build_sugg(cx, expr, reciever, &mut applicability); + } + + // 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); + } + + // (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); + } + + // 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) + { + 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) + { + build_sugg(cx, expr, right1, &mut applicability); + } } } } + +fn build_sugg(cx: &LateContext<'_>, expr: &Expr<'_>, reciever: &Expr<'_>, applicability: &mut Applicability) { + let snippet = snippet_with_applicability(cx, reciever.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, + ); +} + +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 true; + } + false +} + +fn check_eq_expr(cx: &LateContext<'_>, lhs: &Expr<'_>, rhs: &Expr<'_>) -> bool { + SpanlessEq::new(cx).eq_expr(lhs, rhs) +} diff --git a/tests/ui/manual_is_power_of_two.fixed b/tests/ui/manual_is_power_of_two.fixed index beee2eaf665a..dda5fe0ec3e7 100644 --- a/tests/ui/manual_is_power_of_two.fixed +++ b/tests/ui/manual_is_power_of_two.fixed @@ -6,6 +6,12 @@ fn main() { let _ = a.is_power_of_two(); let _ = a.is_power_of_two(); + // Test different orders of expression + let _ = a.is_power_of_two(); + let _ = a.is_power_of_two(); + let _ = a.is_power_of_two(); + let _ = a.is_power_of_two(); + let b = 4_i64; // is_power_of_two only works for unsigned integers diff --git a/tests/ui/manual_is_power_of_two.rs b/tests/ui/manual_is_power_of_two.rs index 0810b4c28da7..a1d3a95c4102 100644 --- a/tests/ui/manual_is_power_of_two.rs +++ b/tests/ui/manual_is_power_of_two.rs @@ -6,6 +6,12 @@ fn main() { let _ = a.count_ones() == 1; let _ = a & (a - 1) == 0; + // Test different orders of expression + let _ = 1 == a.count_ones(); + let _ = (a - 1) & a == 0; + let _ = 0 == a & (a - 1); + let _ = 0 == (a - 1) & a; + let b = 4_i64; // is_power_of_two only works for unsigned integers diff --git a/tests/ui/manual_is_power_of_two.stderr b/tests/ui/manual_is_power_of_two.stderr index c7dfe6b11b99..3cfc6297abf2 100644 --- a/tests/ui/manual_is_power_of_two.stderr +++ b/tests/ui/manual_is_power_of_two.stderr @@ -13,5 +13,29 @@ error: manually reimplementing `is_power_of_two` LL | let _ = a & (a - 1) == 0; | ^^^^^^^^^^^^^^^^ help: consider using `.is_power_of_two()`: `a.is_power_of_two()` -error: aborting due to 2 previous errors +error: manually reimplementing `is_power_of_two` + --> tests/ui/manual_is_power_of_two.rs:10:13 + | +LL | let _ = 1 == a.count_ones(); + | ^^^^^^^^^^^^^^^^^^^ help: consider using `.is_power_of_two()`: `a.is_power_of_two()` + +error: manually reimplementing `is_power_of_two` + --> tests/ui/manual_is_power_of_two.rs:11:13 + | +LL | let _ = (a - 1) & a == 0; + | ^^^^^^^^^^^^^^^^ help: consider using `.is_power_of_two()`: `a.is_power_of_two()` + +error: manually reimplementing `is_power_of_two` + --> tests/ui/manual_is_power_of_two.rs:12:13 + | +LL | let _ = 0 == a & (a - 1); + | ^^^^^^^^^^^^^^^^ help: consider using `.is_power_of_two()`: `a.is_power_of_two()` + +error: manually reimplementing `is_power_of_two` + --> tests/ui/manual_is_power_of_two.rs:13:13 + | +LL | let _ = 0 == (a - 1) & a; + | ^^^^^^^^^^^^^^^^ help: consider using `.is_power_of_two()`: `a.is_power_of_two()` + +error: aborting due to 6 previous errors