diff --git a/CHANGELOG.md b/CHANGELOG.md index f53143e564b6..df0fb10ba434 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6906,6 +6906,7 @@ Released 2018-09-13 [`manual_is_multiple_of`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_is_multiple_of [`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_isolate_lowest_one`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_isolate_lowest_one [`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 [`manual_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_map diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 4218d1fdc290..04835949f3e2 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -311,6 +311,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ 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_isolate_lowest_one::MANUAL_ISOLATE_LOWEST_ONE_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 af25b1f7f1aa..89a7f9fb51f7 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -208,6 +208,7 @@ mod manual_ignore_case_cmp; mod manual_ilog2; mod manual_is_ascii_check; mod manual_is_power_of_two; +mod manual_isolate_lowest_one; mod manual_let_else; mod manual_main_separator_str; mod manual_non_exhaustive; @@ -863,6 +864,7 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co Box::new(|_| Box::::default()), Box::new(move |tcx| Box::new(disallowed_fields::DisallowedFields::new(tcx, conf))), Box::new(move |_| Box::new(manual_ilog2::ManualIlog2::new(conf))), + Box::new(move |_| Box::new(manual_isolate_lowest_one::ManualIsolateLowestOne::new(conf))), Box::new(|_| Box::new(same_length_and_capacity::SameLengthAndCapacity)), Box::new(move |tcx| Box::new(duration_suboptimal_units::DurationSuboptimalUnits::new(tcx, conf))), Box::new(move |_| Box::new(manual_take::ManualTake::new(conf))), diff --git a/clippy_lints/src/manual_isolate_lowest_one.rs b/clippy_lints/src/manual_isolate_lowest_one.rs new file mode 100644 index 000000000000..80dfa5278341 --- /dev/null +++ b/clippy_lints/src/manual_isolate_lowest_one.rs @@ -0,0 +1,113 @@ +use clippy_config::Conf; +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::msrvs::{self, Msrv}; +use clippy_utils::sugg::Sugg; +use clippy_utils::{SpanlessEq, is_from_proc_macro, sym}; +use rustc_errors::Applicability; +use rustc_hir::{BinOpKind, Expr, ExprKind, UnOp}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::impl_lint_pass; +use rustc_span::SyntaxContext; + +declare_clippy_lint! { + /// ### What it does + /// Checks for expressions like `x & -x` or `x & x.wrapping_neg()`, which are manual + /// reimplementations of `x.isolate_lowest_one()`. + /// + /// ### Why is this bad? + /// `x.isolate_lowest_one()` is clearer than the bitwise trick. It also avoids the + /// overflow that occurs when `x == T::MIN` for signed types using the `-` operator, + /// and preserves non-zero type information for `NonZero`. + /// + /// ### Example + /// ```no_run + /// let x: u32 = 5; + /// let lsb = x & x.wrapping_neg(); + /// ``` + /// Use instead: + /// ```no_run + /// let x: u32 = 5; + /// let lsb = x.isolate_lowest_one(); + /// ``` + #[clippy::version = "1.97.0"] + pub MANUAL_ISOLATE_LOWEST_ONE, + complexity, + "manually reimplementing `isolate_lowest_one`" +} + +impl_lint_pass!(ManualIsolateLowestOne => [MANUAL_ISOLATE_LOWEST_ONE]); + +pub struct ManualIsolateLowestOne { + msrv: Msrv, +} + +impl ManualIsolateLowestOne { + pub fn new(conf: &'static Conf) -> Self { + Self { msrv: conf.msrv } + } +} + +impl<'tcx> LateLintPass<'tcx> for ManualIsolateLowestOne { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) { + if expr.span.from_expansion() { + return; + } + + let ExprKind::Binary(op, lhs, rhs) = expr.kind else { + return; + }; + if op.node != BinOpKind::BitAnd || lhs.span.from_expansion() || rhs.span.from_expansion() { + return; + } + + let ctxt = expr.span.ctxt(); + let recv = is_negation_pair(cx, ctxt, lhs, rhs).or_else(|| is_negation_pair(cx, ctxt, rhs, lhs)); + let Some(recv) = recv else { return }; + + if !cx.typeck_results().expr_ty_adjusted(recv).is_integral() { + return; + } + + if !self.msrv.meets(cx, msrvs::ISOLATE_LOWEST_ONE) { + return; + } + + if is_from_proc_macro(cx, expr) { + return; + } + + let mut applicability = Applicability::MachineApplicable; + let snippet = Sugg::hir_with_context(cx, recv, ctxt, "_", &mut applicability); + + span_lint_and_sugg( + cx, + MANUAL_ISOLATE_LOWEST_ONE, + expr.span, + "manually reimplementing `isolate_lowest_one`", + "consider using `.isolate_lowest_one()`", + format!("{}.isolate_lowest_one()", snippet.maybe_paren()), + applicability, + ); + } +} + +/// Returns `Some(base)` if `negated` is `-base` or `base.wrapping_neg()` (where `base` is +/// structurally equal to `expected_base`). +fn is_negation_pair<'tcx>( + cx: &LateContext<'tcx>, + ctxt: SyntaxContext, + expected_base: &'tcx Expr<'tcx>, + negated: &'tcx Expr<'tcx>, +) -> Option<&'tcx Expr<'tcx>> { + match negated.kind { + ExprKind::Unary(UnOp::Neg, inner) if SpanlessEq::new(cx).eq_expr(ctxt, expected_base, inner) => { + Some(expected_base) + }, + ExprKind::MethodCall(method, inner, [], _) + if method.ident.name == sym::wrapping_neg && SpanlessEq::new(cx).eq_expr(ctxt, expected_base, inner) => + { + Some(expected_base) + }, + _ => None, + } +} diff --git a/clippy_utils/src/msrvs.rs b/clippy_utils/src/msrvs.rs index a56e729c70bb..5151351ed2ad 100644 --- a/clippy_utils/src/msrvs.rs +++ b/clippy_utils/src/msrvs.rs @@ -23,6 +23,7 @@ macro_rules! msrv_aliases { // names may refer to stabilized feature flags or library items msrv_aliases! { + 1,97,0 { ISOLATE_LOWEST_ONE } 1,93,0 { VEC_DEQUE_POP_BACK_IF, VEC_DEQUE_POP_FRONT_IF } 1,91,0 { DURATION_FROM_MINUTES_HOURS } 1,88,0 { LET_CHAINS } diff --git a/clippy_utils/src/sym.rs b/clippy_utils/src/sym.rs index d04257067be3..42f4e7c898c8 100644 --- a/clippy_utils/src/sym.rs +++ b/clippy_utils/src/sym.rs @@ -649,6 +649,7 @@ generate! { warnings, wildcard_imports, with_capacity, + wrapping_neg, wrapping_offset, write, write_unaligned, diff --git a/tests/ui/manual_isolate_lowest_one.fixed b/tests/ui/manual_isolate_lowest_one.fixed new file mode 100644 index 000000000000..a1c35af7ed62 --- /dev/null +++ b/tests/ui/manual_isolate_lowest_one.fixed @@ -0,0 +1,34 @@ +//@aux-build:proc_macros.rs +#![warn(clippy::manual_isolate_lowest_one)] +#![allow(clippy::unnecessary_operation, clippy::no_effect, dead_code)] + +use proc_macros::{external, with_span}; + +fn unsigned(a: u32, b: u64) { + let _ = a.isolate_lowest_one(); //~ manual_isolate_lowest_one + let _ = a.isolate_lowest_one(); //~ manual_isolate_lowest_one + let _ = b.isolate_lowest_one(); //~ manual_isolate_lowest_one +} + +fn signed(a: i32) { + let _ = a.isolate_lowest_one(); //~ manual_isolate_lowest_one + let _ = a.isolate_lowest_one(); //~ manual_isolate_lowest_one + let _ = a.isolate_lowest_one(); //~ manual_isolate_lowest_one +} + +fn no_lint_different_operand(a: i32, c: i32) { + // Different operands — must not lint. + let _ = a & c.wrapping_neg(); + let _ = a & -c; +} + +fn no_lint_macros(a: u32) { + macro_rules! same { + ($x:expr) => { + $x & $x.wrapping_neg() + }; + } + same!(a); + external!($a & $a.wrapping_neg()); + with_span!(span; a & a.wrapping_neg()); +} diff --git a/tests/ui/manual_isolate_lowest_one.rs b/tests/ui/manual_isolate_lowest_one.rs new file mode 100644 index 000000000000..68835ab9d9a6 --- /dev/null +++ b/tests/ui/manual_isolate_lowest_one.rs @@ -0,0 +1,34 @@ +//@aux-build:proc_macros.rs +#![warn(clippy::manual_isolate_lowest_one)] +#![allow(clippy::unnecessary_operation, clippy::no_effect, dead_code)] + +use proc_macros::{external, with_span}; + +fn unsigned(a: u32, b: u64) { + let _ = a & a.wrapping_neg(); //~ manual_isolate_lowest_one + let _ = a.wrapping_neg() & a; //~ manual_isolate_lowest_one + let _ = b & b.wrapping_neg(); //~ manual_isolate_lowest_one +} + +fn signed(a: i32) { + let _ = a & -a; //~ manual_isolate_lowest_one + let _ = -a & a; //~ manual_isolate_lowest_one + let _ = a & a.wrapping_neg(); //~ manual_isolate_lowest_one +} + +fn no_lint_different_operand(a: i32, c: i32) { + // Different operands — must not lint. + let _ = a & c.wrapping_neg(); + let _ = a & -c; +} + +fn no_lint_macros(a: u32) { + macro_rules! same { + ($x:expr) => { + $x & $x.wrapping_neg() + }; + } + same!(a); + external!($a & $a.wrapping_neg()); + with_span!(span; a & a.wrapping_neg()); +} diff --git a/tests/ui/manual_isolate_lowest_one.stderr b/tests/ui/manual_isolate_lowest_one.stderr new file mode 100644 index 000000000000..242ad2961dc3 --- /dev/null +++ b/tests/ui/manual_isolate_lowest_one.stderr @@ -0,0 +1,41 @@ +error: manually reimplementing `isolate_lowest_one` + --> tests/ui/manual_isolate_lowest_one.rs:8:13 + | +LL | let _ = a & a.wrapping_neg(); + | ^^^^^^^^^^^^^^^^^^^^ help: consider using `.isolate_lowest_one()`: `a.isolate_lowest_one()` + | + = note: `-D clippy::manual-isolate-lowest-one` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::manual_isolate_lowest_one)]` + +error: manually reimplementing `isolate_lowest_one` + --> tests/ui/manual_isolate_lowest_one.rs:9:13 + | +LL | let _ = a.wrapping_neg() & a; + | ^^^^^^^^^^^^^^^^^^^^ help: consider using `.isolate_lowest_one()`: `a.isolate_lowest_one()` + +error: manually reimplementing `isolate_lowest_one` + --> tests/ui/manual_isolate_lowest_one.rs:10:13 + | +LL | let _ = b & b.wrapping_neg(); + | ^^^^^^^^^^^^^^^^^^^^ help: consider using `.isolate_lowest_one()`: `b.isolate_lowest_one()` + +error: manually reimplementing `isolate_lowest_one` + --> tests/ui/manual_isolate_lowest_one.rs:14:13 + | +LL | let _ = a & -a; + | ^^^^^^ help: consider using `.isolate_lowest_one()`: `a.isolate_lowest_one()` + +error: manually reimplementing `isolate_lowest_one` + --> tests/ui/manual_isolate_lowest_one.rs:15:13 + | +LL | let _ = -a & a; + | ^^^^^^ help: consider using `.isolate_lowest_one()`: `a.isolate_lowest_one()` + +error: manually reimplementing `isolate_lowest_one` + --> tests/ui/manual_isolate_lowest_one.rs:16:13 + | +LL | let _ = a & a.wrapping_neg(); + | ^^^^^^^^^^^^^^^^^^^^ help: consider using `.isolate_lowest_one()`: `a.isolate_lowest_one()` + +error: aborting due to 6 previous errors +