Skip to content
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 @@ -6540,6 +6540,7 @@ Released 2018-09-13
[`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_ignore_case_cmp`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_ignore_case_cmp
[`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_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
crate::manual_float_methods::MANUAL_IS_INFINITE_INFO,
crate::manual_hash_one::MANUAL_HASH_ONE_INFO,
crate::manual_ignore_case_cmp::MANUAL_IGNORE_CASE_CMP_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 @@ -201,6 +201,7 @@ mod manual_clamp;
mod manual_float_methods;
mod manual_hash_one;
mod manual_ignore_case_cmp;
mod manual_ilog2;
mod manual_is_ascii_check;
mod manual_is_power_of_two;
mod manual_let_else;
Expand Down Expand Up @@ -819,5 +820,6 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co
store.register_late_pass(|_| Box::new(toplevel_ref_arg::ToplevelRefArg));
store.register_late_pass(|_| Box::new(volatile_composites::VolatileComposites));
store.register_late_pass(|_| Box::new(replace_box::ReplaceBox::default()));
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`
}
110 changes: 110 additions & 0 deletions clippy_lints/src/manual_ilog2.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
use clippy_config::Conf;
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::msrvs::{self, Msrv};
use clippy_utils::source::snippet_with_applicability;
use clippy_utils::{is_from_proc_macro, sym};
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, LintContext};
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();
/// let log = x.ilog(2);
/// ```
/// Use instead:
/// ```no_run
/// let x: u32 = 5;
/// let log = x.ilog2();
/// let log = x.ilog2();
/// ```
#[clippy::version = "1.92.0"]
pub MANUAL_ILOG2,
complexity,
"manually reimplementing `ilog2`"
}

pub struct ManualIlog2 {
msrv: Msrv,
}

impl ManualIlog2 {
pub fn new(conf: &Conf) -> Self {
Self { msrv: conf.msrv }
}
}

impl_lint_pass!(ManualIlog2 => [MANUAL_ILOG2]);

impl LateLintPass<'_> for ManualIlog2 {
fn check_expr<'tcx>(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
if expr.span.in_external_macro(cx.sess().source_map()) {
return;
}

match expr.kind {
// `BIT_WIDTH - 1 - n.leading_zeros()`
ExprKind::Binary(op, left, right)
if left.span.eq_ctxt(right.span)
&& op.node == BinOpKind::Sub
&& let ExprKind::Lit(lit) = left.kind
&& let LitKind::Int(Pu128(val), _) = lit.node
&& let ExprKind::MethodCall(leading_zeros, recv, [], _) = right.kind
&& leading_zeros.ident.name == sym::leading_zeros
&& let ty = cx.typeck_results().expr_ty(recv)
&& let Some(bit_width) = match ty.kind() {
ty::Int(int_ty) => int_ty.bit_width(),
ty::Uint(uint_ty) => uint_ty.bit_width(),
_ => return,
}
&& val == u128::from(bit_width) - 1
&& self.msrv.meets(cx, msrvs::ILOG2)
&& !is_from_proc_macro(cx, expr) =>
Comment on lines +74 to +75
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd move this to the start of the function, since they're really not part of the 31 - x.leading_zeros() case.

Idem below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would remove some duplication, yes, but the problem is that both of these checks are rather expensive:

  • the MSRV check requires walking up through all the attributes, and I think also going through the Clippy config
  • the proc macro check is very expensive I'm told (though I'm not completely sure why), and so it should generally be the last if of any lint

Copy link
Contributor

Choose a reason for hiding this comment

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

These checks aren't first in general as a perf thing. They're bad filters in general and neither are fast.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation, sorry for the noise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All good!

{
emit(cx, recv, expr);
},

