Skip to content

refactor(non_canonical_impls): lint starting from impls #15749

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

refactor(non_canonical_impls): lint starting from impls #15749

wants to merge 3 commits into from

Conversation

ada4a
Copy link
Contributor

@ada4a ada4a commented Sep 23, 2025

Based on #15520 (comment), with the following changes/additions:

  • No longer use the Trait enum for quickly filtering out irrelevant impls -- instead, check the trait_of basically right away:
    1. pre-fetch DefIds of the relevant traits into the lint pass
    2. reuse cx.tcx.impl_trait_ref's output for the the DefId of the trait being implemented
    3. compare those DefIds, which should be very cheap
  • Next, check whether self_ty implements the other relevant trait.
  • Pre-filter impl items in the same (lazy) iterator, but delay the proc-macro check as much as possible

changelog: none

Not auto-assigning since @blyxyas and/or @Jarcho will be the ones reviewing it:
r? ghost

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 23, 2025
@ada4a
Copy link
Contributor Author

ada4a commented Sep 23, 2025

Depends on #15520, so @rustbot label S-blocked

@rustbot rustbot added the S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work label Sep 23, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 23, 2025 •
edited
Loading

No changes for 3c02c0e

@ada4a ada4a force-pushed the non_canonical_impls-2 branch 3 times, most recently from 7b7a795 to 9718c47 Compare September 23, 2025 22:32
@ada4a ada4a mentioned this pull request Sep 24, 2025
Merged
@ada4a
Copy link
Contributor Author

ada4a commented Sep 24, 2025

The PR was merged, so @rustbot label -S-blocked

@rustbot rustbot removed the S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work label Sep 24, 2025
@ada4a ada4a force-pushed the non_canonical_impls-2 branch from 9718c47 to 948e23d Compare September 24, 2025 14:23
@rustbot

This comment has been minimized.

Sign in to view
@ada4a ada4a force-pushed the non_canonical_impls-2 branch from 948e23d to 2168603 Compare September 24, 2025 15:18
Jarcho
Jarcho requested changes Oct 1, 2025
View reviewed changes
Copy link
Contributor

@Jarcho Jarcho left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Not a change you made, but can you move the trait impl type argument check for PartialOrd to be before walking the assoc items. If the left and right types don't match then there is no Ord impl to call.

View changes since this review

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Oct 1, 2025
@ada4a
Copy link
Contributor Author

ada4a commented Oct 1, 2025

Not a change you made, but can you move the trait impl type argument check for PartialOrd to be before walking the assoc items. If the left and right types don't match then there is no Ord impl to call.

Good idea. I'd like to keep that a separate commit though, if that's okay

@ada4a ada4a force-pushed the non_canonical_impls-2 branch from 2168603 to e776e42 Compare October 1, 2025 21:09
@rustbot

This comment has been minimized.

Sign in to view
Jarcho
Jarcho reviewed Oct 11, 2025
View reviewed changes
clippy_lints/src/non_canonical_impls.rs Outdated
// If `Self` and `Rhs` are not the same type, then a corresponding `Ord` impl is not possible,
// since it doesn't have an `Rhs`
&& match trait_impl.args.as_slice() {
[_] => true,
Copy link
Contributor

Choose a reason for hiding this comment

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

If this happens something is very broken. All argument lists are complete once you get to the type system layer in the compiler so both arguments to PartialOrd (Self and Rhs) have to exist here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I thought there will be only one argument if Self=Rhs

Jarcho
Jarcho reviewed Oct 11, 2025
View reviewed changes
clippy_lints/src/non_canonical_impls.rs Outdated
&& let Some(of_trait) = impl_.of_trait
&& let Some(trait_did) = of_trait.trait_ref.trait_def_id()
// Check this early to hopefully bail out as soon as possible
&& [self.clone_trait, self.partial_ord_trait].contains(&Some(trait_did))
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 just convert it to an enum and do an exhaustive match later.

clippy_lints/src/non_canonical_impls.rs Outdated
Comment on lines 191 to 195
for (assoc, body, block) in assoc_fns {
if assoc.ident.name == sym::partial_cmp {
check_partial_ord_on_ord(cx, assoc, item, body, block);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use assoc_fns.iter().find(..) since there should only be one function with that name.

clippy_lints/src/non_canonical_impls.rs Outdated
Comment on lines 230 to 232
if is_from_proc_macro(cx, impl_item) {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Why the nested if instead of && !is_from_proc_macro(..).

@rustbot

This comment has been minimized.

Sign in to view
ada4a added 3 commits October 11, 2025 09:24
@ada4a
add missing test for macros
097f2fd 
The missing external macro test for `non_canonical_clone_impl` was
caught thanks to lintcheck -- I went ahead and added all the other
combinations of the test while at it
@ada4a
check earlier for PartialEq where Rhs != Self
16880f7
@ada4a
refactor(non_canonical_impls): lint starting from impls
3c02c0e
@ada4a ada4a force-pushed the non_canonical_impls-2 branch from e776e42 to 3c02c0e Compare October 11, 2025 08:05
@rustbot
Copy link
Collaborator

rustbot commented Oct 11, 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
Copy link
Contributor Author

ada4a commented Oct 11, 2025

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Oct 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Jarcho Jarcho Jarcho requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@Jarcho Jarcho

@blyxyas blyxyas

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ada4a @rustbot @Jarcho @blyxyas