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`.
`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.
Add a `TaggedQueryKey` to identify a query instance
This adds back a `TaggedQueryKey` enum which represents a query kind and the associated key. This is used to replace `QueryStackDeferred` and `QueryStackFrameExtra` and the associated lifting operation for cycle errors
This approach has a couple of benefits:
- We only run description queries when printing the query stack trace in the panic handler
- The unsafe code for `QueryStackDeferred` is removed
- Cycle handles have access to query keys, which may be handy
Some further work could be replacing `QueryStackFrame` with `TaggedQueryKey` as the extra information can be derived from it.
`execute_job` has a single call site in `try_execute_query`. This commit
inlines and removes `execute_job`, but also puts the part that checks
feedable results in its own separate function, `check_feedable`, because
it's a nicely separate piece of logic.
The big win here is that all the code dealing with the `QueryState` is
now together in `try_execute_query`: get the lock, do the lookup, drop
the lock, create the job guard, and complete the job guard. Previously
these steps were split across two functions which I found hard to
follow.
This commit purely moves code around, there are no other changes.
Introduce `for_each_query_vtable!` to move more code out of query macros
After https://github.com/rust-lang/rust/pull/153114 moved a few for-each-query functions into the big `rustc_query_impl::plumbing` macro, I have found that those functions became much harder to navigate and modify, because they no longer have access to ordinary IDE features in rust-analyzer. Even *finding* the functions is considerably harder, because a plain go-to-definition no longer works smoothly.
This PR therefore tries to move as much of that code back out of the macro as possible, with the aid of a smaller `for_each_query_vtable!` helper macro. A typical use of that macro looks like this:
```rust
for_each_query_vtable!(ALL, tcx, |query| {
query_key_hash_verify(query, tcx);
});
```
The result is an outer function consisting almost entirely of plain Rust code, with all of the usual IDE affordances expected of normal Rust code. Because it uses plain Rust syntax, it can also be formatted automatically by rustfmt.
Adding another layer of macro-defined macros is not something I propose lightly, but in this case I think the improvement is well worth it:
- The outer functions can once again be defined as “normal” Rust functions, right next to their corresponding inner functions, making navigation and modification much easier.
- The closure expression is ordinary Rust code that simply gets repeated ~300 times in the expansion, once for each query, in order to account for the variety of key/value/cache types used by different queries. Even within the closure expression, IDE features still *mostly* work, which is an improvement over the status quo.
- For future maintainers looking at the call site, the macro's effect should hopefully be pretty obvious and intuitive, reducing the need to even look at the helper macro. And the helper macro itself is largely straightforward, with its biggest complication being that it necessarily uses the `$name` metavar from the outer macro.
There should be no change to compiler behaviour.
r? nnethercote (or compiler)
Simplify `type_of_opaque`.
There is a bunch of complexity supporting the "cannot check whether the hidden type of opaque type satisfies auto traits" error that shows up in `tests/ui/impl-trait/auto-trait-leak.rs`. This is an obscure error that shows up in a single test. If we are willing to downgrade that error message to a cycle error, we can do the following.
- Simplify the `type_of_opaque` return value.
- Remove the `cycle_stash` query modifier.
- Remove the `CyclePlaceholder` type.
- Remove the `SelectionError::OpaqueTypeAutoTraitLeakageUnknown` variant.
- Remove a `FromCycleError` impl.
- Remove `report_opaque_type_auto_trait_leakage`.
- Remove the `StashKey::Cycle` variant.
- Remove the `CycleErrorHandling::Stash` variant.
That's a lot! I think this is a worthwhile trade-off.
r? @oli-obk
There is a bunch of complexity supporting the "cannot check whether the
hidden type of opaque type satisfies auto traits" error that shows up in
`tests/ui/impl-trait/auto-trait-leak.rs`. This is an obscure error that
shows up in a single test. If we are willing to downgrade that error
message to a cycle error, we can do the following.
- Simplify the `type_of_opaque` return value.
- Remove the `cycle_stash` query modifier.
- Remove the `CyclePlaceholder` type.
- Remove the `SelectionError::OpaqueTypeAutoTraitLeakageUnknown`
variant.
- Remove a `FromCycleError` impl.
- Remove `report_opaque_type_auto_trait_leakage`.
- Remove the `StashKey::Cycle` variant.
- Remove the `CycleErrorHandling::Stash` variant.
That's a lot! I think this is a worthwhile trade-off.
Eliminate `QueryLatchInfo`.
The boolean `complete` flag indicates whether the `waiters` vec is still in use, which means the `waiters` vec must be empty when `complete` is true. This is achieved by using `drain` on the waiters just after `complete` is set.
We can do better by using the type system to make invalid combinations impossible. This commit removes `complete` and puts the waiters inside an `Option`. `Some` means the query job is still active, and `None` once it is complete. And `QueryLatchInfo` is eliminated.
(The `Arc<Mutex<Option<Vec<Arc<QueryWaiter<'tcx>>>>>>` type is a mouthful, but when it's all in one place I find it easier to understand than when it's split across two types. And we can use `Option` methods like `as_ref`, `as_mut`, and `take`, which is nice.)
r? @petrochenkov
The boolean `complete` flag indicates whether the `waiters` vec is still
in use, which means the `waiters` vec must be empty when `complete` is
true. This is achieved by using `drain` on the waiters just after
`complete` is set.
We can do better by using the type system to make invalid combinations
impossible. This commit removes `complete` and puts the waiters inside
an `Option`. `Some` means the query job is still active, and `None` once
it is complete. And `QueryLatchInfo` is eliminated.
(The `Arc<Mutex<Option<Vec<Arc<QueryWaiter<'tcx>>>>>>` type is a
mouthful, but when it's all in one place I find it easier to understand
than when it's split across two types. And we can use `Option` methods
like `as_ref`, `as_mut`, and `take`, which is nice.)
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.
Zalathar