normalizing in writeback causes cycle errors with generators

[compiler-errors]

We normalize types during writeback resolve:
https://github.com/rust-lang/rust/blob/b6a8c762eed0ae0383658c38d65cb91bbd9800a1/compiler/rustc_hir_typeck/src/writeback.rs#L790-L794

When normalizing types, this may cause us to try to prove an auto trait bound on a generator witness that is local to the body:

trait Mirror {
    type Assoc;
}

impl<T> Mirror for T where T: Send {
    type Assoc = T;
}

fn mirror<T: Mirror>(x: Option<&T>) -> Option<T::Assoc> { None }

fn main() {
    let mut async_block = None;
    //^^ begins as `<?0 as Mirror>::Assoc`
    let local = mirror(async_block.as_ref());
    async_block = Some(async {});
    //^^ Now `<Generator as Mirror>::Assoc`.
    // When we try to resolve during writeback, we need to prove `Generator: Send`,
    // which makes the trait solver attempt to compute `GeneratorWitness: Send`. Cycle!
}

---

From #82 (comment) this should probably be fixed by extending TypingMode::Analysis to also track the generator witnesses of the current function.

[compiler-errors]

I see a few solutions for this problem:

1. Typeck's infcx stores a list of generator witness def-ids it must stall. This should be trivial to compute from the HIR. This is canonicalized as part of the goal (like opaque type storage) and we force any nested goals to be ambiguous if we encounter one of these witnesses.

2. We don't normalize during writeback. This seems problematic when it comes to modifying the rest of the compiler.

3. We use the defining anchor + reveal to stall any generator witnesses that we can determine are local to the body we're typecking. This seems problematic because check_coroutine_obligations also runs in a user-facing reveal mode, so I don't know how to distinguish typeck from post-typeck here...

[lcnr]

my initial vibe is to normalize in writeback before instantiating the witness infer var and accept ambiguous obligations in deeply_normalize as long as the normalized-to type is not ambiguous, returning these nested obligations. This is a bit annoying to implement as we need to restructure the code a bit, but I expect that to be the easiest and "cleanest" solution.

[compiler-errors]

my initial vibe is to normalize in writeback before instantiating the witness infer var and accept ambiguous obligations in deeply_normalize as long as the normalized-to type is not ambiguous, returning these nested obligations.

This concerns me a bit, since we're now breaking two invariants in both deeply normalize and writeback:

1. writeback shouldn't encounter infer vars, which are ambiguity errors.
2. deeply normalize should either normalize something, or it shouldn't, and if it does, then it shouldn't be ambiguous.

The fact that we stall obligations on an infer var and draining them feels ad-hoc, and it suggests to me that the solver should be taught about this generator witness stalling first-class, the same way it knows about opaques in a first-class way.

[lcnr]

writeback shouldn't encounter infer vars, which are ambiguity errors.

It still shouldn't, it would be: deeply-normalize -> instantiate-witness-infer -> writeback

deeply normalize should either normalize something, or it shouldn't, and if it does, then it shouldn't be ambiguous.

It is not ideal 🤔 for me the core invariant is that we don't leave some projection which can be later normalized. So erroring of the term infer var is unconstrained but accepting the projection goal itself to be ambig seems fine to me.

I guess this could cause issues if a wf project goal actually manages to result in constraints but later still does not hold. I think it's kind of an invariant that that doesn't happen once we cover impls using param env candidates. I think we currently have the defacto invariant: if a Projection goal constrains the term, it either holds or the trait is not implemented, resulting in an error elsewhere.

The fact that we stall obligations on an infer var and draining them feels ad-hoc[..]

I agree that it is adhoc, however, given that hir typeck and mir build are separate queries, I personally think it's fairly clean. in a sense writeback is really just a deep structurally_resolve needed by mir_build. That we require all obligations to be proven at that time is more or less incidental.

[..], and it suggests to me that the solver should be taught about this generator witness stalling first-class, the same way it knows about opaques in a first-class way.

I disagree quite strongly here. The witness being an infer var is the first-class way to tell the solver that it should stall.

[compiler-errors]

it would be: deeply-normalize -> instantiate-witness-infer -> writeback

That seems really annoying to handle:

1. deeply_normalize isn't equipped to return stalled ambiguous predicates (and every other call site doesn't care about them)
2. typeck results aren't TypeFoldable, so we need to ad-hoc pass over every field like writeback does
3. drain_unstalled_obligations is just wrong

[compiler-errors]

Thoughts:

• Add the generator def ids that we should keep ambiguous to the Analysis typing mode. If we try to check an auto trait for it, then we force ambiguity.
  • In typeck we set these def ids, but in borrowck (which also uses analysis_in_body) we don't need to.
• At the end of typeck, when we drain_unstalled_obligations, we could implement a proof tree visitor to just drain the obligations that end up touching one of these generator def ids. We don't need to do that now, tho, but it allows us to emit "true ambiguities" properly rather than just draining all obligations lol

Making the typing mode larger is a bit concerning. Maybe we should put the opaques and the coroutines into the same list together?

[lcnr]

affected ui tests:

• tests/ui/async-await/issues/issue-64477-2.rs
• tests/ui/async-await/issue-64130-4-async-move.rs
• tests/ui/async-await/issues/issue-64477.rs
• tests/ui/borrowck/async-reference-generality.rs
• tests/ui/fmt/nested-awaits-issue-122674.rs
• tests/ui/type-alias-impl-trait/issue-93411.rs

[lcnr]

I just realized. The only way we should encounter generators which are not from the current function is by leaking auto traits of an opaque. We could probably get away with simply always stalling on generators during analysis 🤔

[compiler-errors]

We could probably get away with simply always stalling on generators during analysis

I think this would cause us to stall way more obligations than we want to.

[lcnr]

We could probably get away with simply always stalling on generators during analysis

I think this would cause us to stall way more obligations than we want to.

my point is (if we have a typing mode only used during hir typeck), this would only additionally stall on generators leaked as part of opaque type auto trait leakage. I don't believe that stalling on this should be in any way problematic. I believe that I am generally less worried about stalling on more obligations than u are though