wrap ty field of CoroutineSavedTy in EarlyBinder

[levyry]

Currently CoroutineSavedTy contains a regular Ty, but to use coroutine fields correctly, they need to be instantiated first. This PR changes CoroutineSavedTy to contain an EarlyBinder<Ty> instead, so instantiation cannot be skipped.

Relevant zulip thread: #t-types/call-for-participation > coroutine witness dropck instantiate fields

This is my first contribution to the project, I'd appreciate any and all feedback!

r? @lcnr

[rustbot]

Thanks for the pull request, and welcome! The Rust Project has assigned @lcnr (or someone else) to review your changes, you should hear from them (or someone else) within the next two weeks.

Please see the contribution instructions and our LLM policy for more information.

[lcnr]

oh, why is this TypeFoldable?

ignoring this field is wrong. We should not type fold CoroutineSavedTy. What breaks if you remove the derive there?

View changes since the review

[levyry]

if I just remove the derive I get an error about how CoroutineLayout can't implement TypeFoldable because its fields don't implement it.

full error

error[E0277]: the trait bound `mir::query::CoroutineSavedTy<'_>: rustc_type_ir::TypeFoldable<context::TyCtxt<'_>>` is not satisfied
  --> compiler/rustc_middle/src/mir/query.rs:39:5
   |
37 |   #[derive(TyEncodable, TyDecodable, StableHash, TypeFoldable, TypeVisitable)]
   |                                                  ------------
   |                                                  |
   |                                                  required by a bound introduced by this call
   |                                                  in this derive macro expansion
38 |   pub struct CoroutineLayout<'tcx> {
39 |       /// The type of every local stored inside the coroutine.
   |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ unsatisfied trait bound
   |
  ::: /home/levy/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/synstructure-0.13.2/src/macros.rs:95:9
   |
