Add a `TaggedQueryKey` to identify a query instance by Zoxc · Pull Request #153492 · rust-lang/rust

Zoxc · 2026-03-06T11:52:59Z

This adds back a TaggedQueryKey enum which represents a query kind and the associated key. This is used to replace QueryStackDeferred and QueryStackFrameExtra and the associated lifting operation for cycle errors

This approach has a couple of benefits:

We only run description queries when printing the query stack trace in the panic handler
The unsafe code for QueryStackDeferred is removed
Cycle handles have access to query keys, which may be handy

Some further work could be replacing QueryStackFrame with TaggedQueryKey as the extra information can be derived from it.

rustbot · 2026-03-06T11:53:05Z

r? @mati865

rustbot has assigned @mati865.
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: compiler, query-system
compiler, query-system expanded to 69 candidates
Random selection from 16 candidates

Zoxc · 2026-03-06T11:55:12Z

cc @nnethercote @Zalathar

Zalathar · 2026-03-06T12:42:37Z

If we can get rid of a lot of the QueryStackFrame complexity by just storing the query key in an enum, that seems great.

The name QueryKeyId seems strange to me, though. It’s not the ID of a query key; it’s the key itself, “tagged” with the query that it belongs to.

Zoxc · 2026-03-06T13:09:14Z

The name QueryKeyId seems strange to me, though. It’s not the ID of a query key; it’s the key itself, “tagged” with the query that it belongs to.

Yeah, QueryId was taken, maybe QueryInstance is better?

nnethercote · 2026-03-06T23:09:14Z

This looks great! I hate the deferred/extra split, this seems like a really clean way to eliminate it.

@Zoxc: can you expand the first paragraph of the PR description to explain (a) the current situation, and (b) how this PR changes that situation? I've skimmed the changes and they look good, but I will need to look again more closely on Monday to get my head fully around how the new QueryKeyId is used and how it lets so much of the existing complexity be removed. Extra description will help with that.

cc @zetanumbers

Zalathar · 2026-03-06T23:49:39Z

Yeah, QueryId was taken, maybe QueryInstance is better?

I was thinking of something like TaggedQueryKey, because the value being stored is a query key, tagged with its corresponding query.

I think it's easier to say “this is a key that also indicates what query it came from” than to try to come up with an intuitive name for query+key.

Or we could go for something like QueryAndKey or QueryWithKey.

nnethercote · 2026-03-07T00:01:06Z

TaggedQueryKey sounds good to me -- that's exactly what it is!

nnethercote · 2026-03-10T11:07:02Z

Just a little more work required to get this into shape.

@rustbot author

rustbot · 2026-03-10T11:07:06Z

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

nnethercote

Getting closer! Please squash the two commits together.

View changes since this review

nnethercote · 2026-03-12T03:11:43Z

compiler/rustc_middle/src/query/plumbing.rs

