Use closures more consistently in `dep_graph.rs`.
This file has several methods that take a `FnOnce() -> R` closure:
- `DepGraph::with_ignore`
- `DepGraph::with_query_deserialization`
- `DepGraph::with_anon_task`
- `DepGraphData::with_anon_task_inner`
It also has two methods that take a faux closure via an `A` argument and a `fn(TyCtxt<'tcx>, A) -> R` argument:
- DepGraph::with_task
- DepGraphData::with_task
The rationale is that the faux closure exercises tight control over what state they have access to. This seems silly when (a) they are passed a `TyCtxt`, and (b) when similar nearby functions take real closures. And they are more awkward to use, e.g. requiring multiple arguments to be gathered into a tuple. This commit changes the faux closures to real closures.
r? @Zalathar
Simplify `try_load_from_disk_fn`.
`try_load_from_disk_fn` has a single call site. We can move some of the stuff within it to its single call site, which simplifies it, and also results in all of the query profiling code ending up in the same module. Details in individual commits.
r? @Zalathar
From `plumbing.rs` to `execution.rs`, which is where most of the other
query profiling occurs. It also avoids eliminates some fn parameters.
This means the `provided_to_erased` call in `try_from_load_disk_fn` is
now included in the profiling when previously it wasn't. This is
good because `provided_to_erased` is included in other profiling calls
(e.g. calls to `invoke_provider_fn`).
It doesn't need to be in there, it can instead be at the single call
site. Removing it eliminates one parameter, makes `define_queries!`
smaller (which is always good), and also enables the next commit which
tidies up profiling.
This commit also changes how `value` and `verify` are initialized,
because I don't like the current way of doing it after the declaration.
Split out the creation of `Cycle` to a new `process_cycle` function
This splits out the creation of `CycleError` to a new `process_cycle` function. This makes it a bit clearer which operations are done for diagnostic purposes vs. what's needed to break cycles.
This modifier indicates that a query has a custom handler for cycles.
That custom handler must be found at
`rustc_query_impl::handle_cycle_error::$name`.
This eliminates the need for `specialize_query_vtables`, which is the
current hack to install custom handlers. It's more lines of code in
total, but indicating special treatment of a query via a modifier in
`queries.rs` is more consistent with how other aspects of queries are
handled.
`rustc_queries` generates a macro and two 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 in `TaggedQueryKey::description`.
This commit removes the `_description_fns` module entirely. For each
query we now instead generate a description closure that is used
instead. This closure is passed in the modifiers list.
This change simplifies `rustc_queries` quite a bit. It requires adding
another query modifier, but query modifiers are how other query-specific
details are already passed to the declarative macros, so it's more
consistent.
This file has several methods that take a `FnOnce() -> R` closure:
- `DepGraph::with_ignore`
- `DepGraph::with_query_deserialization`
- `DepGraph::with_anon_task`
- `DepGraphData::with_anon_task_inner`
It also has two methods that take a faux closure via an `A` argument and
a `fn(TyCtxt<'tcx>, A) -> R` argument:
- DepGraph::with_task
- DepGraphData::with_task
The rationale is that the faux closure exercises tight control over what
state they have access to. This seems silly when (a) they are passed a
`TyCtxt`, and (b) when similar nearby functions take real closures. And
they are more awkward to use, e.g. requiring multiple arguments to be
gathered into a tuple. This commit changes the faux closures to real
closures.
Queries with both `cache_on_disk` and `separate_provide_extern` will only
disk-cache values for local keys.
Other queries with `cache_on_disk` will disk-cache all values unconditionally.
Don't store current-session side effects in `OnDiskCache`
This PR is a series of related cleanups to `OnDiskCache`, which is responsible for loading query return values (and side effects) that were serialized during the previous incremental-compilation session.
The primary change is to move the `current_side_effects` field out of OnDiskCache and into QuerySystem. That field was awkward because it was the only part of OnDiskCache state related to serializing the *current* compilation session, rather than loading values from the previous session.
The other commits should hopefully be straightforward.
r? nnethercote (or compiler)
Clean up query-forcing functions
This PR takes the `force_query` function, inlines it into its only caller `force_from_dep_node_inner`, and renames the resulting function to `force_query_dep_node`. Combining the functions became possible after the removal of `rustc_query_system`, because they are now in the same crate.
There are two other notable cleanups along the way:
- Removing an unhelpful assertion and its verbose comment
- The removed comment was originally added in https://github.com/rust-lang/rust/commit/0454a41, but is too verbose to be worth preserving inline.
- Removing a redundant cache lookup while forcing, as it is useless in the serial compiler and unnecessary (and probably unhelpful) in the parallel compiler
r? nnethercote
Every other part of `OnDiskCache` deals with loading information from the
_previous_ session, except for this one field.
Moving it out to `QuerySystem` makes more sense, because that's also where
query return values are stored (inside the caches in their vtables).
These lists can be considered part of the encoder state, and bundling them
inside the encoder is certainly more convenient than passing them around
separately.
Prior to #154122 it wasn't used on all paths, so we only computed it
when necessary -- sometimes in `check_if_ensure_can_skip_execution`,
sometimes in one of two places in `execute_job_incr` -- and pass around
`Option<DepNode>` to allow this.
But now it's always used, so this commit makes us compute it earlier,
which simplifies things.
- `check_if_ensure_can_skip_execution` can be made simpler, returning a
bool and eliminating the need for `EnsureCanSkip`.
- `execute_job_incr` no longer uses two slightly different methods to
create a `DepNode` (`get_or_insert_with` vs `unwrap_or_else`).
The call chain for a non-incremental query includes the following
functions:
- execute_query_non_incr_inner (assert!)
- try_execute_query (assert!)
- execute_job_non_incr (assert!)
And likewise for an incremental query:
- execute_query_incr_inner (assert!)
- try_execute_query (assert!)
- execute_job_incr (expect)
That is five distinct functions. Every one of them has an `assert!` or
`expect` call that checks that the dep-graph is/is not enabled as
expected. Three cheers for defensive programming but this feels like
overkill, particularly when `execute_job{,_non_incr,_incr}` each have a
single call site.
This commit removes the assertions in `execute_query_*` and
`try_execute_query`, leaving a check in each of the `execute_job_*`
functions.
The existing names have bugged me for a while. Changes:
- `CycleError` -> `Cycle`. Because we normally use "error" for a `Diag`
with `Level::Error`, and this type is just a precursor to that. Also,
many existing locals of this type are already named `cycle`.
- `CycleError::cycle` -> `Cycle::frames`. Because it is a sequence of
frames, and we want to avoid `Cycle::cycle` (and `cycle.cycle`).
- `cycle_error` -> `find_and_handle_cycle`. Because that's what it does.
The existing name is just a non-descript noun phrase.
- `mk_cycle` -> `handle_cycle`. Because it doesn't make the cycle; the
cycle is already made. It handles the cycle, which involves (a)
creating the error, and (b) handling the error.
- `report_cycle` -> `create_cycle_error`. Because that's what it does:
creates the cycle error (i.e. the `Diag`).
- `value_from_cycle_error` -> `handle_cycle_error_fn`. Because most
cases don't produce a value, they just emit an error and quit.
And the `_fn` suffix is for consistency with other vtable fns.
- `from_cycle_error` -> `handle_cycle_error`. A similar story for this
module.
This effectively reverses <https://github.com/rust-lang/rust/commit/4284edc>.
At that time, I thought GetQueryVTable might be useful for other kinds of
static lookup in the future, but after various other simplifications and
cleanups that now seems less likely, and this style is more consistent with
other vtable-related functions.
The different parts of this function used to be split across different crates,
but nowadays they are both in `rustc_query_impl`, so they can be combined.
This commit also hoists the vtable lookup to a closure in
`make_dep_kind_vtable_for_query`, to reduce the amount of code that deals with
`GetQueryVTable`, and to make the inner function more consistent with other
functions in `execution`.
In the serial compiler, we should never be trying to force a query node that
has a cached value, because we would have already marked the node red or green
when putting its value in cache.
In the parallel compiler, we immediately re-check the cache while holding the
state shard lock anyway, and trying to avoid that lock doesn't seem worthwhile.
This assertion and its comment are much visually heavier than the part of the
function that actually performs important work.
This assertion is also useless, because the `codegen_unit` query does not have
a `force_from_dep_node_fn` in its DepKindVTable, so the assertion would never
be reached even in the situation it is trying to guard against.
This assertion is also redundant, because we have plenty of incremental tests
that fail if the corresponding calls to `tcx.ensure_ok().codegen_unit(..)` are
removed.
By removing the early return and using a `match` instead.
- The two paths are of similar conceptual weight, and `match` reflects
that.
- This lets the `incremental_verify_ich` call be factored out.
We can only reach this point if `try_load_from_disk_fn` fails, and the
condition of this assertion is basically just the inverse of what
`try_load_from_disk_fn` does. Basically it's like this:
```
fn foo() -> bool { a && b }
fn bar() {
if foo() {
return;
}
assert!(!a || !b);
}
```
The assertion is just confusing and provides little value.
These queries appear to have been using `anon` for its side-effect of making
them ineligible for forcing.
According to their comments and also `tests/incremental/issue-61323.rs`, these
queries want to avoid forcing so that if a cycle does occur, the whole cycle
will be on the query stack for the cycle handler to find.
Code that previously used `QueryStackFrame` now uses `TaggedQueryKey` directly.
Code that previously used `Spanned<QueryStackFrame>` now uses
`QueryStackFrame`, which includes a span.
variances_of currently used search_for_cycle_permutation, which can fail and abort when constructed error result value does not match query input.
This commit changes variances_of to receive a def_id which means it can compute a value without using search_for_cycle_permutation, avoiding the possible abort.
Fixes#127971
An explicit dep-kind is redundant here. If we want to find the query name, or
check for specific queries, we can inspect the `TaggedQueryKey` instead.
Similarly, in cases where the key is some kind of `DefId`, we can extract it
directly from the `TaggedQueryKey`.
Re-export `rustc_middle::query::{QuerySystem, QueryVTable}`
- Follow-up to https://github.com/rust-lang/rust/pull/153760#discussion_r2928848418.
---
All of the other public items in `rustc_middle::query::plumbing` are re-exported from `query`, except for these two, for no particular reason that I can see.
Re-exporting them allows `rustc_middle::query::plumbing` to have its visibility reduced to pub(crate).
Imports within `rustc_middle` have also been updated to consistently use the re-exports in `crate::query`.
r? nnethercote
Rename all-query functions.
There are four functions that use `for_each_query_vtable!` to call an "inner" function. They are:
- `collect_active_jobs_from_all_queries` -> `gather_active_jobs`
- `alloc_self_profile_query_strings` -> `alloc_self_profile_query_strings_for_query_cache`
- `encode_all_query_results` -> `encode_query_results`
- `query_key_hash_verify_all` -> `query_key_hash_verify`
These names are all over the place. This commit renames them as follows:
- `collect_active_query_jobs{,_inner}`
- `alloc_self_profile_query_strings{,_inner}`
- `encode_query_values{,_inner}`
- `verify_query_key_hashes{,_inner}`
This:
- puts the verb at the start
- uses `_inner` for all the inners (which makes sense now that the inners are all next to their callers)
- uses `_query_` consistently
- avoids `all`, because the plurals are enough
- uses `values` instead of `results`
- removes the `collect`/`gather` distinction, which is no longer important
r? @Zalathar
All of the other public items in `rustc_middle::query::plumbing` are
re-exported from `query`, except for these two, for no particular reason that I
can see.
Re-exporting them allows `rustc_middle::query::plumbing` to have its visibility
reduced to pub(crate).
Imports within `rustc_middle` have also been updated to consistently use the
re-exports in `crate::query`.
There are four functions that use `for_each_query_vtable!` to call an "inner"
function. They are:
- collect_active_jobs_from_all_queries -> gather_active_jobs
- alloc_self_profile_query_strings -> alloc_self_profile_query_strings_for_query_cache
- encode_all_query_results -> encode_query_results
- query_key_hash_verify_all -> query_key_hash_verify
These names are all over the place. This commit renames them as follows:
- collect_active_query_jobs{,_inner}
- alloc_self_profile_query_strings{,_inner}
- encode_query_values{,_inner}
- verify_query_key_hashes{,_inner}
This:
- puts the verb at the start
- uses "inner" for all the inners (which makes sense now that the inners are
all next to their callers)
- uses `_query_` consistently
- avoids `all`, because the plurals are enough
- uses `values` instead of `results`
- removes the `collect`/`gather` distinction, which is no longer
important
`CycleError` has one field containing a `(Span, QueryStackFrame<I>)` and
another field containing a `QueryInfo`, which is a struct containing
just a `Span` and a `QueryStackFrame<I>`.
We already have the `Spanned` type for adding a span to something. This
commit uses it for both fields in `CycleError`, removing the need for
`QueryInfo`. Which is good for the following reasons.
- Any type with `Info` in the name is suspect, IMO.
- `QueryInfo` can no longer be confused with the similar `QueryJobInfo`.
- The doc comment on `QueryInfo` was wrong; it didn't contain a query
key.
Use less `#[macro_use]` in the query system
Macro-rules namespacing and import/export is a bit of a nightmare in general. We can tame it a bit by avoiding `#[macro_use]` as much as possible, and instead putting a `pub(crate) use` after the macro-rules declaration to make the macro importable as a normal item.
I've split this PR into two commits. The first one should hopefully be uncontroversial, while the second commit takes the extra step of declaring `rustc_with_all_queries!` as a macros-2.0 macro, which gives much nicer import/export behaviour, at the expense of having to specify `#[rustc_macro_transparency = "semiopaque"]` to still have access to macro-rules hygiene, because the default macros-2.0 hygiene is too strict here.
There should be no change to compiler behaviour.
---
I stumbled into this while investigating some other changes, such as re-exporting `rustc_middle::query::QueryVTable` or moving the big callback macro out of `rustc_middle::query::plumbing`. I think it makes sense to make this change first, to make other module-juggling tasks easier.
r? nnethercote (or compiler)
teor