Remove `_description_fns` and `_cache_on_disk_if_fns` modules by nnethercote · Pull Request #153065 · rust-lang/rust

nnethercote · 2026-02-24T23:33:07Z

See #153064 for a description of these modules. This PR removes them. This greatly simplifies the rustc_queries! proc macro (which is hard to read and understand) at the cost of slightly complicating the define_callbacks! declarative macro (which is not easy to read and understand, but certainly easier than the proc macro).

r? @Zalathar

rustbot · 2026-02-24T23:33:12Z

Zalathar is not on the review rotation at the moment.
They may take a while to respond.

jyn514 · 2026-02-24T23:55:04Z

This would benefit from a try run. I don't have permissions I don't think.

jyn514

LGTM as long as perf isn't too much worse.

View changes since this review

Zalathar · 2026-02-25T01:00:00Z

I find these changes puzzling, because the descriptions talk about making things simpler, but looking at the changes I find them to be more complicated or neutral.

E.g. for the proc macro changes, I don’t like the idea of adding more smuggling mechanisms and helper macros, to not even fully eliminate a relatively simple occurrence of quote!.

Zalathar · 2026-02-25T01:00:36Z

@bors try @rust-timer queue

Remove `_description_fns` and `_cache_on_disk_if_fns` modules

nnethercote · 2026-02-25T01:09:48Z

This helps with #153064.

nnethercote · 2026-02-25T01:16:12Z

E.g. for the proc macro changes, I don’t like the idea of adding more smuggling mechanisms and helper macros, to not even fully eliminate a relatively simple occurrence of quote!.

Maybe we're looking at different levels. At a high level, I find it very distasteful that rustc_queries! is a macro that produces four (previously five) different things. And you have to understand most of those things (which are basically invisible) to then fully understand how the declarative macros work. E.g. when you see a mention of _description_fns in define_queries! and you wonder "what is that?" you end up in the proc macro. I'm trying to simplify that. I'm not really considering individual quote! calls. -53 lines of code in the proc macro is what I'm going for.

nnethercote · 2026-02-25T02:32:33Z

Here's another attempt at describing things, in case it's helpful.

The proc macro has to generate a description function for each query.

Currently for query foo the proc macro generates _description_fns::foo and then define_queries can refer to it by that name. This is a bit like making a value available to a function by putting it in a global variable that the function can access.
With this change the proc macro generates a closure and passes it to define_queries (via a modifier), which can use it directly. No _description_fns required. This is a bit like passing a value directly to a function as an argument.

I prefer the "pass directly" approach to the "put it in a global variable" approach. It's simpler, more direct, and doesn't require putting a name in the global namespace.

rust-bors · 2026-02-25T03:12:08Z

☀️ Try build successful (CI)
Build commit: 2495573 (24955730e60c43b9b4554aaabde96b82aa8cb5de, parent: 859951e3c7c9d0322c39bad49221937455bdffcd)

rust-timer · 2026-02-25T03:52:20Z

Finished benchmarking commit (2495573): 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)

Results (secondary -7.2%)

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)	-7.2%	[-7.2%, -7.2%]	2
All ❌✅ (primary)	-	-	0

Cycles

