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)
Unlike `macro_rules!`, macros-2.0 macros have sensible item-like namespacing
and visibility by default, which avoids the need for `#[macro_export]` and
makes it easier to import the macro.
The tradeoff is having to use `#[rustc_macro_transparency = "semiopaque"]` to
still get macro-rules hygiene, because macros-2.0 hygiene is too strict here.
Remove `def_id_for_ty_in_cycle`
This removes `QueryKey::def_id_for_ty_in_cycle` and `QueryStackFrame.def_id_for_ty_in_cycle` by computing it instead from `TaggedQueryKey`.
Remove `value_from_cycle_error` specializations
This removes `value_from_cycle_error` specializations which are not involved in custom cycle error handling.
This is a step towards removing query cycle recovery.
Move and expand the big `rustc_query_impl` macro into a physical `query_impl.rs`
While looking through https://github.com/rust-lang/rust/pull/153588, I came up with a related but different change that I think resolves a lot of tension in the current module arrangement.
The core idea is that if we both define and expand the big macro in the same physical module `rustc_query_impl::query_impl`, then we no longer need to worry about where `mod query_impl` should be declared, or where its imports should go, because those questions now have simple and obvious answers.
The second commit follows up with some more changes inspired by https://github.com/rust-lang/rust/pull/153588. Those particular follow-ups are not essential to the main idea of this PR.
r? nnethercote
Replace `visit_waiters` with `abstracted_waiters_of`
This replaces `visit_waiters` which uses closures to visit waiters with `abstracted_waiters_of` which returns a list of waiters. This makes the control flow of their users a bit clearer.
Additionally `abstracted_waiters_of` includes non-query waiters unlike `visit_waiters`. This means that `connected_to_root` now considers resumable waiters from the compiler root as being connected to root, while previously only non-resumable waiters counted. This can result in some additional entry points being found. Similarly in the loop detecting entry points we now consider queries in the cycle with direct resumable waiters from the compiler root as being entry points.
When looking for entry points we now look at waiters until we found a query to populate the `EntryPoint.waiter` field instead of stopping when we determined it to be an entry point.
cc @petrochenkov @nnethercote
Moving the macro and its expansion into the same physical file resolves a lot
of tension in the current module arrangement.
Code in the macro is now free to use plain imports in the same file, and there
is no longer any question of whether `mod query_impl` should be declared inside
the macro, or surrounding a separate expansion site.
Remove `CycleErrorHandling`.
This PR removes the `cycle_*` query modifiers and `CycleErrorHandling`. Some good simplicity wins. Details in individual commits.
r? @Zalathar
Currently if a cycle error occurs the error is created, then handled
according to `CycleErrorHandling` (which comes from the
`cycle_delay_bug` query modifer, or lack thereof), and then
`value_from_cycle_error` is called.
This commit changes things so the error is created and then just passed
to `value_from_cycle_error`. This gives `value_from_cycle_error` full
control over how it's handled, eliminating the need for
`CycleErrorHandling`. This makes things simpler overall.
`collect_active_jobs_from_all_queries` takes a `require_complete` bool,
and then some callers `expect` a full map result while others allow a
partial map result. The end result is four possible combinations, but
only three of them are used/make sense.
This commit introduces `CollectActiveJobsKind`, a three-value enum that
describes the three sensible combinations, and rewrites
`collect_active_jobs_from_all_queries` around it. This makes it and its
call sites much clearer, and removes the weird `Option<()>` and
`Result<QueryJobMap, QueryJobMap>` return types.
Other changes of note.
- `active` is removed. The comment about `make_frame` is out of date,
and `create_deferred_query_stack_frame` *is* safe to call with the
query state locked.
- When shard locking failure is allowed, collection no longer stops on
the first failed shard.
Zalathar