Rework rustc_query_impl::define_queries.
#152459
Conversation
To make it more similar to `rustc_middle::define_callbacks`, because I think it's very helpful for the two macros to work similarly. `define_queries` currently generates one module called `query_impl::$name` for every query, and also generates a few types/consts/funcs into crate root. After this change, it instead generates one module called `queries::$name` for every query, and also generates those few types/consts/funcs into `queries`. This makes it much more like `define_callbacks`. The commit also adjusts imports so the body of `define_queries` needs fewer qualifiers, and to minimize potential confusion between `rustc_middle::queries` and `rustc_query_impl::queries`. And removes some unnecessary `$crate`/`crate` qualifiers.
rustbot
added
A-query-system
|
☔ The latest upstream changes (presumably #151943) made this pull request unmergeable. Please resolve the merge conflicts. |
| use $name::QueryType; | ||
| plumbing::make_dep_kind_vtable_for_query::<QueryType<'tcx>, _, _>( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| rustc_middle::rustc_with_all_queries! { define_queries! } | ||
| pub mod queries { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
3 participants
To make it more similar to
rustc_middle::define_callbacks, because I think it's very helpful for the two macros to work similarly.define_queriescurrently generates one module calledquery_impl::$namefor every query, and also generates a few types/consts/funcs into crate root.After this change, it instead generates one module called
queries::$namefor every query, and also generates those few types/consts/funcs intoqueries. This makes it much more likedefine_callbacks.The commit also adjusts imports so the body of
define_queriesneeds fewer qualifiers, and to minimize potential confusion betweenrustc_middle::queriesandrustc_query_impl::queries. And removes some unnecessary$crate/cratequalifiers.r? @Zalathar