When pointing at each step of cycle errors, do not include the code snippet when the note points at the same place as the previous one (by setting the note's span to DUMMY_SP).
`collect_active_query_jobs` with `CollectActiveJobsKind::PartialAllowed`
is only used to print the query stack when the compiler panics. It
intentionally skips any query state shard whose lock it cannot take
without waiting, since a complete job map is not needed for that.
Under the parallel front-end another thread can still hold a shard lock
while the panic is being reported, so the skip happens nondeterministically
and the `warn!` was printed into the panic output. Because warnings are
shown by default, this leaked a "Failed to collect active jobs" line into
the diagnostics of panicking compilations and made their output unstable.
Lower the message to `debug!` so it stays available with `RUSTC_LOG` but
no longer pollutes the default output.
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.
Jacob Pratt