Non-Send values that are assigned to a variable but not used across an await point incorrectly mark the entire Future as non-Send

[wallefan]

I tried this code: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=f9f172724348c228b481bcd813cc92e5

use std::sync::Mutex;

fn main() {
    assert_send(foo(&Mutex::new(SomeStruct)));
}

struct SomeStruct;

impl SomeStruct {
    fn some_method(&self) -> u32 {
        return 1;
    }
}

async fn foo(mutex: &Mutex<SomeStruct>) {
    let guard = mutex.lock().unwrap();
    guard.some_method();
    std::mem::drop(guard);
    bar().await;
}

async fn bar() {
}

fn assert_send<T: Send>(_t:T) {}

It fails to compile since the future is not Send.

Since the guard is explicitly dropped before the await point, I would expect this future to be Send. Indeed, if I remove the variable guard and condense those three lines onto a single line, like so:

async fn foo(mutex: &Mutex<SomeStruct>) {
    mutex.lock().unwrap().some_method();
    bar().await;
}

...it compiles and runs fine. I created a second Playground to confirm this that does some additional testing to make sure the future runs correctly.

I suspect, therefore, that this is a bug. I've confirmed it's present in the most recent stable, nightly, and beta releases of the compiler available from rustup.

Of note is that the bug occurs even if I remove the call to some_method(). Merely assigning the guard to a variable is sufficient to make the future non-Send.

Meta

rustup run stable rustc --version --verbose:

rustc 1.73.0 (cc66ad468 2023-10-03)
binary: rustc
commit-hash: cc66ad468955717ab92600c770da8c1601a4ff33
commit-date: 2023-10-03
host: x86_64-unknown-linux-gnu
release: 1.73.0
LLVM version: 17.0.2

rustup run nightly rustc --version --vernose:

rustc 1.75.0-nightly (475c71da0 2023-10-11)
binary: rustc
commit-hash: 475c71da0710fd1d40c046f9cee04b733b5b2b51
commit-date: 2023-10-11
host: x86_64-unknown-linux-gnu
release: 1.75.0-nightly
LLVM version: 17.0.2

rustup run beta rustc --version --verbose:

rustc 1.74.0-beta.1 (b5c050feb 2023-10-03)
binary: rustc
commit-hash: b5c050febf10c9bcc0459d41fe2a1e190ad30b8d
commit-date: 2023-10-03
host: x86_64-unknown-linux-gnu
release: 1.74.0-beta.1
LLVM version: 17.0.2

[traviscross]

@rustbot labels +AsyncAwait-Triaged +WG-async -needs-triage

We reviewed this today in WG-async triage.

The rules around what's considered to be held across an await point are intentionally a bit coarse. But this is an interesting case as it conceivably could be supported. We believe this is a feature request.

[JakkuSakura]

This is weird because we have some tests already.
f712df8

The minimal example I can get is

async fn local_dropped_before_await() {
    let mutex = Mutex::new(1);
    let mut guard = mutex.lock().unwrap();
    *guard = 2;
    std::mem::drop(guard);
    fut().await;
}

Saved local contains std::sync::MutexGuard<'{erased}, i32>, which is not expected
If we comment out *guard = 2, it's Send again.
At first I thought it's about braces, now probably related to liveness check.

a local has to be stored if it is either directly used after the
the suspend point, or if it is live and has been previously borrowed.

[JakkuSakura]

So the root cause is:

fn locals_live_across_suspend_points<'tcx>(
    tcx: TyCtxt<'tcx>,
    body: &Body<'tcx>,
    always_live_locals: &DenseBitSet<Local>,
    movable: bool,
) -> LivenessInfo {
    // Calculate when MIR locals have live storage. This gives us an upper bound of their
    // lifetimes.
    let mut storage_live = MaybeStorageLive::new(std::borrow::Cow::Borrowed(always_live_locals))
        .iterate_to_fixpoint(tcx, body, None)
        .into_results_cursor(body);

    // Calculate the MIR locals which have been previously
    // borrowed (even if they are still active).
    let borrowed_locals_results =
        MaybeBorrowedLocals.iterate_to_fixpoint(tcx, body, Some("coroutine"));

    // HERE: borrowed_locals_results contains all variables that has been borrowed somehow. 

    let mut borrowed_locals_cursor = borrowed_locals_results.clone().into_results_cursor(body);

    // Calculate the MIR locals that we need to keep storage around for.
    let mut requires_storage_results =
        MaybeRequiresStorage::new(borrowed_locals_results.into_results_cursor(body))
            .iterate_to_fixpoint(tcx, body, None);

    // HERE: requires_storage_results fails to eliminate our variable that is "fake-borrowed". 

    let mut requires_storage_cursor = requires_storage_results.as_results_cursor(body);

    // Calculate the liveness of MIR locals ignoring borrows.
    let mut liveness =
        MaybeLiveLocals.iterate_to_fixpoint(tcx, body, Some("coroutine")).into_results_cursor(body);
    ...
}

[JakkuSakura]

In MoveVisitor, it fails to kill drop(guard) because previously it has been borrowed in *guard = 2;

impl<'tcx> Visitor<'tcx> for MoveVisitor<'_, '_, 'tcx> {
    fn visit_local(&mut self, local: Local, context: PlaceContext, loc: Location) {
        if PlaceContext::NonMutatingUse(NonMutatingUseContext::Move) == context {
            self.borrowed_locals.seek_before_primary_effect(loc);
            if !self.borrowed_locals.get().contains(local) {
                self.state.kill(local);
            }
        }
    }
}

Currently, MaybeBorrowedLocals tracks all borrows other than *, & and &mut.

Rvalue::RawPtr(_, borrowed_place)
  | Rvalue::Ref(_, BorrowKind::Mut { .. } | BorrowKind::Shared, borrowed_place) => {
      if !borrowed_place.is_indirect() {
          self.trans.gen_(borrowed_place.local);
      }
  }

This is probably too aggressive because we only want those references across termination points. It also causes a conceptual deadlock with the above MoveVisitor implementation.