Misleading help suggests `Sync` bound when shareable reference is passed across or into await

[mzabaluev]

#59245 results in an error that may be difficult to interpret when structuring generic async code:

use std::fmt::Display;

async fn run(mut state: impl Display) {
    do_stuff(&state).await;
    // ...
}

async fn do_stuff(state: &impl Display) {
    println!("{state}");
}

fn spawn_task<T>(state: T)
where
    T: Display + Send + 'static,
{
    tokio::spawn(run(state));
}

The compiler (as of 1.82.0-nightly (80eb5a8 2024-08-13)) produces this error output:

error[E0277]: `T` cannot be shared between threads safely
   --> src/main.rs:16:18
    |
16  |     tokio::spawn(run(state));
    |     ------------ ^^^^^^^^^^ `T` cannot be shared between threads safely
    |     |
    |     required by a bound introduced by this call
    |
    = note: required for `&T` to implement `Send`
note: required because it's used within this `async` fn body
   --> src/main.rs:8:41
    |
8   |   async fn do_stuff(state: &impl Display) {
    |  _________________________________________^
9   | |     println!("{state}");
10  | | }
    | |_^
note: required because it's used within this `async` fn body
   --> src/main.rs:3:35
    |
3   |   async fn run(state: impl Display) {
    |  ___________________________________^
4   | |     do_stuff(&state).await;
5   | |     // ...
6   | | }
    | |_^
note: required by a bound in `tokio::spawn`
   --> /home/mzabaluev/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.39.2/src/task/spawn.rs:167:21
    |
165 |     pub fn spawn<F>(future: F) -> JoinHandle<F::Output>
    |            ----- required by a bound in this function
166 |     where
167 |         F: Future + Send + 'static,
    |                     ^^^^ required by this bound in `spawn`
help: consider further restricting this bound
    |
14  |     T: Display + Send + 'static + std::marker::Sync,
    |                                 +++++++++++++++++++

A non-restrictive, but also unintuitive, solution is to make the reference passed to do_stuff mutable (i.e. provably exclusive), even though mutability is not required by the function body.

Desired outcome

The help heuristic should detect that the Sync bound arises due to a shareable reference becoming a member of an async closure for which Send is required, and suggest using an exclusive reference as an alternative to restricting the bound.

Originally posted by @mzabaluev in #59245 (comment)

[traviscross]

@rustbot labels +AsyncAwait-Triaged -needs-triage

Interesting. We talked about this today in WG-async triage.

We minimized the issue as follows:

fn spawn<F: Future + Send + 'static>(_: F) {}

fn spawn_task<T: Send + 'static>(mut state: T) {
    spawn(async move {
        let x = &mut state; // <-- Remove `mut` for error.
        async move { _ = [x] }.await;
    });
}

fn main() {}

We also wrote this to demonstrate the interesting behavior of &mut here:

trait AllSend<'a, T: 'a + ?Sized>
where
    &'a mut T: Send, // <-- Remove `mut` to get error.
{
}

impl<'a, T: ?Sized + Send + 'a> AllSend<'a, T> for T {}

Overall, we felt in general that adding the Sync bound is going to be the right thing to do in most cases. We were skeptical about how generalizable the suggestion of threading mut throughout would be. We think the error message might just get too long if we were to try to suggest mut here, so we think things are probably fine as-is. Using mut to solve the problem here is a bit of a subtle technique.

[mzabaluev]

I understand that carrying a reference across await points requires Sync in general. But the suggestion to add the Sync bound without alternatives is misleading in cases when a simple code refactoring can make in unnecessary.

[traviscross]

Perhaps you could make a proposal here for exactly what the diagnostic should say and how it should look. When on WG-async we talked about this, we just couldn't really picture a good way to do this. A concrete proposal might help here.

[mzabaluev]

The current hint could be augmented like so:

help: consider further restricting this bound
    |
14  |     T: Display + Send + 'static + std::marker::Sync,
    |                                 +++++++++++++++++++
help: alternatively, `&mut T` is unique and can be safely passed between threads

[traviscross]

Reopening so as to enable consideration of the concrete proposal above.