Results (secondary -1.1%)

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)	2.1%	[2.1%, 2.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.7%	[-3.4%, -2.1%]	2
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 481s -> 481.174s (0.04%)
Artifact size: 397.83 MiB -> 395.88 MiB (-0.49%)

`rustc_queries` generates a macro and three modules. One of the modules looks like this: ``` mod _description_fns { ... #[allow(unused_variables)] pub fn hir_module_items<'tcx>(tcx: TyCtxt<'tcx>, key: LocalModDefId) -> String { format!("getting HIR module items in `{}`", tcx.def_path_str(key)) } ... } ``` Members of this module are then used to initialize `QueryVTables::description_fn` fields. This commit removes the `_description_fns` module entirely. For each query we now instead generate a description closure that is used to initialize `QueryVTables::description_fn`. This closure is passed as `(desc { $desc_fn_closure })` in the modifiers list. This makes the handling of the `desc` modifier closer to the handling of the other modifiers (though it's still a bit special). This change simplifies `rustc_queries` quite a bit, eliminating an entire module from its output, at the cost of complicating `define_queries` slightly. This is a good trade-off because the declarative macro is easier to understand than the proc macro.

There are three query vtable functions related to the `cache_on_disk_if` modifier: - `will_cache_on_disk_for_key_fn` - `try_load_from_disk_fn` - `is_loadable_from_disk_fn` These are all function ptrs within an `Option`. They each have a wrapper that returns `false`/`None` if the function ptr is missing. This commit removes the `Option` wrappers. In the `None` case we now set the function ptr to a trivial closure that returns `false`/`None`. The commit also removes some typedefs that each have a single use. All this is a bit simpler, with less indirection. More importantly, it enables the following commit, which will make more related changes.

One of the modules generated by `rustc_queries` looks like this: ``` mod _cache_on_disk_if_fns { ... #[inline] pub fn const_param_default<'tcx>( _: TyCtxt<'tcx>, param: &crate::queries::const_param_default::Key<'tcx> ) -> bool { param.is_local() } ... ``` Members of this module are used to initialize multiple `QueryVTable` fields. This commit removes the `_cache_on_disk_if_fns` module entirely. For each cacheable query we generate a check closure which is passed as `(cache_on_disk_if { $check_closure }` in the modifiers list. This is similar to how `desc` modifiers are handled.

These are now just trivial wrappers around the fn ptrs. Might as well just call the fn ptrs directly.

rustbot · 2026-02-25T08:11:06Z

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.

Zalathar · 2026-02-25T09:47:19Z

Here's my thinking:

There's a small subset of modifiers that require some extra parsing and postprocessing that must be done in the proc-macro.
For those modifiers, the amount of postprocessing that could usefully be done in macro-rules is basically nil.
Given that the parsed elements are already available in the proc-macro, the simplest way to put them into a function is to do so directly in the proc-macro.
The end result is a plain function, which is just about the easiest and most natural thing to access from another module.

I don't see what simplicity is supposed to be gained by constructing a closure in the proc macro, inserting those closure tokens back in the modifier list, writing a novel and unintuitive desc_fn! macro, and then having the use site (setting description_fn) convey nothing at all of what's really going on. That's a lot of non-obvious hoop-jumping to avoid doing the straightforward thing.

The tokens are right there in the proc-macro already; we can just assemble them into a finished function that can be used to initialize description_fn in a very obvious and normal way.

Zalathar · 2026-02-25T10:03:26Z

From another perspective, it seems to me that the philosophy of this PR is “uniformity at all costs”, and I think the costs to simplicity and clarity are higher than the benefit from uniformity.

petrochenkov · 2026-02-25T11:39:56Z

The second commit (33e27c7) basically reverts #151736, another recent PR.

To me the "trivial closure" approach looks simpler than options, but it potentially causes instantiation bloat and unnecessary function pointer calls, but if rust-perf says it's there's no difference, then it should be fine.

petrochenkov · 2026-02-25T11:58:51Z

compiler/rustc_query_impl/src/lib.rs

 #![feature(try_blocks)]
 // tidy-alphabetical-end

+use rustc_ast::tokenstream::TokenStream;


It's a bit weird to see TokenStream here, where is it used? I don't see any uses of in in rustc_query_impl.

TokenStream is used in some query keys. The generated closure had to type-annotate its arguments, unfortunately. And rustc_middle::rustc_with_all_queries! { define_queries! } appears in this file.

nnethercote · 2026-02-25T23:30:54Z

The second commit (33e27c7) basically reverts #151736, another recent PR.

Looking back, there were some good things in there (e.g. reducing four vtable functions down to three) but in hindsight I don't like the addition of the Option and the type synonyms.

nnethercote · 2026-02-25T23:42:23Z

The tokens are right there in the proc-macro already; we can just assemble them into a finished function that can be used to initialize description_fn in a very obvious and normal way.

The proc-macro has to assemble a fn-like thing. Currently that's a function, which requires a name and a special underscore-prefixed module to put it in. I'm instead proposing a closure, which doesn't require a name or a special module. (It does require adjusting the modifiers; I think the cost of that adjustment is outweighed by the benefits of shrinking the proc macro.)

You know what got me to work on this? This comment in the code:

        // FIXME(Zalathar): Instead of declaring these functions directly, can
        // we put them in a macro and then expand that macro downstream in
        // `rustc_query_impl`, where the functions are actually used?
        pub mod _cache_on_disk_if_fns {
            use super::*;
            #cache_on_disk_if_fns_stream
        }

This suggests that you had some concerns about the existing approach, and I agreed. But I find macros that generates other macros (e.g. define_dep_nodes! generates make_dep_kind_array) very spaghetti, so I found a different way to do it, piggy-backing off the existing modifier mechanism. It's a net reduction of 65 lines of code. I think it's good.

Zalathar · 2026-02-26T02:23:17Z

Looking back, there were some good things in there (e.g. reducing four vtable functions down to three) but in hindsight I don't like the addition of the Option and the type synonyms.

The type synonyms can definitely go. I found them useful at the time, but they haven't aged well.

I personally like the Option here, because it allows non-macro code to indicate that there's a trivial special case for non-disk-cached queries, without having to wade through macros to find the stub implementation.

(But this I feel less strongly about. I do think removing the options would be net worse, but it's not the end of the world, and I guess we could compensate by documenting the special case on the vtable field instead.)

Zalathar · 2026-02-26T02:39:56Z

compiler/rustc_query_impl/src/plumbing.rs

-                    description_fn: $crate::queries::_description_fns::$name,
+                    format_value: |value|
+                        format!("{:?}", erase::restore_val::<queries::$name::Value<'tcx>>(*value)),
+                    description_fn: desc_fn!([$($modifiers)*]),


To me, this seems like a clear downgrade in understandability.

Instead of seeing a reference to a _description_fns module that I can search for, all I see is an opaque desc_fn! call.

And if I look at the definition of that helper macro, it gives very little indication of what's happening. At least the other helper macros can eventually be understood as a necessary idiom for checking a boolean condition.

In order to understand what's going on here, I have to know every part of what the proc macro is doing, and what the smuggling mechanism is doing. And it's harder to chase down that chain, because there are fewer clues or landmarks along the way.

(I personally do understand, but I'm trying to put myself in the shoes of someone seeing this for the first time. To me, this is the kind of complexity and indirection that I found very frustrating when trying to unravel the query plumbing macros.)

Zalathar · 2026-02-26T02:52:00Z

This suggests that you had some concerns about the existing approach, and I agreed. But I find macros that generates other macros (e.g. define_dep_nodes! generates make_dep_kind_array) very spaghetti, so I found a different way to do it, piggy-backing off the existing modifier mechanism. It's a net reduction of 65 lines of code. I think it's good.

All I had in mind when writing that FIXME was that it might be good to expand those definitions in a downstream crate to reduce the compile time of rustc_middle. But only if it turns out to be worth the cost of adding another (relatively simple) layer of macro indirection.

I have no problem with the _description_fns and _cache_on_disk_if_fns modules existing. To me they serve a useful purpose, and replacing them with something else is a total non-goal.

(I do think that the way rustc_macros::query builds those modules is a bit more spaghetti than necessary, but that's something that can be addressed by internal tweaks to the proc-macro definition.)

I'm definitely concerned about macro spaghetti, but that's a big part of why I find this PR so confusing. The main thing I see is an increase in macro spaghetti, in order to “fix” something that I don't see as a problem at all.

rust-bors · 2026-02-26T08:16:57Z

☔ The latest upstream changes (presumably #153116) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot assigned Zalathar Feb 24, 2026

jyn514 approved these changes Feb 24, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

rust-bors bot pushed a commit that referenced this pull request Feb 25, 2026

Auto merge of #153065 - nnethercote:rm-desc-cache-modules, r=<try>

2495573

Remove `_description_fns` and `_cache_on_disk_if_fns` modules

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 25, 2026

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 Feb 25, 2026

This comment has been minimized.

Sign in to view

nnethercote added 4 commits February 25, 2026 17:31

Remove trivial QueryVTable methods.

9cc3fcd

These are now just trivial wrappers around the fn ptrs. Might as well just call the fn ptrs directly.

nnethercote force-pushed the rm-desc-cache-modules branch from f52773b to 9cc3fcd Compare February 25, 2026 08:11

petrochenkov reviewed Feb 25, 2026

View reviewed changes

Zalathar reviewed Feb 26, 2026

View reviewed changes

Uh oh!

Conversation

nnethercote commented Feb 24, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Feb 24, 2026

Uh oh!

jyn514 commented Feb 24, 2026

Uh oh!

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

Zalathar commented Feb 25, 2026

Uh oh!

Zalathar commented Feb 25, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

nnethercote commented Feb 25, 2026

Uh oh!

nnethercote commented Feb 25, 2026

Uh oh!

nnethercote commented Feb 25, 2026

Uh oh!

rust-bors bot commented Feb 25, 2026

Uh oh!

This comment has been minimized.

rust-timer commented Feb 25, 2026

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

This comment has been minimized.

rustbot commented Feb 25, 2026

Uh oh!

Zalathar commented Feb 25, 2026

Uh oh!

Zalathar commented Feb 25, 2026

Uh oh!

petrochenkov commented Feb 25, 2026

Uh oh!

petrochenkov Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

nnethercote Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

nnethercote commented Feb 25, 2026

Uh oh!

nnethercote commented Feb 25, 2026

Uh oh!

Zalathar commented Feb 26, 2026

Uh oh!

Zalathar Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Zalathar commented Feb 26, 2026

Uh oh!

rust-bors bot commented Feb 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nnethercote commented Feb 24, 2026 •

edited by rustbot

Loading

jyn514 left a comment •

edited by rustbot

Loading