[E0521] coroutine should own upvars and assigning internal references to moved captures does not leak the references

[dingxiangfei2009]

I tried this code:

#![feature(coroutines, stmt_expr_attributes)]

fn foo(x: &i32) {
    let mut a = &3;
    let mut b = #[coroutine]
    move || {
        yield ();
        let b = 5;
        a = &b;
        //~^ ERROR borrowed data escapes outside of coroutine
    };
}

I expected to see this happen:
This should compile. The following is the current MIR of the coroutine at the analysis phase.

fn foo::{closure#0}(_1: {coroutine@test.rs:6:5: 6:12}, _2: ()) -> ()
yields ()
 {
    debug a => (_1.0: &i32);
    let mut _0: ();
    let _3: ();
    let mut _4: ();
    let _5: i32;
    let mut _6: &i32;
    let _7: &i32;
    scope 1 {
        debug b => _5;
    }

    bb0: {
        StorageLive(_3);
        StorageLive(_4);
        _4 = ();
        _3 = yield(move _4) -> [resume: bb1, drop: bb3];
    }

    bb1: {
        StorageDead(_4);
        StorageDead(_3);
        StorageLive(_5);
        _5 = const 5_i32;
        FakeRead(ForLet(None), _5);
        StorageLive(_6);
        StorageLive(_7);
        _7 = &_5;
        _6 = &(*_7);
        (_1.0: &i32) = move _6;
        StorageDead(_6);
        StorageDead(_7);
        _0 = const ();
        StorageDead(_5);
        drop(_1) -> [return: bb2, unwind: bb5];
    }

    bb2: {
        return;
    }

    bb3: {
        StorageDead(_4);
        StorageDead(_3);
        drop(_1) -> [return: bb4, unwind: bb5];
    }

    bb4: {
        coroutine_drop;
    }

    bb5 (cleanup): {
        resume;
    }
}

Instead, this happened:
This unfortunately does not compile.

error[E0521]: borrowed data escapes outside of closure
 --> test.rs:7:9
  |
4 |     let mut a = &3;
  |         ----- `a` declared here, outside of the closure body
...
9 |         a = &b;
  |         ^^^^--
  |         |   |
  |         |   borrow is only valid in the closure body
  |         reference to `b` escapes the closure body here

This boils down to a borrow not living long enough due to the StorageDead(_5) statement in bb1, at which _1 is still alive. However, _1.0 is a capture moved into the closure and is no longer accessible outside of this coroutine.

Original issue description

I tried this code:

fn fnonce(_: impl FnOnce()) {}

fn foo(x: &i32) {
    let mut a = &3;
    fnonce(move || {
        let b = 5;
        a = &b;
        //~^ ERROR borrowed data escapes outside of coroutine
    });
}

I expected to see this happen:
This should compile. The following is the current MIR of the closure at the analysis phase.

// MIR for `foo::{closure#0}` after analysis

fn foo::{closure#0}(_1: {closure@fnonce.rs:5:12: 5:19}) -> () {
    debug a => (_1.0: &i32);
    let mut _0: ();
    let _2: i32;
    let mut _3: &i32;
    let _4: &i32;
    scope 1 {
        debug b => _2;
    }

    bb0: {
        StorageLive(_2);
        _2 = const 5_i32;
        FakeRead(ForLet(None), _2);
        StorageLive(_3);
        StorageLive(_4);
        _4 = &_2;
        _3 = &(*_4);
        (_1.0: &i32) = move _3;
        StorageDead(_3);
        StorageDead(_4);
        _0 = const ();
        StorageDead(_2);
        return;
    }
}

Note that the closure value _1 has a correct type for a FnOnce so that it is entirely owned by the closure body.

Instead, this happened:
This unfortunately does not compile.

error[E0521]: borrowed data escapes outside of closure
 --> fnonce.rs:7:9
  |
4 |     let mut a = &3;
  |         ----- `a` declared here, outside of the closure body
...
7 |         a = &b;
  |         ^^^^--
  |         |   |
  |         |   borrow is only valid in the closure body
  |         reference to `b` escapes the closure body here

This boils down to a borrow not living long enough due to the StorageDead(_2) statement, at which _1 is still alive.

A similar situation also applies to coroutines as implemented today. See this test suite

Meta

rustc --version --verbose:

rustc 1.88.0-dev
binary: rustc
commit-hash: unknown
commit-date: unknown
host: x86_64-unknown-linux-gnu
release: 1.88.0-dev
LLVM version: 20.1.2

The commit hash should be 49e5e4e3a5610c240a717cb99003a5d5d3356679.

Possible way forward

We could generate a separate body for FnOnce variant of closures, when requested, and apply a MIR transformation early to lift the upvars into their own locals. This is a known technique in #135527.

Backtrace

N/A

[compiler-errors]

. We could generate a separate body for FnOnce variant of closures, when requested, and apply a MIR transformation early to lift the upvars into their own locals.

I'm not sure if this part makes sense. This transformation is only sound if the closure implements only FnOnce -- if it implements FnMut or Fn, then the transformation is unsound. Therefore I'm not sure why we'd be generating a separate body, since the only body that we have is the FnOnce implementation which we'd just lower differently before analysis.

I'm not totally sure that this is worth fixing either, since it adds a somewhat nontrivial behavioral shift between whether a closure is FnOnce or FnMut/Fn.

I don't believe that this is a bug though, but instead an enhancement. Still not convinced that it's worth fixing, though.

[dingxiangfei2009]

I agree with your opinion on FnOnce. I would like to talk about the other half of this ticket, about coroutines.

There is surely only one body per coroutine that we generate and it takes ownership of its states throughout its lifecycle and a coroutine is not re-entrant like a Fn/FnMut by design.

What I worry about is that coroutines are not a real closure. They can capture like closures and the current syntax resembles a closure for some reason. Users may mistake them as false twins of FnOnce closures.

Leaving discussion on the syntax and the false-twining consequence aside, I would like to introduce this enhancement is because it would also allow us to enable space-saving optimisation on the coroutine data layout. I believe this is a fair enhancement to bring onto coroutines.

I hope that replacing the closure syntax with the gen block will resolve this FnOnce illusion.

[dingxiangfei2009]

Optional content for discussion

Besides, I remark that we are already generating a separate body for an AsyncFnOnce out of its existing AsyncFn* body.