`non_canonical_impls`: split the main `check` function #15520

ada4a · 2025-08-20T21:19:41Z

this is based #15519, but mainly to avoid gnarly rebase conflicts later

changelog: none

rustbot · 2025-08-20T21:19:45Z

rustbot has assigned @Alexendoo.
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 · 2025-08-20T21:19:54Z

r? clippy

blyxyas · 2025-08-24T13:15:53Z

:0 triagebot was updated with new functionality. Neat!

blyxyas · 2025-09-05T22:04:48Z

~~Profiling results: 0.5% performance improvement found on tokio for this pull request.~~

ada4a · 2025-09-05T22:10:18Z

That's nice, but also a bit unexpected -- this PR (as far as I can recollect from looking at the diff lol) only moves some stuff around. My only explanation would be that reducing the size of the check function allowed LLVM to optimize it better? Or does a 0.5% change lie within noice threshold @blyxyas?

blyxyas · 2025-09-05T23:31:18Z

Hi Ada, sorry to announce that my previous comment was not accurate. I just forgot to propagate the -Wclippy::non_canonical* rust flag to master. The actual numbers are not in the noticeable range.

ada4a · 2025-09-05T23:38:18Z

Ah, thanks for letting me know. But yeah, I wasn't expecting them to be, so all good:)

clippy_lints/src/non_canonical_impls.rs

blyxyas · 2025-09-21T16:08:38Z

clippy_lints/src/non_canonical_impls.rs

