Limit impl_trait_header query to only trait impls #144607

camsteffen · 2025-07-28T21:17:04Z

Changes impl_trait_header to panic on inherent impls intstead of returning None. A few downstream functions are split into option and non-option returning functions. This gets rid of a lot of unwraps where we know we have a trait impl, while there are still some cases where the Option is helpful.

Summary of changes to tcx methods:

impl_is_of_trait (new)
impl_trait_header -> impl_trait_header/impl_opt_trait_header
impl_trait_ref -> impl_trait_ref/impl_opt_trait_ref
trait_id_of_impl -> impl_trait_id/impl_opt_trait_id

rustbot · 2025-07-28T21:17:09Z

r? @jackh726

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

rustbot · 2025-07-28T21:17:12Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred in compiler/rustc_sanitizers

cc @rcvalle

HIR ty lowering was modified

cc @fmease

lqd · 2025-07-28T21:41:34Z

@bors try @rust-timer queue

Limit impl_trait_header query to only trait impls

rust-bors · 2025-07-28T21:41:39Z

⌛ Trying commit c4b4025 with merge a9207a1…

To cancel the try build, run the command @bors try cancel.

rust-bors · 2025-07-28T22:04:01Z

💔 Test failed (CI). Failed jobs:

bors build finished (web logs, extended logs)
try - dist-x86_64-linux (web logs, extended logs)

camsteffen · 2025-07-28T22:35:44Z

@bors try @rust-timer queue

rust-bors · 2025-07-28T22:35:46Z

@camsteffen: 🔑 Insufficient privileges: not in try users

lqd · 2025-07-29T05:25:25Z

@bors try @rust-timer queue

Limit impl_trait_header query to only trait impls

rust-bors · 2025-07-29T05:25:30Z

⌛ Trying commit 35dbd1a with merge 60b45cb…

To cancel the try build, run the command @bors try cancel.

rust-bors · 2025-07-29T07:42:34Z

☀️ Try build successful (CI)
Build commit: 60b45cb (60b45cbaa7a7f0ea8340063c9bbac4ed081c245a, parent: cdccba87bf9ad5d87c1417eeb8fa52a4b614bbf9)

rust-timer · 2025-07-29T08:27:53Z

Finished benchmarking commit (60b45cb): comparison URL.

Overall result: ❌✅ regressions and improvements - BENCHMARK(S) FAILED

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

❗ ❗ ❗ ❗ ❗
Warning ⚠️: The following benchmark(s) failed to build:

eza-0.21.2
externs
include-blob
clap_derive-4.5.32
serde-1.0.219
wg-grammar
syn-2.0.101
unify-linearly
ucd
serde_derive-1.0.219
cranelift-codegen-0.119.0
regression-31157
diesel-2.2.10
bitmaps-3.2.1
hyper-1.6.0
ripgrep-14.1.1
tuple-stress
match-stress
ctfe-stress-5
derive
deeply-nested-multi
helloworld
unicode-normalization-0.1.24
wf-projection-stress-65510
coercions
stm32f4-0.15.1
issue-58319
html5ever-0.31.0
projection-caching
token-stream-stress
tt-muncher
serde-1.0.219-threads4
large-workspace
nalgebra-0.33.0
many-assoc-items
issue-46449
image-0.25.6
cargo-0.87.1
typenum-1.18.0
unused-warnings
await-call-tree
regex-automata-0.4.8
issue-88862
deep-vector
libc-0.2.172

