Skip to content

Add [manual_ilog2] lint #13331

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions clippy_config/src/msrvs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 2 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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`
}
102 changes: 102 additions & 0 deletions clippy_lints/src/manual_ilog2.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
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?
/// Manual reimplementations of `ilog2` increase code complexity for little benefit.
/// ### 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;
Comment on lines +52 to +55
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to check the HIR tree first.


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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will also need a check for an inherent method.

{
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 {
suggest_change(cx, reciever, expr, &mut applicability);
}
}

if let ExprKind::MethodCall(method_name, reciever, args, _) = expr.kind
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use [arg] instead to save the length check later.

&& method_name.ident.as_str() == "ilog"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The string check should be moved to after others. It requires locking the symbol table.

&& 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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a check for an inherent method rather than just a type check (could be a trait function with the same name). You can add is_inherent_method_call.

{
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,
);
}
12 changes: 12 additions & 0 deletions tests/ui/manual_ilog2.fixed
Original file line number Diff line number Diff line change
@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it check // No lint

Suppose no need to check this additionally

Copy link
Contributor Author

@Sour1emon Sour1emon Sep 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The manual ilog2 implementation is bit_width - 1 - x.leading_zeros() so this is just making sure that any other constant besides bit_width - 1 does not lint.

}
12 changes: 12 additions & 0 deletions tests/ui/manual_ilog2.rs
Original file line number Diff line number Diff line change
@@ -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 because manual ilog2 is BIT_WIDTH - 1 - x.leading_zeros()
}
23 changes: 23 additions & 0 deletions tests/ui/manual_ilog2.stderr
Original file line number Diff line number Diff line change
@@ -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

Loading