regression: change in async capture rules?

[Mark-Simulacrum]

https://crater-reports.s3.amazonaws.com/beta-1.74-4/beta-2023-10-21/reg/esl01-dag-0.3.0/log.txt

[INFO] [stdout] error: future cannot be sent between threads safely
[INFO] [stdout]    --> src/nameset/lazy.rs:102:9
[INFO] [stdout]     |
[INFO] [stdout] 102 | /         Box::pin(futures::stream::unfold(self, |mut state| async move {
[INFO] [stdout] 103 | |             let result = state.next().await;
[INFO] [stdout] 104 | |             result.map(|r| (r, state))
[INFO] [stdout] 105 | |         }))
[INFO] [stdout]     | |___________^ future created by async block is not `Send`
[INFO] [stdout]     |
[INFO] [stdout]     = help: the trait `Sync` is not implemented for `(dyn VertexStream<Item = std::result::Result<VertexName, DagError>> + 'static)`
[INFO] [stdout] note: future is not `Send` as this value is used across an await
[INFO] [stdout]    --> src/nameset/lazy.rs:90:72
[INFO] [stdout]     |
[INFO] [stdout] 78  |             match inner.state {
[INFO] [stdout]     |                   ----------- has type `&lazy::Inner` which is not `Send`
[INFO] [stdout] ...
[INFO] [stdout] 90  |                             if let Err(err) = inner.load_more(1, None).await {
[INFO] [stdout]     |                                                                        ^^^^^ await occurs here, with `inner.state` maybe used later
[INFO] [stdout]     = note: required for the cast from `Pin<Box<Unfold<Iter, {[email protected]:102:48}, {async block@src/nameset/lazy.rs:102:60: 105:10}>>>` to `Pin<Box<dyn VertexStream<Item = std::result::Result<VertexName, DagError>>>>`
[INFO] [stdout]     = note: the full name for the source type has been written to '/opt/rustwide/target/debug/deps/dag-bd0c9923e92e6bff.long-type-3400019024982072103.txt'
[INFO] [stdout] 

[Noratrieb]

that's probably coming from -Zdrop-tracking-mir
@cjgillot

[apiraino]

for reference: 13e6f24

Assigning priority (Zulip discussion), possibly to be adjusted after discussion.

@rustbot label -I-prioritize +P-high

[Noratrieb]

this is basically a duplicate of #116242

[lqd]

Indeed bisects to #107421 cc @cjgillot.

However, it's not obviously a duplicate of #116242 to me (nothing screams dropck here), and it is indeed not fixed by @lcnr's #117134.

[wesleywiser]

Nominating for T-types discussion. This is a regression similar to #116242 (also caused by #107421) but is not resolved by #117134.

Do we see a possible path to fixing this or should we just accept the breakage?

[lqd]

Heavily reduced, and can still be minimized (as e.g. depends on futures):

use std::sync::Arc;

use futures::lock::Mutex;
use futures::Stream;

pub trait VertexStream: Stream<Item = ()> + Send {}
impl<T> VertexStream for T where T: Stream<Item = ()> + Send {}

struct Inner {
    stream: Box<dyn VertexStream>,
    state: State,
}

enum State {
    Incomplete,
    Complete,
}
impl Inner {
    async fn load_more(&mut self) {}
}

pub(crate) struct Iter {
    inner: Arc<Mutex<Inner>>,
}
impl Iter {
    async fn next(&mut self) {
        let mut inner = self.inner.lock().await;

        match inner.state {
            State::Complete if true => {}
            State::Incomplete => {
                let _ = inner.load_more().await;
            }
            _ => {}
        }
    }
    fn into_stream(self) -> Box<dyn VertexStream> {
        Box::new(futures::stream::unfold(self, |mut state| async move {
            let _ = state.next().await;
            Some(()).map(|r| (r, state))
        }))
    }
}

[lcnr]

further minimized, does not depend on external crates now: https://rust.godbolt.org/z/s5MbWqvWs

struct SendNotSync(*const ());
unsafe impl Send for SendNotSync {}
// impl !Sync for Foo {}

struct Inner {
    stream: SendNotSync,
    state: bool,
}

struct SendSync;
impl std::ops::Deref for SendSync {
    type Target = Inner;
    fn deref(&self) -> &Self::Target {
        todo!();
    }
}

async fn next() {
    let inner = SendSync;
    match inner.state {
        true if true => {}
        false => std::future::ready(()).await,
        _ => {}
    }
}

fn is_send<T: Send>(_: T) {}
fn main() {
    is_send(async move {
        let _ = next().await;
    })
}

[lcnr]

struct SendNotSync(*const ());
unsafe impl Send for SendNotSync {}
// impl !Sync for SendNotSync {} // automatically disabled

struct Inner {
    stream: SendNotSync,
    state: bool,
}

struct SendSync;
impl std::ops::Deref for SendSync {
    type Target = Inner;
    fn deref(&self) -> &Self::Target {
        todo!();
    }
}

async fn next() {
    let inner = SendSync;
    match inner.state {
        true if true => {}
        false => async {}.await,
        _ => {}
    }
}

fn is_send<T: Send>(_: T) {}
fn main() {
    is_send(next())
}

This test fails regardless of whether needs_drop returns yes or no for generators. Even changing generators to never need drop does not fix this regression. This means it is not a change to mir_generator|coroutine_witnesses, but a divergence between HIR and MIR witness computation.

Two questions remain:

• why do we need this weird match structure for this regression
• was the old HIR based computation unsound, can this regression be fixed

[lcnr]

We require a local to be stored at a yield point if it either is used afterwards directly or there exists a reference to that local. If the local has been dropped via a StorageDead we ignore it.

The local for &Inner implicitly created for the match statement is not used directly after the yield. Its StorageDead is after the yield however.

If there is a match guard, we emit a fake borrow of the scrutinee before the first deref, given (*inner).state we therefore fake borrow inner) to prevent us from overwriting it inside of the guard, the example used in the source is:

match x[10] {
    _ if { x = &[0]; false } => (),
    // would now be out of bounds
    y => (),
}

So, because of that fake borrow of &inner we consider it to be potentially borrowed and still required. However, the borrowed value is never actually used, only considered to be used by mir borrowck.

This means we should be able to ignore Shallow borrows when computing the generator layout, going to try that ✨

[lcnr]

reopening until beta-backport is completed

[Mark-Simulacrum]

Backported in #117764