Skip to content
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

New projection macro #20

Closed
Pauan opened this issue Jan 8, 2019 · 8 comments
Closed

New projection macro #20

Pauan opened this issue Jan 8, 2019 · 8 comments

Comments

@Pauan
Copy link

Pauan commented Jan 8, 2019

I have used pin-utils extensively on a Pin-heavy library.

Overall I found it rather unergonomic, so I created a new macro:

#[macro_export]
macro_rules! unsafe_project {
    (@parse $value:expr,) => {};
    (@parse $value:expr, pin $name:ident, $($rest:tt)*) => {
        #[allow(unused_mut)]
        let mut $name = unsafe { ::std::pin::Pin::new_unchecked(&mut $value.$name) };
        $crate::unsafe_project! { @parse $value, $($rest)* }
    };
    (@parse $value:expr, mut $name:ident, $($rest:tt)*) => {
        #[allow(unused_mut)]
        let mut $name = &mut $value.$name;
        $crate::unsafe_project! { @parse $value, $($rest)* }
    };

    ($value:expr => { $($bindings:tt)+ }) => {
        let value = unsafe { ::std::pin::Pin::get_unchecked_mut($value) };
        $crate::unsafe_project! { @parse value, $($bindings)+ }
    };
}

Here is an example of using it:

unsafe_project!(self => {
    pin signal1,
    pin signal2,
    mut left,
    mut right,
    mut callback,
});

Basically, you pass in something which is pinned (in this case self), and then it will destructure the individual fields (which are marked as either pin or mut).

Using pin is similar to unsafe_pinned and using mut is similar to unsafe_unpinned.


Here is an example of real code using the pin-utils macros (unsafe_pinned and unsafe_unpinned):

#[derive(Debug)]
#[must_use = "Futures do nothing unless polled"]
pub struct CancelableFuture<A, B> {
    state: Arc<CancelableFutureState>,
    future: Option<A>,
    when_cancelled: Option<B>,
}

impl<A, B> CancelableFuture<A, B> {
    unsafe_unpinned!(state: Arc<CancelableFutureState>);
    unsafe_pinned!(future: Option<A>);
    unsafe_unpinned!(when_cancelled: Option<B>);
}

impl<A, B> Unpin for CancelableFuture<A, B> where A: Unpin {}

impl<A, B> Future for CancelableFuture<A, B>
    where A: Future,
          B: FnOnce() -> A::Output {

    type Output = A::Output;

    fn poll(mut self: Pin<&mut Self>, waker: &LocalWaker) -> Poll<Self::Output> {
        if self.as_mut().state().is_cancelled.load(Ordering::SeqCst) {
            Pin::set(self.as_mut().future(), None);
            let callback = self.as_mut().when_cancelled().take().unwrap();
            Poll::Ready(callback())

        } else {
            match self.as_mut().future().as_pin_mut().unwrap().poll(waker) {
                Poll::Pending => {
                    *self.as_mut().state().waker.lock().unwrap() = Some(waker.clone().into_waker());
                    Poll::Pending
                },
                a => a,
            }
        }
    }
}

And the same code with my unsafe_project macro:

#[derive(Debug)]
#[must_use = "Futures do nothing unless polled"]
pub struct CancelableFuture<A, B> {
    state: Arc<CancelableFutureState>,
    future: Option<A>,
    when_cancelled: Option<B>,
}

impl<A, B> Unpin for CancelableFuture<A, B> where A: Unpin {}

impl<A, B> Future for CancelableFuture<A, B>
    where A: Future,
          B: FnOnce() -> A::Output {

    type Output = A::Output;

    fn poll(self: Pin<&mut Self>, waker: &LocalWaker) -> Poll<Self::Output> {
        unsafe_project!(self => {
            mut state,
            pin future,
            mut when_cancelled,
        });

        if state.is_cancelled.load(Ordering::SeqCst) {
            Pin::set(future, None);
            let callback = when_cancelled.take().unwrap();
            Poll::Ready(callback())

        } else {
            match future.as_pin_mut().unwrap().poll(waker) {
                Poll::Pending => {
                    *state.waker.lock().unwrap() = Some(waker.clone().into_waker());
                    Poll::Pending
                },
                a => a,
            }
        }
    }
}