❗ ❗ ❗ ❗ ❗

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	2.4%	[0.2%, 3.7%]	9
Regressions ❌ (secondary)	0.5%	[0.4%, 0.5%]	3
Improvements ✅ (primary)	-0.2%	[-0.2%, -0.2%]	1
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.2%]	1
All ❌✅ (primary)	2.1%	[-0.2%, 3.7%]	10

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results (secondary -2.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.6%	[-3.1%, -2.3%]	5
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 469.046s -> 466.887s (-0.46%)
Artifact size: 376.81 MiB -> 376.87 MiB (0.02%)

camsteffen · 2025-07-29T14:02:38Z

Y'all think that's funny? 😤

Fixed the rustdoc issues from above.

camsteffen · 2025-10-13T20:56:51Z

@rustbot ready -S-blocked

fmease · 2025-10-13T21:39:33Z

@bors2 delegate=try

rust-bors · 2025-10-13T21:39:36Z

✌️ @camsteffen, you can now perform try builds on this pull request!

You can now post @bors2 try to start a try build.

fmease · 2025-10-13T21:40:33Z

@bors try @rust-timer queue

Limit impl_trait_header query to only trait impls

rust-bors · 2025-10-13T23:54:19Z

☀️ Try build successful (CI)
Build commit: f97840c (f97840c763a3db50d44f28bf0073d58787a62f3b, parent: ed1d94311e7ed53eabb5667ef577802d88d1aaee)

rust-timer · 2025-10-14T01:35:36Z

Finished benchmarking commit (f97840c): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.2%, 0.2%]	1
Regressions ❌ (secondary)	0.1%	[0.0%, 0.2%]	14
Improvements ✅ (primary)	-0.5%	[-0.8%, -0.2%]	17
Improvements ✅ (secondary)	-1.8%	[-2.6%, -0.1%]	4
All ❌✅ (primary)	-0.5%	[-0.8%, 0.2%]	18

Max RSS (memory usage)

