Add manual_isolate_lowest_one lint

[arunsingh]

View all comments

changelog: new lint: [manual_isolate_lowest_one]

Adds a new manual_isolate_lowest_one lint for manual lowest-set-bit isolation patterns that can be written with .isolate_lowest_one().

This detects simple local expressions in the following forms:

• x & x.wrapping_neg()
• x.wrapping_neg() & x
• x & -x for signed integers
• -x & x for signed integers
• nz.get() & nz.get().wrapping_neg() for NonZero<T> integer locals
• nz.get() & -nz.get() for signed NonZero<T> integer locals

The implementation intentionally limits suggestions to simple local paths so rustfix does not change evaluation count for non-trivial expressions. For NonZero<T>::get() forms, the suggestion preserves the original primitive result type by using nz.isolate_lowest_one().get().

Validation:

• cargo fmt
• cargo check -p clippy_lints
• TESTNAME=manual_isolate_lowest_one cargo uitest
• cargo dev fmt --check
• git diff --check

Note: cargo test --test dogfood was attempted locally but hit the existing Windows tikv_jemalloc_sys crate resolution issue before reaching a PR-specific failure. The focused lint UI test and clippy_lints check pass.

[rustbot]

r? @samueltardieu

rustbot has assigned @samueltardieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: 7 candidates
• 7 candidates expanded to 7 candidates
• Random selection from Jarcho, llogiq, samueltardieu

[samueltardieu]

Since the lint highest level operation is a BitAnd, it should probably belong to the operators module.

Also, please include tests where the various parts come from macros since you exclude them from the lint.

View changes since this review

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[arunsingh]

Addressed review feedback in 45572cf:

• Moved manual_isolate_lowest_one into the operators module since the top-level operation is BitAnd.
• Added macro-origin negative UI cases for the whole & expression, lhs, rhs, and unary negation forms.
• Updated version/MSRV to 1.98.0 and category to complexity.
• Moved the MSRV gate behind the structural/type checks.
• Removed syntax-context plumbing and hir_with_context; matching now uses local ids and the diagnostic helper receives the span directly.

Verification run locally:

• cargo check -p clippy_lints
• TESTNAME=manual_isolate_lowest_one cargo uitest
• cargo dev fmt --check
• git diff --check

@rustbot ready

[arunsingh]

Added explicit MSRV compatibility coverage in manual_isolate_lowest_one UI tests:

• #[clippy::msrv = 1.97] stays silent, so the lint does not suggest an API unavailable to older MSRV projects.
• #[clippy::msrv = 1.98] lints and rustfixes to .isolate_lowest_one().

Local focused verification:

• TESTNAME=manual_isolate_lowest_one cargo uitest
• cargo check -p clippy_lints
• cargo dev fmt --check
• git diff --check

@rustbot ready

[samueltardieu]

NonZero support is intentionally left as follow-up work; this PR limits detection to primitive integer locals to keep the first implementation conservative and rustfix-safe.

Why would NonZero support add rustfix unsafety?

View changes since this review

[arunsingh]

Addressed the latest review in af1af72.

• On NonZero<T>: agreed, my previous PR-body wording was imprecise. NonZero does not add rustfix unsafety for simple local .get() forms, so I added support for nz.get() & nz.get().wrapping_neg() / reversed order and signed nz.get() & -nz.get() / reversed order. The suggestion preserves the original primitive result type as nz.isolate_lowest_one().get().
• On MSRV: updated the alias and lint metadata to 1.97.0, and changed the UI test boundary to verify 1.96 is silent while 1.97 lints.
• I kept the non-simple-expression guard: calls such as next_nonzero_u32().get() & next_nonzero_u32().get().wrapping_neg() remain intentionally silent so rustfix does not change evaluation count.

Verification after the changes:

• cargo fmt
• cargo check -p clippy_lints
• TESTNAME=manual_isolate_lowest_one cargo uibless
• TESTNAME=manual_isolate_lowest_one cargo uitest
• cargo dev fmt --check
• git diff --check

@rustbot ready

[samueltardieu]

This lint should also be added to the list of lints that check the MSRV in clippy_config::conf.

View changes since this review

[arunsingh]

I have addressed the latest review in a9f8638.

based on @samueltardieu comments:

• I had add manual_isolate_lowest_one to clippy_config::conf's msrv lint list so the generated config metadata correctly shows that this lint observes the msrv setting.
• Replaced the duplicated primitive/NonZero diagnostic helpers with one emit_lint(..., add_get) helper, keeping the only behavior difference as the .get() suffix in the suggestion.
• Switched from Sugg::hir to Sugg::hir_with_applicability, so if the source snippet cannot be recovered the suggestion is no longer reported as blindly machine-applicable.

Fresh verification after the patch:

• cargo fmt
• cargo dev fmt
• cargo check -p clippy_lints
• TESTNAME=manual_isolate_lowest_one cargo uitest
• cargo dev fmt --check
• git diff --chec

@rustbot ready

[arunsingh]

Small follow-up for CI: Clippy Test / base caught that book/src/lint_configuration.md needed to be regenerated after adding the lint to the msrv config list. I ran cargo bless --test config-metadata and amended the generated one-line book update in 029457bc14d0a2ab699a3b8c187d39d2c11573f2.

Additional verification:

• cargo bless --test config-metadata
• cargo dev fmt --check
• git diff --check