+            && !block.span.in_external_macro(cx.sess().source_map())
+            && !is_from_proc_macro(cx, impl_item)
+        {
+            let trait_name = cx.tcx.get_diagnostic_name(trait_impl.def_id);


Move this check into a fairly up position with matches!(trait_name, Some(sym::Clone) | Some(sym::PartialOrd)), as it's a fairly cheap check

The link points to the source code of get_diagnostic_item, not get_diagnostic_name? And I've always heard the opposite -- that get_diagnostic_name is fairly expensive, and one should at least avoid calling it twice. But if you say so...

I'm also not exactly sure how you see this being implemented, is it something like this?

diff --git i/clippy_lints/src/non_canonical_impls.rs w/clippy_lints/src/non_canonical_impls.rs index c385f03ba..56422909d 100644 --- i/clippy_lints/src/non_canonical_impls.rs +++ w/clippy_lints/src/non_canonical_impls.rs @@ -115,6 +115,9 @@ impl LateLintPass<'_> for NonCanonicalImpls { fn check_impl_item<'tcx>(&mut self, cx: &LateContext<'tcx>, impl_item: &ImplItem<'tcx>) { if let Node::Item(item) = cx.tcx.parent_hir_node(impl_item.hir_id()) && let Some(trait_impl) = cx.tcx.impl_trait_ref(item.owner_id).map(EarlyBinder::skip_binder) + && let trait_name = cx.tcx.get_diagnostic_name(trait_impl.def_id) + // NOTE: check this early to avoid expensive checks that come after this one + && matches!(trait_name, Some(sym::Clone | sym::PartialOrd)) && !cx.tcx.is_automatically_derived(item.owner_id.to_def_id()) && let ImplItemKind::Fn(_, impl_item_id) = impl_item.kind && let body = cx.tcx.hir_body(impl_item_id) @@ -122,7 +125,6 @@ impl LateLintPass<'_> for NonCanonicalImpls { && !block.span.in_external_macro(cx.sess().source_map()) && !is_from_proc_macro(cx, impl_item) { - let trait_name = cx.tcx.get_diagnostic_name(trait_impl.def_id); if trait_name == Some(sym::Clone) && let Some(copy_def_id) = cx.tcx.get_diagnostic_item(sym::Copy) && implements_trait(cx, trait_impl.self_ty(), copy_def_id, &[])

get_diagnostic_name, get_diagnostic_item, etc... all call to all_diagnostic_items(()) which calls todiagnostic_items(krate_id). This function is querified via the query system (that's why the definition on rustdoc goes to a macro).

Queries cache their results, so just running them the first time is expensive. And we call these functions many times throughout the compiler (every time we want to mention a diagnostic item, so not few). And apart from that, an unconditional all_diagnostic_items(()) is executed here

So the cost of computing diagnostic items doesn't really incur much, just the cost of retrieving the result =^w^=

NOTE: Yep that's more or less how I'd implement it

[...] all call to all_diagnostic_items(()) which calls to diagnostic_items(krate_id). This function is querified via the query system (that's why the definition on rustdoc goes to a macro).

That is indeed what I would have expected, weird that I was hearing people say the opposite.. Thank you again for clearing this up:)

The link points to the source code of get_diagnostic_item, not get_diagnostic_name? And I've always heard the opposite -- that get_diagnostic_name is fairly expensive, and one should at least avoid calling it twice.

Not doing the job twice is always better. My recommendation is usually to avoid calling is_diagnostic_item(A) || is_diagnostic_item(B) when you can call matches!(get_diagnostic_name(), Some(A | B)), that prevents a double-lookup.

And also, such a lookup is also more expensive (even if cheap) than a structural matching against Expr::kind for example.

clippy_lints/src/non_canonical_impls.rs

ada4a · 2025-09-21T20:10:55Z

@rustbot ready

I put the incorporation of the review feedback into its own commit, because all the comments were about the pre-existing code.. Do let me know if you'd prefer something else

blyxyas

LGTM, thanks! ❤️

View changes since this review

blyxyas

Reverting the approval just for being safe, I was testing yesterday if diagnostic_items actually caches its results or not, as it has the eval_always. (No changes are actually requested, just blocking the PR from merging by another maintainer)

blyxyas

Just checked, and indeed queries with eval_always only mean that they are not taken into account between incremental compilations. I'll make a PR to r-l/r because it's a really confusing name TwT.

Just one more nit and I'll merge this

View changes since this review

blyxyas · 2025-09-23T12:30:38Z

clippy_lints/src/non_canonical_impls.rs

+            && let trait_name = cx.tcx.get_diagnostic_name(trait_impl.def_id)
+            // NOTE: check this early to avoid expensive checks that come after this one
+            && matches!(trait_name, Some(sym::Clone | sym::PartialOrd))
+            && !cx.tcx.is_automatically_derived(item.owner_id.to_def_id())
+            && let ImplItemKind::Fn(_, impl_item_id) = impl_item.kind


Could you move this statement above?

Suggested change

&& let trait_name = cx.tcx.get_diagnostic_name(trait_impl.def_id)

// NOTE: check this early to avoid expensive checks that come after this one

&& matches!(trait_name, Some(sym::Clone | sym::PartialOrd))

&& !cx.tcx.is_automatically_derived(item.owner_id.to_def_id())

&& let ImplItemKind::Fn(_, impl_item_id) = impl_item.kind

&& let trait_name = cx.tcx.get_diagnostic_name(trait_impl.def_id)

&& let ImplItemKind::Fn(_, impl_item_id) = impl_item.kind

// NOTE: check this early to avoid expensive checks that come after this one

&& matches!(trait_name, Some(sym::Clone | sym::PartialOrd))

&& !cx.tcx.is_automatically_derived(item.owner_id.to_def_id())

If the motivation is that this is a cheap check that could filter out many cases, then I'd maybe put it as the very first one?

A bit off-topic: staring at the let-chain a bit more, why do we even start from an ImplItem, anyway? It looks like we:

start from the fn clone/eq

go up to the impl

check if it's impl Clone/PartialEq for S

check whether S implements the other trait

check whether the fn clone/eq we started with is the canonical implementation

But I think it would be less confusing to:

start from an impl

check if it's impl Clone/PartialEq for S

check whether S implements the other trait

descend to the method(s) and check those

Great thinking outside the box. If you want, now it's the moment to implement it. It has a +1 from me

Great thinking outside the box. If you want, now it's the moment to implement it. It has a +1 from me

Thank you^^ But in all honesty I'm not completely sure which of the two points you are referring to, so I'll implement the former in this PR, and probably create a second PR for the latter?

If you're going to start at the impl you'll want something like:

enum Trait { Clone, PartialOrd } impl Trait { fn from_def_id(lang_items: &LanguageItems, did: DefId) -> Option<Self> { if Some(trait_id) == lang_items.clone_trait() { Some(Self::Clone) } else if Some(did) == lang_items.partial_ord_trait() { Some(Self::PartialOrd) } else { None } } } fn check_item(&mut self,cx: &LateContext<'_>,item: &'_ rustc_hir::Item<'_>) { if let ItemKind::Impl(impl_) = item.kind && let Some(trait_) = impl_.of_trait && impl_.items.len() <= 2 && let Some(trait_id) = trait_.trait_ref.path.res.opt_def_id() && let Some(trait_) = Trait::from_def_id(cx.tcx.lang_items(), trait_id) && !cx.tcx.is_automatically_derived(item.owner_id.to_def_id()) { match trait_ { Trait::Clone => match *impl_.items { [assoc, _] if let assoc = cx.tcx.hir_impl_item(assoc) && assoc.name == sym::clone => check_clone(assoc), [assoc] | [_, assoc] => check_clone(cx.tcx.hir_impl_item(assoc)), _ => return, } Trait::PartialOrd if let Some(assoc) = impl_.items.iter().find_map(|&id| { let assoc = cx.tcx.hir_impl_item(assoc) (assoc.name == sym::partial_cmp).then_some(assoc) }) => check_partial_ord(assoc), _ => return, } } }

This will minimize the number of impl blocks that need to call queries.

Thank you very much @Jarcho! Though check_clone does check clone_from as well, so I'll hopefully be able to simplify the first match arm a bit:)

Just edited to handle the extra methods on PartialOrd. Forgot those existed.

too many lines no more!

imo it's a bit more clear than checking for the same thing (`expr.kind`) in multiple branches

rustbot · 2025-09-23T14:37:37Z

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 · 2025-09-23T14:38:58Z

Done^^ Also rebased onto the latest master, because the build-on-save would cause the old rust version to be used, completely wrecking (AFAICT) the incremental compilation cache

blyxyas · 2025-09-24T12:13:13Z

Are you sure it's done? I'm not seeing a change similar to what Jason suggested...

ada4a · 2025-09-24T13:20:30Z

I opened a separate PR for it (#15749), since it's a rather major change to the lint structure (and seems to have a bug currently, need to look into that). But I can pull it into this PR as well, if you prefer^^

blyxyas

LGTM, thanks! ❤️

View changes since this review

rustbot assigned Alexendoo Aug 20, 2025

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Aug 20, 2025

rustbot assigned blyxyas and unassigned Alexendoo Aug 20, 2025

ada4a force-pushed the non_canonical_impls branch 2 times, most recently from c0dd48b to e10a7a2 Compare August 22, 2025 13:32

This comment has been minimized.

Sign in to view

ada4a force-pushed the non_canonical_impls branch from e10a7a2 to e93a9fa Compare August 24, 2025 06:16

This comment has been minimized.

Sign in to view

blyxyas requested changes Sep 21, 2025

View reviewed changes

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 Sep 21, 2025

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 Sep 21, 2025

blyxyas approved these changes Sep 22, 2025

View reviewed changes

blyxyas requested changes Sep 23, 2025

View reviewed changes

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 Sep 23, 2025

blyxyas requested changes Sep 23, 2025

View reviewed changes

ada4a added 3 commits September 23, 2025 14:40

create let-chain

526d794

extract helper functions

54e2847

too many lines no more!

use match

34d9a2a

imo it's a bit more clear than checking for the same thing (`expr.kind`) in multiple branches

incorporate review feedback

21a5948

ada4a force-pushed the non_canonical_impls branch from 577e22c to 21a5948 Compare September 23, 2025 14:37

ada4a mentioned this pull request Sep 23, 2025

refactor(non_canonical_impls): lint starting from impls #15749

Open

blyxyas approved these changes Sep 24, 2025

View reviewed changes

blyxyas added this pull request to the merge queue Sep 24, 2025

Merged via the queue into rust-lang:master with commit 3e0590c Sep 24, 2025
11 checks passed

ada4a deleted the non_canonical_impls branch September 24, 2025 14:22

non_canonical_impls: split the main check function #15520

non_canonical_impls: split the main check function #15520

Uh oh!

Conversation

ada4a commented Aug 20, 2025

Uh oh!

rustbot commented Aug 20, 2025

Uh oh!

ada4a commented Aug 20, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

blyxyas commented Aug 24, 2025

Uh oh!

blyxyas commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ada4a commented Sep 5, 2025

Uh oh!

blyxyas commented Sep 5, 2025

Uh oh!

ada4a commented Sep 5, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blyxyas Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ada4a Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ada4a commented Sep 21, 2025

Uh oh!

blyxyas left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blyxyas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blyxyas left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jarcho Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented Sep 23, 2025

Uh oh!

ada4a commented Sep 23, 2025

Uh oh!

blyxyas commented Sep 24, 2025

Uh oh!

ada4a commented Sep 24, 2025

Uh oh!

blyxyas left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

`non_canonical_impls`: split the main `check` function #15520

`non_canonical_impls`: split the main `check` function #15520

blyxyas commented Sep 5, 2025 •

edited

Loading

blyxyas Sep 22, 2025 •

edited

Loading

ada4a Sep 22, 2025 •

edited

Loading

blyxyas left a comment •

edited by rustbot

Loading

blyxyas left a comment •

edited

Loading

blyxyas left a comment •

edited by rustbot

Loading

Jarcho Sep 23, 2025 •

edited

Loading

blyxyas left a comment •

edited by rustbot

Loading