From b97f73184212d9e636b4d4057bf3c9c8bdd64e08 Mon Sep 17 00:00:00 2001 From: Sour1emon Date: Sun, 1 Sep 2024 11:33:04 -0700 Subject: [PATCH 1/6] Add [`manual_ilog2`] lint --- CHANGELOG.md | 1 + clippy_config/src/msrvs.rs | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 2 + clippy_lints/src/manual_ilog2.rs | 105 +++++++++++++++++++++++++++++ tests/ui/manual_ilog2.fixed | 12 ++++ tests/ui/manual_ilog2.rs | 12 ++++ tests/ui/manual_ilog2.stderr | 23 +++++++ 8 files changed, 157 insertions(+) create mode 100644 clippy_lints/src/manual_ilog2.rs create mode 100644 tests/ui/manual_ilog2.fixed create mode 100644 tests/ui/manual_ilog2.rs create mode 100644 tests/ui/manual_ilog2.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 41a86e8ce510..c79345890df4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5621,6 +5621,7 @@ Released 2018-09-13 [`manual_find_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_find_map [`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_ilog2`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_ilog2 [`manual_inspect`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_inspect [`manual_instant_elapsed`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_instant_elapsed [`manual_is_ascii_check`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_is_ascii_check diff --git a/clippy_config/src/msrvs.rs b/clippy_config/src/msrvs.rs index e30df3d32341..8045e8919168 100644 --- a/clippy_config/src/msrvs.rs +++ b/clippy_config/src/msrvs.rs @@ -27,6 +27,7 @@ msrv_aliases! { 1,71,0 { TUPLE_ARRAY_CONVERSIONS, BUILD_HASHER_HASH_ONE } 1,70,0 { OPTION_RESULT_IS_VARIANT_AND, BINARY_HEAP_RETAIN } 1,68,0 { PATH_MAIN_SEPARATOR_STR } + 1,67,0 { MANUAL_ILOG2 } 1,65,0 { LET_ELSE, POINTER_CAST_CONSTNESS } 1,63,0 { CLONE_INTO } 1,62,0 { BOOL_THEN_SOME, DEFAULT_ENUM_ATTRIBUTE, CONST_EXTERN_C_FN } diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 2b229d2fe6ac..34a7fc572fcd 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -305,6 +305,7 @@ pub static LINTS: &[&crate::LintInfo] = &[ crate::manual_float_methods::MANUAL_IS_FINITE_INFO, crate::manual_float_methods::MANUAL_IS_INFINITE_INFO, crate::manual_hash_one::MANUAL_HASH_ONE_INFO, + crate::manual_ilog2::MANUAL_ILOG2_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, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 1d41f568f378..e9160d813767 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -206,6 +206,7 @@ mod manual_clamp; mod manual_div_ceil; mod manual_float_methods; mod manual_hash_one; +mod manual_ilog2; mod manual_is_ascii_check; mod manual_is_power_of_two; mod manual_let_else; @@ -944,5 +945,6 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { store.register_late_pass(|_| Box::new(manual_is_power_of_two::ManualIsPowerOfTwo)); store.register_late_pass(|_| Box::new(non_zero_suggestions::NonZeroSuggestions)); store.register_late_pass(move |_| Box::new(unused_trait_names::UnusedTraitNames::new(conf))); + store.register_late_pass(move |_| Box::new(manual_ilog2::ManualIlog2::new(conf))); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/manual_ilog2.rs b/clippy_lints/src/manual_ilog2.rs new file mode 100644 index 000000000000..56e15fb30eaa --- /dev/null +++ b/clippy_lints/src/manual_ilog2.rs @@ -0,0 +1,105 @@ +use clippy_config::msrvs::{self, Msrv}; +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 rustc_errors::Applicability; +use rustc_hir::{BinOpKind, Expr, ExprKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty; +use rustc_session::impl_lint_pass; + +declare_clippy_lint! { + /// ### What it does + /// Checks for expressions like `31 - x.leading_zeros()` or `x.ilog(2)` which are manual + /// reimplementations of `x.ilog2()` + /// ### Why is this bad? + /// It is easier to read and understand + /// ### Example + /// ```no_run + /// let x: u32 = 5; + /// let log = 31 - x.leading_zeros() + /// ``` + /// Use instead: + /// ```no_run + /// let x: u32 = 5; + /// let log = x.ilog2() + /// ``` + #[clippy::version = "1.82.0"] + pub MANUAL_ILOG2, + complexity, + "manually reimplementing `ilog2`" +} + +pub struct ManualIlog2 { + msrv: Msrv, +} + +impl ManualIlog2 { + #[must_use] + pub fn new(conf: &Conf) -> Self { + Self { + msrv: conf.msrv.clone(), + } + } +} + +impl_lint_pass!(ManualIlog2 => [MANUAL_ILOG2]); + +impl LateLintPass<'_> for ManualIlog2 { + fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) { + if !self.msrv.meets(msrvs::MANUAL_ILOG2) { + return; + } + let mut applicability = Applicability::MachineApplicable; + + if let ExprKind::Binary(op, left, right) = expr.kind + && BinOpKind::Sub == op.node + && let ExprKind::Lit(lit) = left.kind + && let LitKind::Int(Pu128(val), _) = lit.node + && let ExprKind::MethodCall(method_name, reciever, _, _) = right.kind + && method_name.ident.as_str() == "leading_zeros" + { + let type_ = cx.typeck_results().expr_ty(reciever); + let Some(bit_width) = (match type_.kind() { + ty::Int(itype) => itype.bit_width(), + ty::Uint(itype) => itype.bit_width(), + _ => return, + }) else { + return; + }; + if val == u128::from(bit_width) - 1 { + let sugg = snippet_with_applicability(cx, reciever.span, "..", &mut applicability); + span_lint_and_sugg( + cx, + MANUAL_ILOG2, + expr.span, + "manually reimplementing `ilog2`", + "consider using .ilog2()", + format!("{sugg}.ilog2()"), + applicability, + ); + } + } + + if let ExprKind::MethodCall(method_name, reciever, args, _) = expr.kind + && method_name.ident.as_str() == "ilog" + && args.len() == 1 + && let ExprKind::Lit(lit) = args[0].kind + && let LitKind::Int(Pu128(2), _) = lit.node + && cx.typeck_results().expr_ty(reciever).is_integral() + { + let sugg = snippet_with_applicability(cx, reciever.span, "..", &mut applicability); + span_lint_and_sugg( + cx, + MANUAL_ILOG2, + expr.span, + "manually reimplementing `ilog2`", + "consider using .ilog2()", + format!("{sugg}.ilog2()"), + applicability, + ); + } + } +} diff --git a/tests/ui/manual_ilog2.fixed b/tests/ui/manual_ilog2.fixed new file mode 100644 index 000000000000..865009db8d79 --- /dev/null +++ b/tests/ui/manual_ilog2.fixed @@ -0,0 +1,12 @@ +#![warn(clippy::manual_ilog2)] + +fn main() { + let a: u32 = 5; + let _ = a.ilog2(); + let _ = a.ilog2(); + + let b: u64 = 543534; + let _ = b.ilog2(); + + let _ = 64 - b.leading_zeros(); // No lint +} diff --git a/tests/ui/manual_ilog2.rs b/tests/ui/manual_ilog2.rs new file mode 100644 index 000000000000..0bf926b9d5b7 --- /dev/null +++ b/tests/ui/manual_ilog2.rs @@ -0,0 +1,12 @@ +#![warn(clippy::manual_ilog2)] + +fn main() { + let a: u32 = 5; + let _ = 31 - a.leading_zeros(); + let _ = a.ilog(2); + + let b: u64 = 543534; + let _ = 63 - b.leading_zeros(); + + let _ = 64 - b.leading_zeros(); // No lint +} diff --git a/tests/ui/manual_ilog2.stderr b/tests/ui/manual_ilog2.stderr new file mode 100644 index 000000000000..88abb3ce9efd --- /dev/null +++ b/tests/ui/manual_ilog2.stderr @@ -0,0 +1,23 @@ +error: manually reimplementing `ilog2` + --> tests/ui/manual_ilog2.rs:5:13 + | +LL | let _ = 31 - a.leading_zeros(); + | ^^^^^^^^^^^^^^^^^^^^^^ help: consider using .ilog2(): `a.ilog2()` + | + = note: `-D clippy::manual-ilog2` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::manual_ilog2)]` + +error: manually reimplementing `ilog2` + --> tests/ui/manual_ilog2.rs:6:13 + | +LL | let _ = a.ilog(2); + | ^^^^^^^^^ help: consider using .ilog2(): `a.ilog2()` + +error: manually reimplementing `ilog2` + --> tests/ui/manual_ilog2.rs:9:13 + | +LL | let _ = 63 - b.leading_zeros(); + | ^^^^^^^^^^^^^^^^^^^^^^ help: consider using .ilog2(): `b.ilog2()` + +error: aborting due to 3 previous errors + From 3781026760b8123af3e8ccf165d7464a322bad26 Mon Sep 17 00:00:00 2001 From: Sour1emon Date: Mon, 2 Sep 2024 13:29:21 -0700 Subject: [PATCH 2/6] Added extract_msrv_attr macro to manual_ilog2.rs --- clippy_lints/src/manual_ilog2.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/clippy_lints/src/manual_ilog2.rs b/clippy_lints/src/manual_ilog2.rs index 56e15fb30eaa..d7a761cdf430 100644 --- a/clippy_lints/src/manual_ilog2.rs +++ b/clippy_lints/src/manual_ilog2.rs @@ -102,4 +102,6 @@ impl LateLintPass<'_> for ManualIlog2 { ); } } + + extract_msrv_attr!(LateContext); } From 40cd0f86ae68c3d5933aed71fcde4431f0602139 Mon Sep 17 00:00:00 2001 From: Sour1emon Date: Mon, 2 Sep 2024 14:54:14 -0700 Subject: [PATCH 3/6] Add semicolons --- clippy_lints/src/manual_ilog2.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/manual_ilog2.rs b/clippy_lints/src/manual_ilog2.rs index d7a761cdf430..ec79b2599dce 100644 --- a/clippy_lints/src/manual_ilog2.rs +++ b/clippy_lints/src/manual_ilog2.rs @@ -19,12 +19,12 @@ declare_clippy_lint! { /// ### Example /// ```no_run /// let x: u32 = 5; - /// let log = 31 - x.leading_zeros() + /// let log = 31 - x.leading_zeros(); /// ``` /// Use instead: /// ```no_run /// let x: u32 = 5; - /// let log = x.ilog2() + /// let log = x.ilog2(); /// ``` #[clippy::version = "1.82.0"] pub MANUAL_ILOG2, From a9a385c668530bfb957dbb8d69ce4d872e29bc09 Mon Sep 17 00:00:00 2001 From: Isaac Bess Date: Mon, 2 Sep 2024 17:12:39 -0700 Subject: [PATCH 4/6] Improve "Why is this bad" section and inline `sugg` variable --- clippy_lints/src/manual_ilog2.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/manual_ilog2.rs b/clippy_lints/src/manual_ilog2.rs index ec79b2599dce..06a371026c6b 100644 --- a/clippy_lints/src/manual_ilog2.rs +++ b/clippy_lints/src/manual_ilog2.rs @@ -15,7 +15,7 @@ declare_clippy_lint! { /// Checks for expressions like `31 - x.leading_zeros()` or `x.ilog(2)` which are manual /// reimplementations of `x.ilog2()` /// ### Why is this bad? - /// It is easier to read and understand + /// Manual reimplementations of `ilog2` increase code complexity for little benefit. /// ### Example /// ```no_run /// let x: u32 = 5; @@ -90,14 +90,13 @@ impl LateLintPass<'_> for ManualIlog2 { && let LitKind::Int(Pu128(2), _) = lit.node && cx.typeck_results().expr_ty(reciever).is_integral() { - let sugg = snippet_with_applicability(cx, reciever.span, "..", &mut applicability); span_lint_and_sugg( cx, MANUAL_ILOG2, expr.span, "manually reimplementing `ilog2`", "consider using .ilog2()", - format!("{sugg}.ilog2()"), + format!("{}.ilog2()", snippet_with_applicability(cx, reciever.span, "..", &mut applicability)), applicability, ); } From 51be3845344dd9e8c179238a54ca0f10e37f7b0e Mon Sep 17 00:00:00 2001 From: Sour1emon Date: Wed, 4 Sep 2024 17:23:10 -0700 Subject: [PATCH 5/6] Extract suggestions to seperate function --- clippy_lints/src/manual_ilog2.rs | 34 ++++++++++++++------------------ 1 file changed, 15 insertions(+), 19 deletions(-) diff --git a/clippy_lints/src/manual_ilog2.rs b/clippy_lints/src/manual_ilog2.rs index 06a371026c6b..1c57e462d62b 100644 --- a/clippy_lints/src/manual_ilog2.rs +++ b/clippy_lints/src/manual_ilog2.rs @@ -70,16 +70,7 @@ impl LateLintPass<'_> for ManualIlog2 { return; }; if val == u128::from(bit_width) - 1 { - let sugg = snippet_with_applicability(cx, reciever.span, "..", &mut applicability); - span_lint_and_sugg( - cx, - MANUAL_ILOG2, - expr.span, - "manually reimplementing `ilog2`", - "consider using .ilog2()", - format!("{sugg}.ilog2()"), - applicability, - ); + suggest_change(cx, reciever, expr, &mut applicability); } } @@ -90,17 +81,22 @@ impl LateLintPass<'_> for ManualIlog2 { && let LitKind::Int(Pu128(2), _) = lit.node && cx.typeck_results().expr_ty(reciever).is_integral() { - span_lint_and_sugg( - cx, - MANUAL_ILOG2, - expr.span, - "manually reimplementing `ilog2`", - "consider using .ilog2()", - format!("{}.ilog2()", snippet_with_applicability(cx, reciever.span, "..", &mut applicability)), - applicability, - ); + suggest_change(cx, reciever, expr, &mut applicability); } } extract_msrv_attr!(LateContext); } + +fn suggest_change(cx: &LateContext<'_>, reciever: &Expr<'_>, full_expr: &Expr<'_>, applicability: &mut Applicability) { + let sugg = snippet_with_applicability(cx, reciever.span, "..", applicability); + span_lint_and_sugg( + cx, + MANUAL_ILOG2, + full_expr.span, + "manually reimplementing `ilog2`", + "consider using .ilog2()", + format!("{sugg}.ilog2()"), + *applicability, + ); +} From bd65f178e0a91ced4bb80f8ea807479e3cfb8465 Mon Sep 17 00:00:00 2001 From: Sour1emon Date: Sat, 7 Sep 2024 10:34:50 -0700 Subject: [PATCH 6/6] Updated test comment --- tests/ui/manual_ilog2.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/ui/manual_ilog2.rs b/tests/ui/manual_ilog2.rs index 0bf926b9d5b7..c72b1ba66f0b 100644 --- a/tests/ui/manual_ilog2.rs +++ b/tests/ui/manual_ilog2.rs @@ -8,5 +8,5 @@ fn main() { let b: u64 = 543534; let _ = 63 - b.leading_zeros(); - let _ = 64 - b.leading_zeros(); // No lint + let _ = 64 - b.leading_zeros(); // No lint because manual ilog2 is BIT_WIDTH - 1 - x.leading_zeros() }