`async_fn_in_trait` and `return_type_notation` cause awkward awaits

[djkoloski]

This bug is not exclusively related to the unstable async_fn_in_trait and return_type_notation, but the ergonomics of these features is an important contributing factor.

I tried this code:

#![feature(
    async_fn_in_trait,
    return_type_notation,
)]

use tokio::task::{JoinHandle, spawn};

trait Foo: Send {
    async fn bar(&self) -> i32;
}

fn thing(factory: impl Foo<bar(): Send> + 'static) -> JoinHandle<i32> {
    spawn(async move { factory.bar().await })
}

And got this error message:

error: future cannot be sent between threads safely
   --> src/lib.rs:13:11
    |
13  |     spawn(async move { factory.bar().await })
    |           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ future created by async block is not `Send`
    |
note: future is not `Send` as this value is used across an await
   --> src/lib.rs:13:38
    |
13  |     spawn(async move { factory.bar().await })
    |                        -------       ^^^^^ - `factory` is later dropped here
    |                        |             |
    |                        |             await occurs here, with `factory` maybe used later
    |                        has type `&impl Foo<bar() : Send> + 'static` which is not `Send`
help: consider moving this into a `let` binding to create a shorter lived borrow
   --> src/lib.rs:13:24
    |
13  |     spawn(async move { factory.bar().await })
    |                        ^^^^^^^^^^^^^
note: required by a bound in `tokio::spawn`
   --> /usr/local/google/home/dkoloski/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.2/src/task/spawn.rs:163:21
    |
161 |     pub fn spawn<T>(future: T) -> JoinHandle<T::Output>
    |            ----- required by a bound in this function
162 |     where
163 |         T: Future + Send + 'static,
    |                     ^^^^ required by this bound in `spawn`
help: consider further restricting this bound
    |
12  | fn thing(factory: impl Foo<bar(): Send> + 'static + std::marker::Sync) -> JoinHandle<i32> {
    |                                                   +++++++++++++++++++

First things first, this diagnostic is 100% correct. Making either of these changes fixes the issue. However, this is a very annoying problem that will probably get worse as these features stabilize.

The root of the issue is that the impl Foo<..> + 'static parameter is opaque and so conservatively does not implement Sync. That's the fix suggested second in the diagnostic. We really don't need it to impl Sync so it doesn't make sense to do this.

The reason why it does need to implement Sync is because a reference to it is being held across an await point. However, I didn't make this reference - my argument was auto-ref'd to call bar() and that temporary is living until the end of the statement (and thus across an await point). I don't think this behavior makes sense, and so it may make sense to change it in the next edition.

You can reproduce this behavior without any nightly or unstable features with the following code:

use std::{cell::Cell, future::Future};
use tokio::task::{JoinHandle, spawn};

struct Foo(Cell<i32>);

impl Foo {
    fn bar(&self) -> impl Future<Output = i32> + '_ + Send {
        async { 10 }
    }
}

fn thing(factory: Foo) -> JoinHandle<i32> {
    spawn(async move { factory.bar().await })
}

But it's not quite as interesting.

Meta

rustc --version --verbose:

rustc 1.72.0-nightly (37998ab50 2023-06-11)
binary: rustc
commit-hash: 37998ab508d5d9fa0d465d7b535dc673087dda8f
commit-date: 2023-06-11
host: x86_64-unknown-linux-gnu
release: 1.72.0-nightly
LLVM version: 16.0.5

[compiler-errors]

We really don't need it to impl Sync so it doesn't make sense to do this.

Why not? The future returned by async fn bar captures &self, and may rely on that reference living until the end of the statement -- so we can't drop that temporary reference until after the await. Not exactly sure if I'm misunderstanding something, but I don't see how this can be changed 🤔

[djkoloski]

The future returned by async fn bar may capture &self, and if it does then the future will contain an &Foo. Because the signature requires that future to be Send, that will also require &Foo: Send which is the same as Foo: Sync. So it's not necessary for us to take impl Foo<bar(): Send> + Sync + 'static, we're already covered by the bar(): Send bound.

This situation matters for me because it's difficult to generate a Sync bound with trait_transformer when a user asks for a Send-able version of a trait. It's much easier to just add Send in all the correct places if the bounds propagate through.

[compiler-errors]

Maybe would be useful to have a diagnostic suggest to move the future into a separate variable so temps could be dropped.