New must_not_suspend lint triggers even when value has been moved

[jplatte]

I tried this code (playground):

use std::sync::Mutex;

async fn foo() {
    let foo = Mutex::new(1);
    let lock = foo.lock().unwrap();

    // Prevent mutex lock being held across `.await` point.
    drop(lock);

    bar().await;
}

async fn bar() {}

I expected to see this happen: No warnings (except unused fns)

Instead, this happened: must_not_suspend triggered

Meta

rustc --version --verbose:

rustc 1.57.0-nightly (003d8d3f5 2021-10-04)
binary: rustc
commit-hash: 003d8d3f56848b6f3833340e859b089a09aea36a
commit-date: 2021-10-04
host: x86_64-unknown-linux-gnu
release: 1.57.0-nightly
LLVM version: 13.0.0

[jplatte]

When I encountered this in the real world, the drop and subsequent .await was in a branch so using a scope instead would have involved extra control flow. I silenced the lint for now.

[memoryruins]

The lint seems to be correct currently, since using drop is not enough to make the future Send.

fn is_send<T: Send>(_x: T) {}

// error: future cannot be sent between threads safely
fn main() { is_send(foo()); }

Wrapping in a block will avoid this issue for now like in #57478 (comment) until there are more precise generator captures #69663

[jplatte]

In my case whether the future is Send or not didn't seem to matter (for whatever reason), avoiding possible deadlocks seemed good enough. Maybe using an async-aware mutex would be the best solution though 🤔

[jplatte]

Wait it says that I closed this issue. If I did, that wasn't intentional so I'll reopen...

[memoryruins]

To clarify, I expect once the generator captures issue improves, the lint will no longer appear in the above example (and the future will become Send) since rustc will no longer think it's holding the guard across the await point in general.

Good to leave this open though to let someone who better understands the situation confirm if this is expected or not.

@rustbot label: +A-lint +A-async-await

[eholk]

That's correct, this looks like a symptom of #69663, so it should get better as generator captures improve. There may be some things we can do in the shorter term too.

[eholk]

We discussed this in the triage meeting today. It is indeed a result of generator captures being too conservative.

In the short term, @guswynn is working on making the lint allow by default.

Longer term, we have a couple of potential fixes. One is to fix the generator captures, which I'm working on.

Another option is to move the must_not_suspend lint checking to the MIR instead. This point in the code has all the information about what goes in the generator, so it's a good place to do the checking. This location will always have the most precise information available, so that's a point in its favor. However, it seems like it might make marking fns as must_not_suspend, rather than just types, a little trickier to do.

Apparently @guswynn and @nikomatsakis have also been discussing a third option that relies more on the trait system.

@rustbot label +AsyncAwait-Triaged