Results (secondary 1.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.0%	[1.0%, 1.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results (secondary -4.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.3%	[-5.7%, -3.4%]	4
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 473.894s -> 472.442s (-0.31%)
Artifact size: 388.15 MiB -> 388.12 MiB (-0.01%)

camsteffen · 2025-10-14T03:16:42Z

Yay! Consistent wins for rustc and some smaller regressions for doc. I think the doc regressions can be won back by continuing with more refactors to avoid the def_kind lookups, but I think that can be a follow-up and this is worth landing as is.

There are one or two things here that could be split out into smaller chunks if that is preferred but I'll leave it to the reviewer.

lcnr

didn't quite look into clippy yet, but a few nits

I quite like this change and think it's a nice improvement

I do feel like it would have been nice to initially simply rename the query to opt_impl_trait_ref and change impl_trait_ref to simply call that query and unwrap so that this change can be separately reviewed and merged.

View changes since this review

lcnr · 2025-10-14T08:30:20Z

compiler/rustc_hir_typeck/src/method/mod.rs

                            // Note: this cannot come from an inherent impl,
                            // because the first probing succeeded.
-                            CandidateSource::Impl(def) => self.tcx.trait_id_of_impl(def),
+                            CandidateSource::Impl(def) => self.tcx.impl_opt_trait_id(def),


given the note, this could just be impl_trait_id, should it nor?

I wasn't totally sure but I'll give it a try.

lcnr · 2025-10-14T08:32:25Z

compiler/rustc_hir_typeck/src/method/suggest.rs

                    CandidateSource::Trait(def_id) => def_id != info.def_id,
                    CandidateSource::Impl(def_id) => {
-                        self.tcx.trait_id_of_impl(def_id) != Some(info.def_id)
+                        self.tcx.impl_opt_trait_id(def_id) != Some(info.def_id)


why does this change from non-optional to optional

trait_id_of_impl is optional. I'm not sure if it's safe to be non-optional.

lcnr · 2025-10-14T08:38:21Z

compiler/rustc_privacy/src/lib.rs

    }

    fn trait_ref(&mut self) -> &mut Self {
        self.in_primary_interface = true;


... we set this to true here and don't reset it to false after... is this a bug?

This seemed odd to me too. I'm not really sure but several functions above this one do the same. self is unused after this function is called so I think I've preserved behavior.

lcnr · 2025-10-14T08:39:21Z

compiler/rustc_privacy/src/lib.rs

+                check.ty();
+                if of_trait {
+                    check.trait_ref();
+                }


this pattern could be something like check.impl_header(of_trait) given that it's being repeated multiple times here

The patterns is repeated once but it's a different function being called. But I could do that to preserve the chaining?

Pushed a commit with this.

camsteffen · 2025-10-14T14:17:14Z

Replied and added some fixup commits that can be squashed.

I do feel like it would have been nice to initially simply rename the query to opt_impl_trait_ref and change impl_trait_ref to simply call that query and unwrap so that this change can be separately reviewed and merged.

Yeah that's fair. I did kinda do that with separate commits except I lumped in some changes to not use opt along with the rename. It seemed silly to do a simple rename that would leave opt().unwrap() so I guess I fixed those with the rename and then decided I may as well throw in more changes.

rustbot assigned jackh726 Jul 28, 2025

This comment has been minimized.

Sign in to view

rust-bors bot added a commit that referenced this pull request Jul 28, 2025

Auto merge of #144607 - camsteffen:impl-trait-header-option, r=<try>

a9207a1

Limit impl_trait_header query to only trait impls

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 28, 2025

camsteffen force-pushed the impl-trait-header-option branch from c4b4025 to 35dbd1a Compare July 28, 2025 22:35

This comment has been minimized.

Sign in to view

rust-bors bot added a commit that referenced this pull request Jul 29, 2025

Auto merge of #144607 - camsteffen:impl-trait-header-option, r=<try>

60b45cb

Limit impl_trait_header query to only trait impls

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jul 29, 2025

camsteffen added 4 commits October 13, 2025 15:18

Remove some impl_opt_trait_header usages

1688cf3

Split impl_(opt_)trait_ref

ff5635d

Remove Option from impl_trait_header

9bf0b5f

Fix clippy for impl_trait_header changes

19fcf89

camsteffen force-pushed the impl-trait-header-option branch from c6eaf7f to 19fcf89 Compare October 13, 2025 20:19

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Oct 13, 2025

rust-lang deleted a comment from rust-bors bot Oct 13, 2025

rust-lang deleted a comment from bors Oct 13, 2025

rust-lang deleted a comment from rust-bors bot Oct 13, 2025

This comment has been minimized.

Sign in to view

rust-bors bot added a commit that referenced this pull request Oct 13, 2025

Auto merge of #144607 - camsteffen:impl-trait-header-option, r=<try>

f97840c

Limit impl_trait_header query to only trait impls

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 13, 2025

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 14, 2025

lcnr reviewed Oct 14, 2025

View reviewed changes

lcnr assigned lcnr and unassigned jackh726 Oct 14, 2025

camsteffen added 2 commits October 14, 2025 09:10

fixup! Split trait_id_of_impl into impl(_opt)_trait_id

223e868

Preserve chaining in rustc_privacy

84cdf85

Limit impl_trait_header query to only trait impls #144607

Are you sure you want to change the base?

Limit impl_trait_header query to only trait impls #144607

Conversation

camsteffen commented Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jul 28, 2025

Uh oh!

rustbot commented Jul 28, 2025

Uh oh!

This comment has been minimized.

lqd commented Jul 28, 2025

Uh oh!

This comment has been minimized.

rust-bors bot commented Jul 28, 2025

Uh oh!

rust-bors bot commented Jul 28, 2025

Uh oh!

camsteffen commented Jul 28, 2025

Uh oh!

rust-bors bot commented Jul 28, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

lqd commented Jul 29, 2025

Uh oh!

This comment has been minimized.

rust-bors bot commented Jul 29, 2025

Uh oh!

rust-bors bot commented Jul 29, 2025

Uh oh!

This comment has been minimized.

rust-timer commented Jul 29, 2025

Overall result: ❌✅ regressions and improvements - BENCHMARK(S) FAILED

Uh oh!

camsteffen commented Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

camsteffen commented Oct 13, 2025

Uh oh!

fmease commented Oct 13, 2025

Uh oh!

rust-bors bot commented Oct 13, 2025

Uh oh!

fmease commented Oct 13, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

rust-bors bot commented Oct 13, 2025

Uh oh!

This comment has been minimized.

rust-timer commented Oct 14, 2025

Overall result: ❌✅ regressions and improvements - please read the text below

Uh oh!

camsteffen commented Oct 14, 2025

Uh oh!

lcnr 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!

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!

camsteffen commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

camsteffen commented Jul 28, 2025 •

edited

Loading

camsteffen commented Jul 29, 2025 •

edited

Loading

lcnr left a comment •

edited by rustbot

Loading

camsteffen commented Oct 14, 2025 •

edited

Loading