Currently `QueryVTable`'s `value_from_cycle_error` function pointer uses the `FromCycleError` trait to call query cycle handling code and to construct an erroneous query output value.
This trick however relies too heavily on trait impl specialization and makes those impls inconsistent (which is bad AFAIK).
Instead this commit directly modifies `value_from_cycle_error` function pointer upon vtable initialization and gets rid of `FromCycleError`.
`ActiveJobGuard::complete` and `ActiveJobGuard::drop` have very similar
code, though non-essential structural differences obscure this a little.
This commit factors out the common code into a new method
`ActiveJobGuard::complete_inner`. It also inlines and remove
`expect_job`, which ends up having a single call site.
Replace the `try_mark_green` hook with direct calls to `tcx.dep_graph`
All of the existing call sites are directly touching `tcx.dep_graph` anyway, so the extra layer of indirection provides no real benefit.
There should be no change to compiler behaviour.
r? nnethercote (or compiler)
Currently we use a mix of `C::Key` and `&C::Key` parameters. The former
is more common and a bit nicer, so convert some of the latter. This
results in less converting between the two types, and fewer sigils.
Overhaul `ensure_ok`
The interaction of `ensure_ok` and the `return_result_from_ensure_ok` query modifier is weird and hacky. This PR cleans it up. Details in the individual commits.
r? @Zalathar
`ensure_ok` provides a special, more efficient way of calling a query
when its return value isn't needed. But there is a complication: if the
query is marked with the `return_result_from_ensure_ok` modifier, then
it will return `Result<(), ErrorGuaranteed`. This is clunky and feels
tacked on. It's annoying to have to add a modifier to a query to declare
information present in its return type, and it's confusing that queries
called via `ensure_ok` have different return types depending on the
modifier.
This commit:
- Eliminates the `return_result_from_ensure_ok` modifier. The proc macro
now looks at the return type and detects if it matches `Result<_,
ErrorGuarantee>`. If so, it adds the modifier
`returns_error_guaranteed`. (Aside: We need better terminology to
distinguish modifiers written by the user in a `query` declaration
(e.g. `cycle_delayed_bug`) from modifiers added by the proc macro
(e.g. `cycle_error_handling`.))
- Introduces `ensure_result`, which replaces the use of `ensure_ok` for
queries that return `Result<_, ErrorGuarantee>`. As a result,
`ensure_ok` can now only be used for the "ignore the return value"
case.
Get rid of `QueryVTable::call_query_method_fn`
Calling the query method to promote a value is equivalent to doing a cache lookup and then calling `execute_query_fn`, so we can just do that manually instead.
There are two “functional” differences here: If a cache hit occurs, we don't record the hit for self-profiling, and we don't register a read of the dep node. In the context of promotion, which touches *all* eligible cache entries just before writing the memory-cached values to disk, those two steps should be unnecessary overhead anyway.
r? nnethercote (or compiler)
Remove `tls::with_related_context`.
This function gets the current `ImplicitCtxt` and checks that its `tcx` matches the passed-in `tcx`. It's an extra bit of sanity checking: when you already have a `tcx`, and you need access to the non-`tcx` parts of `ImplicitCtxt`, check that your `tcx` matches the one in `ImplicitCtxt`.
However, it's only used in two places: `start_query` and `current_query_job`. The non-checked alternatives (`with_context`, `with_context_opt`) are used in more places, including some where a `tcx` is available. And things would have to go catastrophically wrong for the check to fail -- e.g. if we somehow end up with multiple `TyCtxt`s. In my opinion it's just an extra case to understand in `tls.rs` that adds little value.
This commit removes it. This avoids the need for `tcx` parameters in a couple of places. The commit also adjusts how `start_query` sets up its `ImplicitCtxt` to more closely match how similar functions do it, i.e. with `..icx.clone()` for the unchanged fields.
r? @oli-obk
Remove `cycle_fatal` query modifier
This removes the `cycle_fatal` query modifier as it has no effect on its current users.
The default `CycleErrorHandling::Error` mode does the same as `cycle_fatal` when the default impl of `FromCycleError` is used. The return types of queries using `cycle_fatal` however have no specialized `FromCycleError` impl.
Abort after `representability` errors
Doing so results in better error messages and makes the code a bit simpler. Details in individual commits.
r? @oli-obk
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.
Note that `QueryVTable::hash_value_fn` is still an `Option`. Unlike the
above three functions, which have trivial behaviour in the `None` case,
`hash_value_fn` is used in ways where the `Some`/`None` distinction is
more meaningful.
This removes the `cycle_fatal` query modifier as it has no effect on its current users.
The default `CycleErrorHandling::Error` mode does the same as `cycle_fatal` when the default impl of `FromCycleError` is used. The return types of queries using `cycle_fatal` however have no specialized `FromCycleError` impl.
This function gets the current `ImplicitCtxt` and checks that its `tcx`
matches the passed-in `tcx`. It's an extra bit of sanity checking: when
you already have a `tcx`, and you need access to the non-`tcx` parts of
`ImplicitCtxt`, check that your `tcx` matches the one in `ImplicitCtxt`.
However, it's only used in two places: `start_query` and
`current_query_job`. The non-checked alternatives (`with_context`,
`with_context_opt`) are used in more places, including some where a
`tcx` is available. And things would have to go catastrophically wrong
for the check to fail -- e.g. if we somehow end up with multiple
`TyCtxt`s. In my opinion it's just an extra case to understand in
`tls.rs` that adds little value.
This commit removes it. This avoids the need for `tcx` parameters in a
couple of places. The commit also adjusts how `start_query` sets up its
`ImplicitCtxt` to more closely match how similar functions do it, i.e.
with `..icx.clone()` for the unchanged fields.
This variant was a fallback value used to continue analysis after a
`Representability` error, but it's no longer needed thanks to the
previous commit. This means `Representability` can now be reduced to a
unit type that exists just so `FromCycleError` can exist for the
`representability` query. (There is potential for additional cleanups
here, but those are beyond the scope of this PR.)
This means the `representability`/`representability_adt_ty` queries no
longer have a meaningful return value, i.e. they are check-only queries.
So they now have a `check_` prefix added. And the `rtry!` macro is no
longer needed.
Currently, `Representability::from_cycle_error` prints an "infinite
size" error and then returns `Representability::Infinite`, which lets
analysis continue. This commit changes it so it just aborts after
printing the error. This has two benefits.
First, the error messages are better. The error messages we get after
continuing are mostly bad -- we usually get another cycle error, e.g.
about drop checking or layout, which is not much use to the user, and
then abort after that. The only exception is `issue-105231.rs` where a
"conflicting implementations" error is now omitted, but there are three
other errors before that one so it's no great loss.
Second, it allows some simplifications: see the next commit.
Rejig `rustc_with_all_queries!`
There are three things relating to `rustc_with_all_queries!` that have been bugging me.
- `rustc_with_all_queries!`'s ability to receive `extra_fake_queries` lines like `[] fn Null(()) -> (),` where the only real thing is the `Null`, and everything is just pretending to be a normal query, ugh.
- `make_dep_kind_array!`: a macro produced by one macro (`define_dep_nodes!`) and used by another macro (`define_queries!`) in another crate, ugh.
- The `_dep_kind_vtable_ctors` module, which is a special module with no actual code that serves just a way of collecting vtable constructors from two different places so they can be referred to by `make_dep_kind_array!`, ugh.
By making some adjustments to how `rustc_with_all_queries!` works, all three of these things are eliminated.
r? @Zalathar
Currently `define_dep_nodes` produces a macro `make_dep_kind_array` that
encodes the names of non-queries followed by queries. This macro is used
by `make_dep_kind_vtables` to make the full array of vtables, by
referring to vtable constructor functions that are put into `mod
_dep_kind_vtable_ctors`. Pretty weird!
This commit takes advantage of the previous commit's changes to
`rustc_with_all_queries`, which makes both query and non-query
information available. A new call to `rustc_with_all_queries` is used to
construct the vtable array. (This moves some dep_kind_vtable code from
`plumbing.rs` to `dep_kind_vtables.rs`, which is good.) It's
straightforward now with iterator chaining, and `mod
_dep_kind_vtable_ctors` is no longer needed.
`rustc_with_all_queries` currently provides information about all
queries. Also, a caller can provide an ad hoc list of extra non-queries.
This is used by `define_queries` for non-query dep kinds: `Null`, `Red`,
etc. This is pretty hacky.
This commit changes `rustc_with_all_queries` so that the non-queries
information is available to all callers. (Some callers ignore the
non-query information.) This is done by adding `non_query` entries to
the primary list of queries in `rustc_queries!`.
It has no effect.
`symbol_name` is the only query that produces a `SymbolName`. If it was
marked with `cycle_delayed_bug`/`cycle_stash` then this `FromCycleError`
impl would make sense, but that's not the case. Maybe it was the case in
the past.
`Value` is an unhelpfully generic name. Standard naming procedure for a
trait with a single method is for the trait name to match the method
name, which is what this commit does. Likewise, the enclosing module is
renamed from `values` to `from_cycle_error`.
Also add a comment about some non-obvious behaviour.
Improve the forcing/promotion functions in `DepKindVTable`
This is a bundle of changes to the two function pointers in `DepKindVTable` that are responsible for forcing dep nodes, or promoting disk-cached values from the previous session into memory.
The perf improvements to incr-unchanged and incr-patched are likely from skipping more of the “promotion” plumbing for queries that never cache to disk.
- `force_from_dep_node` → `force_from_dep_node_fn`
- `try_load_from_disk_cache` → `promote_from_disk_fn`
This commit also inlines the wrapper function around `promote_from_disk_fn`.
Clean up `QueryVTable::hash_result` into `hash_value_fn`
This PR:
- Renames the query vtable field `hash_result` to `hash_value_fn`
- Removes the unhelpful `HashResult` type alias, which was hiding an important layer of `Option`
- Replaces the cryptic `hash_result!` helper macro with a more straightforward `if_no_hash!` helper, in line with other modifier-checking macros
- Renames a few identifiers to refer to a query's return value as `value`
There should be no change to compiler behaviour.
r? nnethercote (or compiler)
Remove query function arrays
`define_queries!` produces four arrays of function pointers, which other functions iterate over. These aren't actually necessary.
r? @petrochenkov
This commit:
- Renames the query vtable field `hash_result` to `hash_value_fn`
- Removes the unhelpful `HashResult` type alias, which was hiding an
important layer of `Option`
- Replaces the cryptic `hash_result!` helper macro with a more straightforward
`if_no_hash!` helper, in line with other modifier-checking macros
- Renames a few identifiers to refer to a query's return value as `value`
Don't allow subdiagnostic to use variables from their parent
Tangentially related to https://github.com/rust-lang/rust/issues/151366
This is PR 1/2 for structured diagnostics, will do the unstructured ones next. After the second PR I will be able to remove some code that should compensate for this PR being positive.
Regardless of this PR having a positive diff, I feel that subdiagnostics being able to use variables from their parent is very confusing, so this is for the better,.
r? @jdonszelmann
bors