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.

Zalathar · 2026-03-12T08:53:28Z

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.

Given that the variant constructor is already being passed around as a fn, I don’t see why we wouldn’t want to put it in the vtable.

rustbot · 2026-03-12T20:50:11Z

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.

rustbot assigned mati865 Mar 6, 2026