95 | /         pub fn $derives(
96 | |             i: $crate::macros::TokenStream
97 | |         ) -> $crate::macros::TokenStream {
   | |________________________________________- in this expansion of `#[derive(TypeFoldable)]`
   |
help: the trait `rustc_type_ir::TypeFoldable<context::TyCtxt<'_>>` is not implemented for `mir::query::CoroutineSavedTy<'_>`
  --> compiler/rustc_middle/src/mir/query.rs:24:1
   |
24 | pub struct CoroutineSavedTy<'tcx> {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   = help: the following other types implement trait `rustc_type_ir::TypeFoldable<I>`:
             `&'tcx list::RawList<(), (rustc_type_ir::OpaqueTypeKey<context::TyCtxt<'tcx>>, ty::Ty<'tcx>)>` implements `rustc_type_ir::TypeFoldable<context::TyCtxt<'tcx>>`
             `&'tcx list::RawList<(), generic_args::GenericArg<'tcx>>` implements `rustc_type_ir::TypeFoldable<context::TyCtxt<'tcx>>`
             `&'tcx list::RawList<(), rustc_span::def_id::LocalDefId>` implements `rustc_type_ir::TypeFoldable<context::TyCtxt<'tcx>>`
             `&RawList<(), Binder<TyCtxt<'tcx>, ExistentialPredicate<TyCtxt<'tcx>>>>` implements `rustc_type_ir::TypeFoldable<context::TyCtxt<'tcx>>`
             `&RawList<(), OutlivesClause<TyCtxt<'tcx>, GenericArg<'tcx>>>` implements `rustc_type_ir::TypeFoldable<context::TyCtxt<'tcx>>`
             `&'tcx list::RawList<(), syntax::ProjectionElem<mir::Local, ty::Ty<'tcx>>>` implements `rustc_type_ir::TypeFoldable<context::TyCtxt<'tcx>>`
             `&'tcx list::RawList<(), ty::Ty<'tcx>>` implements `rustc_type_ir::TypeFoldable<context::TyCtxt<'tcx>>`
             `&'tcx list::RawList<(), ty::consts::Const<'tcx>>` implements `rustc_type_ir::TypeFoldable<context::TyCtxt<'tcx>>`
           and 262 others
   = note: required for `rustc_index::IndexVec<mir::query::CoroutineSavedLocal, mir::query::CoroutineSavedTy<'_>>` to implement `rustc_type_ir::TypeFoldable<context::TyCtxt<'_>>`
   = note: the full name for the type has been written to '/home/levy/src/rust-lang/rust/build-rust-analyzer/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/build/rustc_middle/374f18fe06c7bef2/out/rustc_middle-374f18fe06c7bef2.long-type-8635111359931186326.txt'
   = note: consider using `--verbose` to print the full type name to the console

If I add a #[type_foldable(identity)] for the field_tys field which stores the CoroutineSavedTys then x check and x test tests/ui both succeed

[lcnr]

why does CoroutineLAyout implement TypeFoldable :>

[levyry]

the chain goes like this:

CoroutineSavedTy -> CoroutineLayout -> CoroutineInfo -> Body

and if I remove it from Body I get an error at rustc_public_bridge/src/builder.rs about how Body needs to be TypeFoldable because EarlyBinder::bind requires that as a trait bound

[lcnr]

hmm, that's kind of scuffed. Body shouldn't be TypeFoldable ideally as some of the fields of MIR bodies don't make sense after it. Fixing that is annoying and non-trivial and ignoring the EArlyBinder here is fine. Coolio

[lcnr]

instead of skip_normalization I think we should add a fn already_normalized function to Unnormalized and use that in some of the places here. In others we do just actually have to normalize and not doing so is a bug of the current impl.

Can you replace the skip_normalization calls added in this PR with skip_norm_wip so that we can clean them up later?

View changes since this review

[levyry]

Can you replace the skip_normalization calls added in this PR with skip_norm_wip so that we can clean them up later?

Done!

[rustbot]

clippy is developed in its own repository. If possible, consider making this change to rust-lang/rust-clippy instead.

cc @rust-lang/clippy

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[lcnr]

This is somewhat weird, we use CoroutineLayout both directly inside of the MIR body, in which case we shouldn't use EarlyBinder and have the problem that if we instantiate the body we're now no longer instantiating the type in CoroutineLayout

In a way we should store different types in MIR bodies than what we return from the fn coroutine_layout 🤔 If we just return EarlyBinder<&'tcx CoroutineLayout> then we need to deal with the EarlyBinder even when accessing the fields that don't contain a type.

We could make that methods on EarlyBinder<&'tcx CoroutineLayout>, similar to what we do for TraitClause to get the fn def_id 🤔 that feels better to me...

THat's even weirder, fn coroutine_layout already takes an args but just doesn't instantiate the layout, so it's very non-obvious that we need to instantiate the layout, or even more importantly, that it's correct to instantiate with exactly these args again 🤔

We could make that methods on EarlyBinder<&'tcx CoroutineLayout>, similar to what we do for TraitClause to get the fn def_id 🤔 that feels better to me...

We could go even further and have a struct QueriedCoroutineLayout { layout: &'tcx CoroutineLayout, args: Option<GenericArgs> } whose layout field is private and accessors optionally instantiate the saved ty if necessary. The name is slightly scuffed, but sth like that would probably work best. hmm, wanna try that? 😅

View changes since this review

[levyry]

We could go even further and have a struct QueriedCoroutineLayout { layout: &'tcx CoroutineLayout, args: Option<GenericArgs> } whose layout field is private and accessors optionally instantiate the saved ty if necessary.

hmmm looked into this a bit, going to try to implement it now, I'll let you know if I get stuck anywhere! I am wondering about the exact API that QueriedCoroutineLayout should provide, but I'll try figuring this out as I'm refactoring the call sites of fn coroutine_layout after changing the return type...

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[levyry]

This function is only used once here, and I am not sure if it's really needed or not, as ideally get_ty would handle the identity initialization as well, but for this first version of QueriedCoroutineLayout I wanted to include a more personalized API, which I can revise later ^^

View changes since the review

[levyry]

raw_layout is only used here in validate.rs. I could remove this fn rewrite this branch to use QueriedCoroutineLayout if you'd like, but it seemed messier to me

View changes since the review

[levyry]

To clarify the messiness: in this branch we have a &'a CoroutineLayout instead of a &'tcx CoroutineLayout, so it seems to me the only way to work with QueriedCoroutineLayout directly here is to do:

pub struct QueriedCoroutineLayout<'a, 'tcx> {
    layout: &'a CoroutineLayout<'tcx>,
    args: Option<GenericArgsRef<'tcx>>,
}

which seemed undesirable to me.

[levyry]

Some functions still return just a normal CoroutineLayout (like TyCtxt::coroutine_layout_raw, or the mir_coroutine_witnesses query in this function), so some manual instantiation is still present. I assume this is fine? Just wanted to make sure ^^

View changes since the review

[levyry]

I made this an Option like you wrote previously, I assume this is so that when this is None we know to use fn instantiate_identity, but during my refactorings I didn't really see any places that would warrant instantiating this field with None. Could you let me know if I misinterpreted your comment or if I didn't instantiate this field correctly somewhere?

View changes since the review