There are three huge advantages to this:

  1. It's overall much shorter. Rather than using self.as_mut().future() you just use future. This makes working with pinned structs almost as convenient as unpinned structs!

  2. unsafe_pinned and unsafe_unpinned require you to specify the bounds/types of the fields twice. But with unsafe_project you don't need to.

  3. It fully supports disjoint borrows, unlike unsafe_pinned and unsafe_unpinned.

    This allowed me to take this horrible code... and replace it with this lovely code.

    In addition, there are a couple places where I need disjoint borrows, like here. Using unsafe_project works great, but I cannot use unsafe_pinned and unsafe_unpinned for this.

I ported my entire codebase to use unsafe_project, overall I'm very happy with it.

The only downside I found is that you can't call other self methods at the same time as using unsafe_project:

unsafe_project!(self.as_mut() => {
    pin signal1,
    pin signal2,
    mut left,
    mut right,
    mut callback,
});

// Error: doesn't work, since `self` is borrowed
self.foo();

Are you interested in adding in a macro like unsafe_project into pin-utils?

@Nemo157
Copy link
Member

Nemo157 commented Jan 8, 2019

A similar idea I had was to provide a derive that would create a projection struct covering all the fields, e.g. taking the same example:

#[derive(Debug, UnsafePinProject)]
#[must_use = "Futures do nothing unless polled"]
pub struct CancelableFuture<A, B> {
    state: Arc<CancelableFutureState>,
    #[project(pin)]
    future: Option<A>,
    when_cancelled: Option<B>,
}

would give something like

pub struct CancelableFuture<A, B> {
    state: Arc<CancelableFutureState>,
    future: Option<A>,
    when_cancelled: Option<B>,
}

struct CancelableFutureProjection<'a, A, B> {
    state: &'a mut Arc<CancelableFutureState>,
    future: Pin<&'a mut Option<A>>,
    when_cancelled: &'a mut Option<B>,
}

impl<A, B> CancelableFuture<A, B> {
    fn project<'a>(self: Pin<&'a mut Self>) -> CancelableFutureProjection<'a, A, B> {
        let this = unsafe { Pin::get_unchecked_mut(self) };
        CancelableFutureProjection {
            state: &mut this.state,
            future: unsafe { Pin::new_unchecked(&mut this.future) },
            when_cancelled: &mut this.when_cancelled,
        }
    }

(having UnsafePinProject be an actual trait would require GATs, so maybe a proc-macro-attribute would make more sense than a custom derive)

@Pauan
Copy link
Author

Pauan commented Jan 8, 2019

@Nemo157 Yeah, I also thought about something like that, but unsafe_project is much easier to create, and possibly faster (depending on whether Rust can optimize away the struct or not).

Of course there's nothing wrong with having both.

@taiki-e
Copy link
Member

taiki-e commented Jan 8, 2019

I wrote an attribute macro based on @Nemo157's idea: pin-project

It can be written the same example as follows:

#[unsafe_project]
#[derive(Debug)]
#[must_use = "Futures do nothing unless polled"]
pub struct CancelableFuture<A, B> {
    state: Arc<CancelableFutureState>,
    #[pin]
    future: Option<A>,
    when_cancelled: Option<B>,
}

@Nemo157
Copy link
Member

Nemo157 commented Jan 8, 2019