+                }
+                match self {
+                    $(
+                        TaggedQueryKey::$name(key) => inner(key, tcx),


Why have inner? Why not just call key.key_as_def_id()... here?

It reduces code generation as inner can be reused for shared key types.

Can you add a comment explaining this?

nnethercote · 2026-03-12T03:15:34Z

compiler/rustc_middle/src/query/stack.rs

-    pub info: I,
-
+pub struct QueryStackFrame<'tcx> {
+    pub id: TaggedQueryKey<'tcx>,


This field needs renaming. Maybe tagged_key?

It is used as an identifier though?

In the compiler, an “id” is usually an opaque or mostly-opaque integer that indexes into some other table, so I don’t understand the motivation behind calling this value an id.

nnethercote · 2026-03-12T03:16:10Z

compiler/rustc_query_impl/src/execution.rs

    query: &'tcx QueryVTable<'tcx, C>,
-    tcx: TyCtxt<'tcx>,
    require_complete: bool,
+    make_id: fn(C::Key) -> TaggedQueryKey<'tcx>,


This parameter needs renaming. Maybe mk_tagged_key?

nnethercote · 2026-03-12T03:16:33Z

compiler/rustc_query_impl/src/plumbing.rs

    vtable: &'tcx QueryVTable<'tcx, C>,
    key: C::Key,
-) -> QueryStackFrame<QueryStackDeferred<'tcx>>
+    make_id: fn(C::Key) -> TaggedQueryKey<'tcx>,


mk_tagged_key?

Zoxc · 2026-03-12T07:57:06Z

#153685 conflicts a bit as $name is no longer accessible in collect_active_jobs_from_all_queries. Adding a TaggedQueryKey constructor as a closure argument in for_each_query_vtable! doesn't seem very clean either.

Zalathar · 2026-03-12T08:00:46Z

Would it work to add the enum constructor as a function in QueryVTable?

Zoxc · 2026-03-12T08:07:24Z

I think so, but I'd probably prefer reverting the use of for_each_query_vtable! for collect_active_jobs_from_all_queries over that though.

Add a `TaggedQueryKey` to identify a query instance

rust-bors · 2026-03-13T00:26:50Z

☀️ Try build successful (CI)
Build commit: dfc9e7a (dfc9e7ad0cd9465bcd7e5316855d9fc5d6ca90f6, parent: 3102493c71626b5912d11d06c393b013569beb52)

rust-timer · 2026-03-13T01:06:50Z

Finished benchmarking commit (dfc9e7a): comparison URL.

Overall result: no relevant changes - no action needed

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.

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

Instruction count

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

Max RSS (memory usage)

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

Cycles

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

Binary size

Results (primary 0.0%, secondary 0.1%)

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

	mean	range	count
Regressions ❌ (primary)	0.0%	[0.0%, 0.0%]	11
Regressions ❌ (secondary)	0.1%	[0.0%, 0.1%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[0.0%, 0.0%]	11

Bootstrap: 481.658s -> 479.781s (-0.39%)
Artifact size: 396.92 MiB -> 397.11 MiB (0.05%)

Zalathar · 2026-03-13T01:16:12Z

Restoring rollup status since perf is neutral.
@bors rollup=maybe

nnethercote · 2026-03-13T01:55:19Z

I'm very close to giving r+, I just want an answer about the reduced queries check, thanks.

rustbot · 2026-03-13T06:50:18Z

This PR was rebased onto a different main 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.

nnethercote · 2026-03-13T06:55:46Z

Hooray for getting rid of the deferred/extra frame split!

@bors r+ rollup=maybe

rust-bors · 2026-03-13T06:55:49Z

📌 Commit 85967c4 has been approved by nnethercote

It is now in the queue for this repository.

Add a `TaggedQueryKey` to identify a query instance This adds back a `TaggedQueryKey` enum which represents a query kind and the associated key. This is used to replace `QueryStackDeferred` and `QueryStackFrameExtra` and the associated lifting operation for cycle errors This approach has a couple of benefits: - We only run description queries when printing the query stack trace in the panic handler - The unsafe code for `QueryStackDeferred` is removed - Cycle handles have access to query keys, which may be handy Some further work could be replacing `QueryStackFrame` with `TaggedQueryKey` as the extra information can be derived from it.

rust-bors · 2026-03-13T07:25:27Z

💔 Test for 398abb9 failed: CI. Failed job:

auto - aarch64-gnu-debug (web logs, enhanced plaintext logs)

Zalathar · 2026-03-13T07:27:51Z

@bors retry

Rollup of 3 pull requests Successful merges: - #153792 (tests/debuginfo/basic-stepping.rs: Add lldb test) - #153492 (Add a `TaggedQueryKey` to identify a query instance) - #153808 (rustfmt.toml: drop ignore of deleted test)

rust-log-analyzer · 2026-03-13T07:33:45Z

A job failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

rustbot assigned mati865 Mar 6, 2026