Drop tracking ICE involving await in left hand side of assignment (-Zdrop-tracking)

[eholk]

This was found during the most recent crater run for #97334. The test case below is reduced from https://github.com/haydnv/tinychain.

pub struct File<B> {
    block: B,
}

pub async fn commit<B: Clone>(this: &mut File<B>) {
    async {}.await;
    async {}.await;
    async {}.await;
    async {}.await;

    let file = async { &this }.await;
    *async { &mut this.block }.await = file.block.clone();
}

This ICEs with the following message:

error: internal compiler error: compiler/rustc_mir_transform/src/generator.rs:760:13: Broken MIR: generator contains type B in MIR, but typeck only knows about {ResumeTy, &mut File<B>, impl Future<Output = ()>, (), impl Future<Output = ()>, impl Future<Output = ()>, impl Future<Output = ()>, impl Future<Output = &&mut File<B>>, &&mut File<B>, File<B>, impl Future<Output = &mut B>} and [&mut File<B>]
  --> src/lib.rs:5:51
   |
5  |   pub async fn commit<B: Clone>(this: &mut File<B>) {
   |  ___________________________________________________^
6  | |     async {}.await;
7  | |     async {}.await;
8  | |     async {}.await;
...  |
12 | |     *async { &mut this.block }.await = file.block.clone();
13 | | }
   | |_^

My guess is that the B that MIR and typeck disagree on is the one produced by file.block.clone().

Weirdly, if any of the async {}.await lines are removed, the program compiles successfully.

[Rageking8]

@rustbot label +T-compiler +C-bug

[eholk]

@rustbot label +AsyncAwait-Triaged

[vincenzopalazzo]

As discussed on Zulip I will try to make some report tomorrow regarding this issue!

@rustbot claim

[vincenzopalazzo]

After playing a little bit with this today, as posted also on Zulip I can have a thesis that can make sense, I take a look to the following crates report https://crater-reports.s3.amazonaws.com/pr-97334-1/try%23b755f8b9936d084e0363ce6b393c7e444a37080e/gh/88IO.blogpost-rs/log.txt

where the message crash is the following one

Broken MIR: generator contains type CurrentUser in MIR, but typeck only knows about {ResumeTy, &Handler, serenity::prelude::Context, serenity::model::channel::Message, impl std::future::Future<Output = CurrentUser>, (), bool, String, Arc<serenity::http::Http>, impl std::future::Future<Output = Result<serenity::model::channel::Message, SerenityError>>, Handler, Option<regex::Captures>, regex::Captures, u64, Token, impl std::future::Future<Output = Result<Response<egg_mode::tweet::Tweet>, egg_mode::error::Error>>, impl std::future::Future<Output = Result<Reaction, SerenityError>>, &DraftTweet, DraftTweet, impl std::future::Future<Output = Result<Response<egg_mode::tweet::Tweet>, egg_mode::error::Error>>, Response<egg_mode::tweet::Tweet>, impl std::future::Future<Output = Result<serenity::model::channel::Message, SerenityError>>} and [&Handler, serenity::prelude::Context, serenity::model::channel::Message]

Where the MIR known type does not know the type CurrentUser but knows the type impl std::future::Future<Output = CurrentUser>, I think this was a regression right? the MIR should know also CurrentUser or vice versa the future need to be unwrapped to CurrentUser!

[eholk]

Yeah, so for some more background, this bug is with a new generator capture analysis called Drop Tracking. Drop tracking is more precise than the current analysis, which means that it finds fewer types in the generator type than before (basically, the current analysis has a lot of false positives and drop tracking has less of them). This analysis happens during type checking, and then the code is lowered to MIR. In MIR a similar analysis is done to find the values that need to be captured as part of the generator state. The MIR analysis is also more conservative than in strictly needs to be (it also has false positives), so sometimes we have to make drop tracking less aggressive than it could be to match the MIR analysis. We've had a lot of bugs like that so far, so I would expect that's what's going on here. That said, it could also be that the drop tracking analysis is bad and is missing something that it actually should be finding.

[vincenzopalazzo]

Minimal Exaple to reproduce it

pub struct File<B> {
    block: B,
}

pub async fn commit<B: Clone>(this: &mut File<B>) {
    async {}.await;
    async {}.await;
    async {}.await;
    async {}.await;

    let file = async { &this }.await;
    *async { &mut this.block }.await = file.block.clone();
}

fn main() {
    let mut file = File {
        block: String::new(),
    };
    let _ = async move {
        commit(&mut file).await;
    };
}

[bryangarza]

@rustbot claim

[bryangarza]

Taking a look at this one this sprint

[vincenzopalazzo]

passing the task to @bryangarza noted that I had a couple of think on my plate already and I do not want have to much fun alone!

I will be available for help if needed!

[bryangarza]

fyi @vincenzopalazzo I have been looking at other tasks so I have not been able to get to this -- unassigned myself for now

[matthiaskrgr]

@eholk has this been fixed? Can you still reproduce this ice?

[compiler-errors]

This issue still reproduces with -Zdrop-tracking.

[eholk]

I found another instance of this:

use std::collections::HashMap;

fn main() {
    let _ = real_main();
}

async fn nop() {}

async fn real_main() {
    nop().await;
    nop().await;
    nop().await;
    nop().await;

    let mut map: HashMap<(), ()> = HashMap::new();
    map.insert((), nop().await);
}

Once again we have the four load-bearing nops. I'm guessing this issue is more generally about having four or more awaits followed by some mutable borrow being held across another await.

[eholk]

@rustbot claim

I'm going to take a look at fixing this.

[eholk]

Okay, I made some progress this afternoon so I wanted to update here.

The problem is that we refer to yields by where they come up in a post-order traversal of the HIR tree, but we have some disagreement in how they are counted. For a loop that gets desugared from a .await, cfg_build.rs counts one less expression than the similar calculation in region.rs. This is why the bug only shows up after having four useless awaits, since that's how far we need the counts to drift so we'll have different liveness information.

I've started a branch that will eventually have a fix, but for now I've just added some more debug logging and assertions to detect this drift better.

I looked for places in region.rs where we change expr_and_pat_count to find cases that we might not be matching in cfg_build.rs. I noticed we have some save and restore logic when handling bodies that we don't have cfg_build.rs. Tomorrow I plan to look into this some more to figure out what it's doing and how we should match that logic.