There are two reasons I prefer a type level macro for this instead of an expression level one:

  1. It gives a more accurate indication of where the unsafety lies. The soundness of the projection mainly depends on three things: which fields are being projected to, the drop implementation of the type and the Unpin implementation. Other than the actual projection these are all type level concerns. When you are verifying the projection at the type level then within the function actually doing the projection is safe (where possible I really like to try and avoid unsafe in the actual logic of implementations, there's almost always some way to wrap it in a small abstraction that's separate from the main function body).

  2. Avoiding accidental projection of different fields. It's not something that comes up often in Futures implementations, but sometimes a type will be doing pin projection in multiple functions. If that's done by listing the fields to project out in each function you might accidentally introduce inconsistencies between the functions that could let unsoundness creep in.

@Pauan
Copy link
Author

Pauan commented Jan 11, 2019

The soundness of the projection mainly depends on three things: which fields are being projected to, the drop implementation of the type and the Unpin implementation. Other than the actual projection these are all type level concerns.

I don't think that's quite correct. Consider the above CancelableFuture example. The Unpin impl says that it is Unpin if A is Unpin. But in order for that to be sound, it has to have some sort of constraint on B.

That constraint is provided by the Future impl, which constrains B to be FnOnce() -> A::Output. Thus technically it is only safe to project within the Future impl. So in that sense, the unsafe_project! macro is more correct, since it only allows usage within the safe areas.

However, I agree that a proc-macro or derive solution is nicer (and more intuitive?). So I'm fine with pushing on that as well. Just so long as there's some sort of solution for this (since right now there's none).


As a side note, @taiki-e 's implementation uses &mut T by default, requiring #[pin] annotation to use Pin<&mut T> instead.

That makes me uncomfortable, since projecting from Pin<&mut T> to Pin<&mut T.foo> is safer than projecting from Pin<&mut T> to &mut T.foo

So I think the defaults should be reversed: it should pin by default, and require a #[mut] or #[unpin] annotation to use &mut

Avoiding accidental projection of different fields. It's not something that comes up often in Futures implementations, but sometimes a type will be doing pin projection in multiple functions. If that's done by listing the fields to project out in each function you might accidentally introduce inconsistencies between the functions that could let unsoundness creep in.

This is a good argument in favor of a derive/proc-macro solution.

@Nemo157
Copy link
Member

Nemo157 commented Jan 11, 2019

I don't think that's quite correct. Consider the above CancelableFuture example. The Unpin impl says that it is Unpin if A is Unpin. But in order for that to be sound, it has to have some sort of constraint on B.

As long as you never pin-project to B then you don't care whether B is Unpin or not. If the proc-macro projection is the only projection done then really the correct constraint should be based on the field projected to: impl Unpin for CancelableFuture where Option<A>: Unpin {} (maybe the proc-macro could even optionally generate this implementation).

since projecting from Pin<&mut T> to Pin<&mut T.foo> is safer than projecting from Pin<&mut T> to &mut T.foo

As long as you never project to a pinned reference to the field (or otherwise assume the field is pinned) then projecting to an unpinned reference is safe. Also, pin-projecting to a field with the wrong conditional Unpin implementation is unsound, while projecting from a pin to an unpinned reference isn't. So I would argue that projecting to &mut T.foo is the safer default (as long as you are using the proc-macro as the only form of pin-projection).

@taiki-e
Copy link
Member

taiki-e commented Jan 12, 2019

@Nemo157

(maybe the proc-macro could even optionally generate this implementation)

It can reduce items on safety that human should ensure, so it seems preferable to add this option.
And I implemented this option as follows:

details

#[unsafe_project(Unpin)]
struct Foo<T, U> {
    #[pin]
    future: T,
    field: U,
}

// Automatically create the appropriate Unpin implementation.
// impl<T, U> Unpin for Foo<T, U> where T: Unpin {} // Conditional Unpin impl

@taiki-e
Copy link
Member

taiki-e commented Jun 22, 2020

I believe this issue can be closed because the current pin-project is safe.

Also, projection macros currently provided by pin-utils will be deprecated in favor of pin-project and pin-project-lite (#29).

@taiki-e taiki-e closed this as completed Jun 22, 2020
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

3 participants