Skip to content

Rework rustc_query_impl::define_queries. #152459

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

Open
nnethercote wants to merge 1 commit into
base: main
Choose a base branch
from
+71 −70
Open

Rework rustc_query_impl::define_queries. #152459

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/rustc_interface/src/passes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -991,7 +991,7 @@ pub fn create_and_enter_global_ctxt<T, F: for<'tcx> FnOnce(TyCtxt<'tcx>) -> T>(
hir_arena,
untracked,
dep_graph,
rustc_query_impl::make_dep_kind_vtables(arena),
rustc_query_impl::queries::make_dep_kind_vtables(arena),
rustc_query_impl::query_system(
providers.queries,
providers.extern_queries,
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_middle/src/query/plumbing.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,14 +270,15 @@ macro_rules! separate_provide_extern_default {
};
}

// See also `define_queries!`.
macro_rules! define_callbacks {
(
$(
$(#[$attr:meta])*
[$($modifiers:tt)*] fn $name:ident($($K:tt)*) -> $V:ty,
)*
) => {
$(#[allow(unused_lifetimes)] pub mod $name {
$(pub mod $name {
use super::*;
use $crate::query::erase::{self, Erased};

Expand Down
11 changes: 7 additions & 4 deletions compiler/rustc_query_impl/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,11 @@ use rustc_data_structures::sync::AtomicU64;
use rustc_middle::arena::Arena;
use rustc_middle::dep_graph::{self, DepKind, DepKindVTable, DepNode, DepNodeIndex};
use rustc_middle::queries::{
self, ExternProviders, Providers, QueryCaches, QueryEngine, QueryStates,
ExternProviders, PerQueryVTables, Providers, QueryCaches, QueryEngine, QueryStates,
};
use rustc_middle::query::AsLocalKey;
use rustc_middle::query::on_disk_cache::{CacheEncoder, EncodedDepNodeIndex, OnDiskCache};
use rustc_middle::query::plumbing::{HashResult, QuerySystem, QuerySystemFns, QueryVTable};
use rustc_middle::query::values::Value;
use rustc_middle::ty::TyCtxt;
use rustc_query_system::dep_graph::SerializedDepNodeIndex;
use rustc_query_system::query::{
Expand All @@ -31,6 +30,7 @@ pub use crate::plumbing::{QueryCtxt, query_key_hash_verify_all};
use crate::plumbing::{encode_all_query_results, try_mark_green};
use crate::profiling_support::QueryKeyStringCache;
pub use crate::profiling_support::alloc_self_profile_query_strings;
use crate::queries::{engine, make_query_vtables};

mod error;
mod execution;
Expand Down Expand Up @@ -205,7 +205,7 @@ impl<'tcx, C: QueryCache, const FLAGS: QueryFlags> SemiDynamicQueryDispatcher<'t
/// expansion.
///
/// There is one macro-generated implementation of this trait for each query,
/// on the type `rustc_query_impl::query_impl::$name::QueryType`.
/// on the type `rustc_query_impl::queries::$name::QueryType`.
trait QueryDispatcherUnerased<'tcx, C: QueryCache, const FLAGS: QueryFlags> {
type UnerasedValue;

Expand Down Expand Up @@ -239,7 +239,10 @@ pub fn query_system<'tcx>(
}
}

rustc_middle::rustc_with_all_queries! { define_queries! }
pub mod queries {
Copy link
Member

@Zalathar Zalathar Feb 11, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Calling this module queries seems very bad to me, because it results in maximum confusion between rustc_middle and rustc_query_impl.

Working with macro-generated code is inherently hard, which is why I always prefer to have distinctive “landmarks” to help keep things separate in my mind.

I also don't see a benefit in moving the module declaration out of the macro, because now it's less obvious that the module's contents are defined by the macro.

Copy link
Contributor Author

@nnethercote nnethercote Feb 11, 2026

Choose a reason for hiding this comment

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

The double use of queries has pros (symmetry) and cons (potential confusion), and I'm not wedded to it.

Moving the module declaration makes define_callbacks and define_queries more similar, which is the primary motivation of this whole PR. Does increasing similarity of the macros seem valuable to you?

use super::*;
rustc_middle::rustc_with_all_queries! { define_queries! }
}

pub fn provide(providers: &mut rustc_middle::util::Providers) {
providers.hooks.alloc_self_profile_query_strings = alloc_self_profile_query_strings;
Expand Down
123 changes: 60 additions & 63 deletions compiler/rustc_query_impl/src/plumbing.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ use rustc_span::def_id::LOCAL_CRATE;

use crate::error::{QueryOverflow, QueryOverflowNote};
use crate::execution::{all_inactive, force_query};
use crate::{QueryDispatcherUnerased, QueryFlags, SemiDynamicQueryDispatcher};
use crate::{QueryDispatcherUnerased, QueryFlags, SemiDynamicQueryDispatcher, queries};

/// Implements [`QueryContext`] for use by [`rustc_query_system`], since that
/// crate does not have direct access to [`TyCtxt`].
Expand Down Expand Up @@ -151,7 +151,7 @@ impl<'tcx> QueryContext<'tcx> for QueryCtxt<'tcx> {
let mut jobs = QueryMap::default();
let mut complete = true;

for gather_fn in crate::PER_QUERY_GATHER_ACTIVE_JOBS_FNS.iter() {
for gather_fn in queries::PER_QUERY_GATHER_ACTIVE_JOBS_FNS.iter() {
if gather_fn(self.tcx, &mut jobs, require_complete).is_none() {
complete = false;
}
Expand Down Expand Up @@ -190,15 +190,15 @@ pub(super) fn encode_all_query_results<'tcx>(
encoder: &mut CacheEncoder<'_, 'tcx>,
query_result_index: &mut EncodedDepNodeIndex,
) {
for encode in super::ENCODE_QUERY_RESULTS.iter().copied().flatten() {
for encode in queries::ENCODE_QUERY_RESULTS.iter().copied().flatten() {
encode(tcx, encoder, query_result_index);
}
}

pub fn query_key_hash_verify_all<'tcx>(tcx: TyCtxt<'tcx>) {
if tcx.sess().opts.unstable_opts.incremental_verify_ich || cfg!(debug_assertions) {
tcx.sess.time("query_key_hash_verify_all", || {
for verify in super::QUERY_KEY_HASH_VERIFY.iter() {
for verify in queries::QUERY_KEY_HASH_VERIFY.iter() {
verify(tcx);
}
})
Expand Down Expand Up @@ -573,6 +573,8 @@ macro_rules! expand_if_cached {
};
}

// See also `define_callbacks!`.
//
// NOTE: `$V` isn't used here, but we still need to match on it so it can be passed to other macros
// invoked by `rustc_with_all_queries`.
macro_rules! define_queries {
Expand All @@ -582,11 +584,11 @@ macro_rules! define_queries {
[$($modifiers:tt)*] fn $name:ident($($K:tt)*) -> $V:ty,
)*
) => {

pub(crate) mod query_impl { $(pub(crate) mod $name {
use super::super::*;
$(pub(crate) mod $name {
use super::*;
use std::marker::PhantomData;
use ::rustc_middle::query::erase::{self, Erased};
use rustc_middle::query::erase::{self, Erased};
use rustc_middle::queries::$name::{Key, Storage, Value, provided_to_erased};

pub(crate) mod get_query_incr {
use super::*;
Expand All @@ -597,9 +599,9 @@ macro_rules! define_queries {
pub(crate) fn __rust_end_short_backtrace<'tcx>(
tcx: TyCtxt<'tcx>,
span: Span,
key: queries::$name::Key<'tcx>,
key: Key<'tcx>,
mode: QueryMode,
) -> Option<Erased<queries::$name::Value<'tcx>>> {
) -> Option<Erased<Value<'tcx>>> {
#[cfg(debug_assertions)]
let _guard = tracing::span!(tracing::Level::TRACE, stringify!($name), ?key).entered();
execution::get_query_incr(
Expand All @@ -619,9 +621,9 @@ macro_rules! define_queries {
pub(crate) fn __rust_end_short_backtrace<'tcx>(
tcx: TyCtxt<'tcx>,
span: Span,
key: queries::$name::Key<'tcx>,
key: Key<'tcx>,
__mode: QueryMode,
) -> Option<Erased<queries::$name::Value<'tcx>>> {
) -> Option<Erased<Value<'tcx>>> {
Some(execution::get_query_non_incr(
QueryType::query_dispatcher(tcx),
QueryCtxt::new(tcx),
Expand All @@ -635,7 +637,6 @@ macro_rules! define_queries {
/// function pointer in the query's vtable.
mod compute_fn {
use super::*;
use ::rustc_middle::queries::$name::{Key, Value, provided_to_erased};

/// This function would be named `compute`, but we also want it
/// to mark the boundaries of an omitted region in backtraces.
Expand All @@ -657,9 +658,13 @@ macro_rules! define_queries {
}
}

pub(crate) fn make_query_vtable<'tcx>()
-> QueryVTable<'tcx, queries::$name::Storage<'tcx>>
{
pub(crate) fn make_query_vtable<'tcx>() -> QueryVTable<'tcx, Storage<'tcx>> {
// These imports are unused for queries where `should_ever_cache_on_disk!` is false.
#[allow(unused)]
use rustc_middle::queries::$name::ProvidedValue;
#[allow(unused)]
use rustc_middle::queries::cached;

QueryVTable {
name: stringify!($name),
eval_always: is_eval_always!([$($modifiers)*]),
Expand All @@ -668,7 +673,7 @@ macro_rules! define_queries {
query_state: std::mem::offset_of!(QueryStates<'tcx>, $name),
query_cache: std::mem::offset_of!(QueryCaches<'tcx>, $name),
will_cache_on_disk_for_key_fn: should_ever_cache_on_disk!([$($modifiers)*] {
Some(queries::cached::$name)
Some(cached::$name)
} {
None
}),
Expand All @@ -677,34 +682,34 @@ macro_rules! define_queries {
try_load_from_disk_fn: should_ever_cache_on_disk!([$($modifiers)*] {
Some(|tcx, key, prev_index, index| {
// Check the `cache_on_disk_if` condition for this key.
if !queries::cached::$name(tcx, key) {
if !cached::$name(tcx, key) {
return None;
}

let value: queries::$name::ProvidedValue<'tcx> =
$crate::plumbing::try_load_from_disk(tcx, prev_index, index)?;
let value: ProvidedValue<'tcx> =
plumbing::try_load_from_disk(tcx, prev_index, index)?;

// Arena-alloc the value if appropriate, and erase it.
Some(queries::$name::provided_to_erased(tcx, value))
Some(provided_to_erased(tcx, value))
})
} {
None
}),
is_loadable_from_disk_fn: should_ever_cache_on_disk!([$($modifiers)*] {
Some(|tcx, key, index| -> bool {
::rustc_middle::queries::cached::$name(tcx, key) &&
$crate::plumbing::loadable_from_disk(tcx, index)
cached::$name(tcx, key) && plumbing::loadable_from_disk(tcx, index)
})
} {
None
}),
value_from_cycle_error: |tcx, cycle, guar| {
let result: queries::$name::Value<'tcx> = Value::from_cycle_error(tcx, cycle, guar);
let result: Value<'tcx> =
rustc_middle::query::values::Value::from_cycle_error(tcx, cycle, guar);
erase::erase_val(result)
},
hash_result: hash_result!([$($modifiers)*][queries::$name::Value<'tcx>]),
format_value: |value| format!("{:?}", erase::restore_val::<queries::$name::Value<'tcx>>(*value)),
description_fn: $crate::queries::_description_fns::$name,
hash_result: hash_result!([$($modifiers)*][Value<'tcx>]),
format_value: |value| format!("{:?}", erase::restore_val::<Value<'tcx>>(*value)),
description_fn: rustc_middle::queries::_description_fns::$name,
}
}

Expand All @@ -719,27 +724,23 @@ macro_rules! define_queries {
is_feedable: feedable!([$($modifiers)*]),
};

impl<'tcx> QueryDispatcherUnerased<'tcx, queries::$name::Storage<'tcx>, FLAGS>
for QueryType<'tcx>
{
type UnerasedValue = queries::$name::Value<'tcx>;
impl<'tcx> QueryDispatcherUnerased<'tcx, Storage<'tcx>, FLAGS> for QueryType<'tcx> {
type UnerasedValue = Value<'tcx>;

const NAME: &'static &'static str = &stringify!($name);

#[inline(always)]
fn query_dispatcher(tcx: TyCtxt<'tcx>)
-> SemiDynamicQueryDispatcher<'tcx, queries::$name::Storage<'tcx>, FLAGS>
-> SemiDynamicQueryDispatcher<'tcx, Storage<'tcx>, FLAGS>
{
SemiDynamicQueryDispatcher {
vtable: &tcx.query_system.query_vtables.$name,
}
}

#[inline(always)]
fn restore_val(value: <queries::$name::Storage<'tcx> as QueryCache>::Value)
-> Self::UnerasedValue
{
erase::restore_val::<queries::$name::Value<'tcx>>(value)
fn restore_val(value: <Storage<'tcx> as QueryCache>::Value) -> Self::UnerasedValue {
erase::restore_val::<Value<'tcx>>(value)
}
}

Expand All @@ -753,11 +754,11 @@ macro_rules! define_queries {
) -> Option<()> {
let make_frame = |tcx: TyCtxt<'tcx>, key| {
let vtable = &tcx.query_system.query_vtables.$name;
$crate::plumbing::create_deferred_query_stack_frame(tcx, vtable, key)
plumbing::create_deferred_query_stack_frame(tcx, vtable, key)
};

// Call `gather_active_jobs_inner` to do the actual work.
let res = crate::execution::gather_active_jobs_inner(&tcx.query_system.states.$name,
let res = execution::gather_active_jobs_inner(&tcx.query_system.states.$name,
tcx,
make_frame,
qmap,
Expand All @@ -780,7 +781,7 @@ macro_rules! define_queries {
tcx: TyCtxt<'tcx>,
string_cache: &mut QueryKeyStringCache
) {
$crate::profiling_support::alloc_self_profile_query_strings_for_query_cache(
profiling_support::alloc_self_profile_query_strings_for_query_cache(
tcx,
stringify!($name),
&tcx.query_system.caches.$name,
Expand All @@ -794,12 +795,8 @@ macro_rules! define_queries {
encoder: &mut CacheEncoder<'_, 'tcx>,
query_result_index: &mut EncodedDepNodeIndex
) {
$crate::plumbing::encode_query_results::<
query_impl::$name::QueryType<'tcx>,
_,
_
> (
query_impl::$name::QueryType::query_dispatcher(tcx),
plumbing::encode_query_results::<$name::QueryType<'tcx>, _, _> (
$name::QueryType::query_dispatcher(tcx),
QueryCtxt::new(tcx),
encoder,
query_result_index,
Expand All @@ -808,29 +805,29 @@ macro_rules! define_queries {
}}

pub(crate) fn query_key_hash_verify<'tcx>(tcx: TyCtxt<'tcx>) {
$crate::plumbing::query_key_hash_verify(
query_impl::$name::QueryType::query_dispatcher(tcx),
plumbing::query_key_hash_verify(
$name::QueryType::query_dispatcher(tcx),
QueryCtxt::new(tcx),
)
}
})*}
})*

pub(crate) fn engine(incremental: bool) -> QueryEngine {
if incremental {
QueryEngine {
$($name: query_impl::$name::get_query_incr::__rust_end_short_backtrace,)*
$($name: $name::get_query_incr::__rust_end_short_backtrace,)*
}
} else {
QueryEngine {
$($name: query_impl::$name::get_query_non_incr::__rust_end_short_backtrace,)*
$($name: $name::get_query_non_incr::__rust_end_short_backtrace,)*
}
}
}

pub fn make_query_vtables<'tcx>() -> queries::PerQueryVTables<'tcx> {
queries::PerQueryVTables {
pub(crate) fn make_query_vtables<'tcx>() -> PerQueryVTables<'tcx> {
PerQueryVTables {
$(
$name: query_impl::$name::make_query_vtable(),
$name: $name::make_query_vtable(),
)*
}
}
Expand All @@ -843,27 +840,27 @@ macro_rules! define_queries {
/// (We arbitrarily use the word "gather" when collecting the jobs for
/// each individual query, so that we have distinct function names to
/// grep for.)
const PER_QUERY_GATHER_ACTIVE_JOBS_FNS: &[
pub(crate) const PER_QUERY_GATHER_ACTIVE_JOBS_FNS: &[
for<'tcx> fn(TyCtxt<'tcx>, &mut QueryMap<'tcx>, require_complete: bool) -> Option<()>
] = &[
$(query_impl::$name::gather_active_jobs),*
$($name::gather_active_jobs),*
];

const ALLOC_SELF_PROFILE_QUERY_STRINGS: &[
pub(crate) const ALLOC_SELF_PROFILE_QUERY_STRINGS: &[
for<'tcx> fn(TyCtxt<'tcx>, &mut QueryKeyStringCache)
] = &[$(query_impl::$name::alloc_self_profile_query_strings),*];
] = &[$($name::alloc_self_profile_query_strings),*];

const ENCODE_QUERY_RESULTS: &[
pub(crate) const ENCODE_QUERY_RESULTS: &[
Option<for<'tcx> fn(
TyCtxt<'tcx>,
&mut CacheEncoder<'_, 'tcx>,
&mut EncodedDepNodeIndex)
>
] = &[$(expand_if_cached!([$($modifiers)*], query_impl::$name::encode_query_results)),*];
] = &[$(expand_if_cached!([$($modifiers)*], $name::encode_query_results)),*];

const QUERY_KEY_HASH_VERIFY: &[
pub(crate) const QUERY_KEY_HASH_VERIFY: &[
for<'tcx> fn(TyCtxt<'tcx>)
] = &[$(query_impl::$name::query_key_hash_verify),*];
] = &[$($name::query_key_hash_verify),*];

/// Module containing a named function for each dep kind (including queries)
/// that creates a `DepKindVTable`.
Expand Down Expand Up @@ -969,8 +966,8 @@ macro_rules! define_queries {
}

$(pub(crate) fn $name<'tcx>() -> DepKindVTable<'tcx> {
use $crate::query_impl::$name::QueryType;
$crate::plumbing::make_dep_kind_vtable_for_query::<QueryType<'tcx>, _, _>(
use $name::QueryType;
plumbing::make_dep_kind_vtable_for_query::<QueryType<'tcx>, _, _>(
Comment on lines +969 to +970
Copy link
Member

@Zalathar Zalathar Feb 11, 2026

Choose a reason for hiding this comment

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

I find that this sort of change makes the macro code significantly harder to understand, for no real benefit.

In the old code, it's easy to see that we're referring to things defined in the current crate, inside a module with the uniquely-searchable name query_impl. I can also see that query_impl is defined by the enclosing macro, and that other parts of the macro also refer to query_impl.

In the new code, it's much harder to know what the bare $name or plumbing names refer to. IDE features don't work because this is macro code, and I can't usefully search for those names either.

Copy link
Contributor Author

@nnethercote nnethercote Feb 11, 2026

Choose a reason for hiding this comment

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

I find the opposite, that unnecessary qualification obscures. If I see $name I know it's from the current crate and module. If I see crate::query_impl::$name it seems like it must be from another module.

Copy link
Member

@Zalathar Zalathar Feb 11, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

When we're dealing with 3+ levels of nested macros, expanded cross-module and in some cases cross-crate, I'd prefer not to have to solve a logic puzzle to figure out what and where $name is when the source code could just tell me instead.

In trying to get my head around the query system macros, I never found myself wishing for paths to be less explicit, but very often found myself wishing that they were more explicit.

is_eval_always!([$($modifiers)*]),
)
})*
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_query_impl/src/profiling_support.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ pub fn alloc_self_profile_query_strings(tcx: TyCtxt<'_>) {

let mut string_cache = QueryKeyStringCache::new();

for alloc in super::ALLOC_SELF_PROFILE_QUERY_STRINGS.iter() {
for alloc in crate::queries::ALLOC_SELF_PROFILE_QUERY_STRINGS.iter() {
alloc(tcx, &mut string_cache)
}
tcx.sess.prof.store_query_cache_hits();
Expand Down
Loading