Skip to content

feat(manual_ilog2): new lint#15865

Merged
Alexendoo merged 1 commit into
rust-lang:masterfrom
ada4a:manual_ilog2
Dec 2, 2025
Merged

feat(manual_ilog2): new lint#15865
Alexendoo merged 1 commit into
rust-lang:masterfrom
ada4a:manual_ilog2

Conversation

@ada4a
Copy link
Copy Markdown
Contributor

@ada4a ada4a commented Oct 11, 2025

Revival of #13331

Part of #12894

changelog: [manual_ilog2]: new lint

@rustbot rustbot added needs-fcp PRs that add, remove, or rename lints and need an FCP S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Oct 11, 2025
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Oct 11, 2025

r? @dswij

rustbot has assigned @dswij.
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

@ada4a
Copy link
Copy Markdown
Contributor Author

ada4a commented Oct 11, 2025

@Jarcho you might be interested in this as you reviewed the original PR

@dswij
Copy link
Copy Markdown
Member

dswij commented Oct 31, 2025

r? clippy

@rustbot rustbot assigned Alexendoo and unassigned dswij Oct 31, 2025
Alexendoo
Alexendoo reviewed Oct 31, 2025
View reviewed changes
Comment thread clippy_lints/src/manual_ilog2.rs Outdated
Comment on lines +68 to +69
// Whether `leading_zeros` is an inherent method, i.e. doesn't come from some unrelated trait
&& cx.ty_based_def(right).opt_parent(cx).is_impl(cx)
Copy link
Copy Markdown
Member

@Alexendoo Alexendoo Oct 31, 2025

Choose a reason for hiding this comment

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

I believe this wouldn't catch that since a trait impl is also a DefKind::Impl. I think it's fine without the check though since inherent methods come first in method resolution

Copy link
Copy Markdown
Contributor Author

@ada4a ada4a Oct 31, 2025

Choose a reason for hiding this comment

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

Oh, it looks like you're right; I'll remove the call then.

The reason I wrote it that way is that that's what is_inherent_method_call was replaced with in https://github.com/rust-lang/rust-clippy/pull/15682/files#diff-9d3f62135af85bbb426d0e358666790f639f6777c8dc504a7527e2bc7f830fb1R741, and so I assumed it would only match, well, inherent method calls. @Jarcho maybe this should be a separate method, something like is_inherent_impl?

Copy link
Copy Markdown
Contributor

@Jarcho Jarcho Nov 1, 2025

Choose a reason for hiding this comment

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

TypeckResults always has things resolved to either the inherent item or the trait item, but never a trait impl item. Checking that the parent is an impl is enough because of this.

Comment thread clippy_lints/src/manual_ilog2.rs Outdated
@Alexendoo Alexendoo added S-final-comment-period Status: final comment period it will be merged unless new objections are raised (~1 week) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties needs-fcp PRs that add, remove, or rename lints and need an FCP labels Oct 31, 2025
@Alexendoo
Copy link
Copy Markdown
Member

Alexendoo commented Oct 31, 2025

Opened https://rust-lang.zulipchat.com/#narrow/channel/257328-clippy/topic/FCP.20manual_ilog2/with/548234188

@rustbot rustbot added the needs-fcp PRs that add, remove, or rename lints and need an FCP label Oct 31, 2025
@rustbot

This comment has been minimized.

Sign in to view
Jarcho
Jarcho reviewed Nov 1, 2025
View reviewed changes
Comment thread clippy_lints/src/manual_ilog2.rs Outdated
&& ilog.ident.name == sym::ilog
&& let ExprKind::Lit(lit) = two.kind
&& let LitKind::Int(Pu128(2), _) = lit.node
&& cx.typeck_results().expr_ty(recv).is_integral()
Copy link
Copy Markdown
Contributor

@Jarcho Jarcho Nov 1, 2025

Choose a reason for hiding this comment

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

This should be the adjusted type. You want the type passed in to the method.

@rustbot

This comment has been minimized.

Sign in to view
teofr
teofr reviewed Nov 1, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@teofr teofr left a comment •
edited by rustbot
Loading

Choose a reason for hiding this comment

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

Could be a separate PR, but it'd be quite easy to also lint x.checked_ilog(2) into x.checked_ilog2()

View changes since this review

Comment thread clippy_lints/src/manual_ilog2.rs
Jarcho
Jarcho reviewed Nov 2, 2025
View reviewed changes
Comment thread clippy_lints/src/manual_ilog2.rs Outdated
@rustbot

This comment has been minimized.

Sign in to view
@rustbot

This comment has been minimized.

Sign in to view
@rustbot

This comment has been minimized.

Sign in to view
@rustbot

This comment has been minimized.

Sign in to view
@rustbot

This comment has been minimized.

Sign in to view
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Nov 27, 2025

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@ada4a ada4a force-pushed the manual_ilog2 branch 2 times, most recently from ac35c0e to d9cc76a Compare November 27, 2025 21:39
Alexendoo
Alexendoo reviewed Nov 28, 2025
View reviewed changes
Comment thread clippy_lints/src/manual_ilog2.rs Outdated
Comment on lines +16 to +17
/// Checks for expressions like `31 - x.leading_zeros()` which are manual
/// reimplementations of `x.ilog2()`
Copy link
Copy Markdown
Member

@Alexendoo Alexendoo Nov 28, 2025

Choose a reason for hiding this comment

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

I think it's worth mentioning the type here so that it's clear the 31 is for u32 only

Copy link
Copy Markdown
Contributor Author

@ada4a ada4a Nov 28, 2025

Choose a reason for hiding this comment

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

Added a (somewhat wordy) note

@Alexendoo Alexendoo removed the needs-fcp PRs that add, remove, or rename lints and need an FCP label Dec 2, 2025
Alexendoo
Alexendoo approved these changes Dec 2, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@Alexendoo Alexendoo left a comment •
edited by rustbot
Loading

Choose a reason for hiding this comment

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

Thanks!

View changes since this review

@Alexendoo
Copy link
Copy Markdown
Member

Alexendoo commented Dec 2, 2025 •
edited
Loading

Oh wait this will need a rebase for #15994

I was looking at the wrong diff

@Alexendoo Alexendoo added this pull request to the merge queue Dec 2, 2025
Merged via the queue into rust-lang:master with commit a10cafe Dec 2, 2025
11 checks passed
@ada4a ada4a deleted the manual_ilog2 branch December 2, 2025 19:26
@theemathas theemathas mentioned this pull request Dec 3, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Jarcho Jarcho Jarcho left review comments

@Alexendoo Alexendoo Alexendoo approved these changes

+1 more reviewer

@teofr teofr teofr left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@Alexendoo Alexendoo

Labels

S-final-comment-period Status: final comment period it will be merged unless new objections are raised (~1 week)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@ada4a @rustbot @dswij @Alexendoo @Jarcho @teofr