// `n.ilog(2)`
ExprKind::MethodCall(ilog, recv, [two], _)
if expr.span.eq_ctxt(two.span)
&& ilog.ident.name == sym::ilog
&& let ExprKind::Lit(lit) = two.kind
&& let LitKind::Int(Pu128(2), _) = lit.node
&& cx.typeck_results().expr_ty_adjusted(recv).is_integral()
/* no need to check MSRV here, as `ilog` and `ilog2` were introduced simultaneously */
&& !is_from_proc_macro(cx, expr) =>
{
emit(cx, recv, expr);
},

_ => {},
}
}
}

fn emit(cx: &LateContext<'_>, recv: &Expr<'_>, full_expr: &Expr<'_>) {
let mut app = Applicability::MachineApplicable;
let recv = snippet_with_applicability(cx, recv.span, "_", &mut app);
span_lint_and_sugg(
cx,
MANUAL_ILOG2,
full_expr.span,
"manually reimplementing `ilog2`",
"try",
format!("{recv}.ilog2()"),
app,
);
}
1 change: 1 addition & 0 deletions clippy_utils/src/msrvs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,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 { ILOG2 }
1,65,0 { LET_ELSE, POINTER_CAST_CONSTNESS }
1,63,0 { CLONE_INTO, CONST_SLICE_FROM_REF }
1,62,0 { BOOL_THEN_SOME, DEFAULT_ENUM_ATTRIBUTE, CONST_EXTERN_C_FN }
Expand Down
2 changes: 2 additions & 0 deletions clippy_utils/src/sym.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,7 @@ generate! {
has_significant_drop,
hidden_glob_reexports,
hygiene,
ilog,
insert,
insert_str,
inspect,
Expand Down Expand Up @@ -207,6 +208,7 @@ generate! {
join,
kw,
lazy_static,
leading_zeros,
lint_vec,
ln,
lock,
Expand Down
32 changes: 32 additions & 0 deletions tests/ui/manual_ilog2.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
//@aux-build:proc_macros.rs
#![warn(clippy::manual_ilog2)]
#![allow(clippy::unnecessary_operation)]

use proc_macros::{external, with_span};

fn foo(a: u32, b: u64) {
a.ilog2(); //~ manual_ilog2
a.ilog2(); //~ manual_ilog2

b.ilog2(); //~ manual_ilog2
64 - b.leading_zeros(); // No lint because manual ilog2 is `BIT_WIDTH - 1 - x.leading_zeros()`

// don't lint when macros are involved
macro_rules! two {
() => {
2
};
};

macro_rules! thirty_one {
() => {
31
};
};

a.ilog(two!());
thirty_one!() - a.leading_zeros();

external!($a.ilog(2));
with_span!(span; a.ilog(2));
}
32 changes: 32 additions & 0 deletions tests/ui/manual_ilog2.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
//@aux-build:proc_macros.rs
#![warn(clippy::manual_ilog2)]
#![allow(clippy::unnecessary_operation)]

use proc_macros::{external, with_span};

fn foo(a: u32, b: u64) {
31 - a.leading_zeros(); //~ manual_ilog2
a.ilog(2); //~ manual_ilog2

63 - b.leading_zeros(); //~ manual_ilog2
64 - b.leading_zeros(); // No lint because manual ilog2 is `BIT_WIDTH - 1 - x.leading_zeros()`

// don't lint when macros are involved
macro_rules! two {
() => {
2
};
};

macro_rules! thirty_one {
() => {
31
};
};

a.ilog(two!());
thirty_one!() - a.leading_zeros();

external!($a.ilog(2));
with_span!(span; a.ilog(2));
}
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:8:5
|
LL | 31 - a.leading_zeros();
| ^^^^^^^^^^^^^^^^^^^^^^ help: try: `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:9:5
|
LL | a.ilog(2);
| ^^^^^^^^^ help: try: `a.ilog2()`

error: manually reimplementing `ilog2`
--> tests/ui/manual_ilog2.rs:11:5
|
LL | 63 - b.leading_zeros();
| ^^^^^^^^^^^^^^^^^^^^^^ help: try: `b.ilog2()`

error: aborting due to 3 previous errors