Skip to content

Enforce no (stable) auto traits are implemented for the returned Future #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ghost opened this issue Feb 17, 2021 · 6 comments
Open

Comments

@ghost
Copy link

ghost commented Feb 17, 2021

Currently #[clear] (and maybe also #[set]) do not enforce that as this compiles:

#[async_auto_traits::clear]
async fn foo() {}

fn assert_send(_: impl Send) {}

fn main() {
    assert_send(foo());
}

Because impl Future does not hide auto traits.

The enforcement could be done through something like this (play):

use pin_project_lite::pin_project;
use std::{
    future::Future,
    marker::PhantomData,
    pin::Pin,
    task::{Context, Poll},
};

pin_project! {
    pub struct NotSendButSyncFuture<F: Future> {
        #[pin]
        future: F,
        ghost: PhantomData<*const ()>,
    }
}

unsafe impl<F: Future + Sync> Sync for NotSendButSyncFuture<F> {}

impl<F: Future> NotSendButSyncFuture<F> {
    pub fn new(future: F) -> Self {
        Self {
            future,
            ghost: PhantomData,
        }
    }
}

impl<F: Future> Future for NotSendButSyncFuture<F> {
    type Output = F::Output;

    fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
        self.project().future.poll(cx)
    }
}

fn foo() -> impl Future {
    NotSendButSyncFuture::new(async move { todo!() })
}

fn assert_send(_: impl Send) {}

fn main() {
    assert_send(foo());
}

Or something like this (but may not work properly for async fns that have no .await, play):

use std::{future, marker::PhantomData};

async fn foo() {
    let _phantom = PhantomData::<*mut ()>;
    future::ready(()).await;
}

fn assert_send_sync(_: impl Send + Sync) {}

fn main() {
    assert_send_sync(foo());
}

(Moved from rust-lang/rust#82187 (comment), sorry for being off-topic there!)

@jplatte
Copy link
Owner

jplatte commented Feb 17, 2021

but may not work properly for async fns that have no .await

what do you mean by this?

@ghost
Copy link
Author

ghost commented Feb 17, 2021

I meant something like (more complicated than) this (play):

use std::marker::PhantomData;

async fn foo() {
    let _phantom = PhantomData::<*mut ()>;
}

fn assert_send_sync(_: impl Send + Sync) {}

fn main() {
    assert_send_sync(foo());
}

Then the attribute may have to add an .await to the function body, which may cause some variables to be holded across the .await point, and cause some lifetime or Sync/Send issues that are tricky to resolve.

@jplatte
Copy link
Owner

jplatte commented Feb 17, 2021

I think the problematic case is really when such a non-Send / Sync type was passed as a parameter. The given example would not be problematic because the await point would be before the declaration of _phantom. Still, should go with the more complicated solution probably. For now I'll commit a rewrite that just panics on opt-out and combines all attributes into one.

@jplatte
Copy link
Owner

jplatte commented Feb 17, 2021

Done. 82f8627

@hyd-dev wdyt about !Send vs ?Send for the opt-out syntax?

@ghost
Copy link
Author

ghost commented Feb 17, 2021

I personally prefer !Send because ?Send make me think of input type bounds, while this is for the output (return) type.

EDIT: Another reason: async-trait uses ?Send, but it add the Send bound by default, ?Send is used to elide that, while this attribute does not add any bound (except Future) by default, which is different, so I think it's worth using a different !Send syntax.

@jyn514
Copy link

jyn514 commented Feb 22, 2021

See also https://github.com/tokio-rs/tokio/blob/master/tokio/tests/async_send_sync.rs, which enforces !Sync using an AmbiguousIfSync trait.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants