Missing Pin API: are they sound?

I just realized that the following Pin APIs are missing:

impl Rc<T> {
    pub fn get_pin_mut(this: &mut Pin<Rc<T>>) -> Option<Pin<&mut T>>;
    pub unsafe fn get_pin_mut_unchecked(this: &mut Pin<Rc<T>>) -> Pin<&mut T>;
}

impl Rc<T>
where
    T: Clone,
{
    pub fn make_pin_mut(this: &mut Pin<Rc<T>>) -> Pin<&mut T>;
}

impl Arc<T> {
    pub fn get_pin_mut(this: &mut Pin<Arc<T>>) -> Option<Pin<&mut T>>;
    pub unsafe fn get_pin_mut_unchecked(this: &mut Pin<Arc<T>>) -> Pin<&mut T>;
}

impl Arc<T>
where
    T: Clone,
{
    pub fn make_pin_mut(this: &mut Pin<Arc<T>>) -> Pin<&mut T>;
}

Is this an oversight or are their straightforward implementations unsound (assuming for get_pin_mut_unchecked the same requirements for get_mut_unchecked)?

EDIT: changed from this: Pin<&mut Arc<T>> to this: &mut Pin<Arc<T>>, because this is what you need to handle when doing pin projection. Maybe a possilbe implementation is not straightforward anymore?

EDIT: fixed unsafe pub -> pub unsafe, thanks @steffahn

1 Like

Looks sound to me (with the edited API, i.e. the Pin directly at the Arc/Rc). Implementation should still be straightforward.

Well, seems like you do need to resort to pointer casting. Anyways, something like e.g.

impl Rc<T> {
    pub fn get_pin_mut(this: &mut Pin<Rc<T>>) -> Option<Pin<&mut T>> {
        unsafe {
            Rc::get_mut(NonNull::<Pin<Rc<T>>>::from(this).cast::<Rc<T>>().as_mut())
                .map(|r| Pin::new_unchecked(r))
        }
    }
}

should probably work.

1 Like

make_pin_mut probably isn't what would be expected, since make_mut (potentially) clones the pinned value.

Other than that, though, lifting get_mut and get_mut_unchecked to pinned versions should be sound and makes sense.

1 Like

Cloning a pinned value is allowed though? You can even dereference Pin<&T> to &T without any restrictions.

Edit: Ah, you’re not saying it’s unsound, just unexpected.

Edit 2: I wouldn’t necessarily think it’s unexpected. What else would you expect? When you have a pinned value you don’t really care about if it’s moved / cloned / etc… as a user, it’s just that the implementor of the type you’ve pinned a value of might care about moving.

I guess, : Clone + !Unpin types might not be too common anyway, but if someone implements a type that can be cloned but must not be moved (after some initialization) and that offers Pin<&mut self> API, then I personally think that make_pin_mut can be a useful function.

1 Like

I believe that several C++ bridge types will be Clone + !Unpin, but it seems doubtful they would end up in a Rust Rc<>.

1 Like

If you can Clone it, you can put it into an Rc, right? I mean the clone, put the clone in an Rc. You can also move the clone around freely before calling the first Pin<&self> (or Pin<&mut self>) method on it. Given this latter point, I’d be curious if there actually are any “C++ bridge” types that are Clone + !Unpin.

2 Likes

In theory, even if I don't like it, it should also be possible to transmute &mut Pin<Rc<T>> into &mut Rc<T>, because Pin is transparent with only one member. In any case, I almost never feel confident about using transmute.

I agree with @steffahn, it is a pretty uncommon situation, but the feature is meaningful. I can think of an !Unpin Future with an inner Arc that can be cloned to be sent across different threads. Then, you could need to pin-project the pinned Future to get a &mut Pin<Arc<T>> and to perform a copy-on-write on T during polling. It is surely a niche case -- I could say that probably no one needed Arc::get_pin_mut before today :grin: .

In any case, I will open an issue for these feature. Maybe some further discussion is needed, but at least you convinced me that the idea is sound.

I know! Just, this kind of transmute seems to be discouraged, AFAICT. I don’t know why, I do know that transmute is way to general in many cases. This is another time where I’m thinking that std needs some unsafe fn whatever_name<S, T>(&S) -> &T and unsafe fn whatever_name_mut<S, T>(&mut S) -> &mut T which is a safer alternative to using pointer cast’s. No need to handle null (in general I’m regularly unhappy that Rust choose to have the “default” pointer type include a null value) and also no risk of inadvertently extending the lifetime of the reference in the process.

3 Likes

Unlikely, as this requires the cloned value to be safe to bitmove before creating a new Pinned pointer. C++ bridged objects will need to either safe to bitcopy or pinned from birth, via construction in C++ and/or techniques like moveit.

That said, I see no real problem with providing the method, but it should have a noticable reminder on it that it will (potentially) create a new clone of the pinned value.

1 Like

I think I need to carefully look at all the (A)Rc APIs in order to understand if a _pin version is needed.

For instance, I just stumbled against the missing Arc::try_unwrap_pin (or Arc::try_pin_unwrap?), which, if I am not wrong, should return a Result<Pin<Box<T>>, Pin<Arc<T>>>.

It is a bit unfortunate that allocating containers don't compose well with Pin, but at this point I don't think we have any other choice than implementing the missing pieces for containers...

I don’t think you can convert an Arc<T> into a Box<T> without moving the value since Arc stores the reference counters in the allocated space together with the T.

1 Like

You are totally right -- I was looking at the code, and I just realized I forgot about ArcInner and its layout.

At this point I also don't know if I am able to achieve the same thing I was doing before my underling type become !Unpin...

I don't think those are safe due to the existance of Arc::get_mut and Rc::get_mut. If there is only a single reference, users can get safely get a mutable reference and then take the inner value out of of the container (e.g. using mem::replace or mem::take).

That will invalidate any assumptions that some codepath might have taken when it encountered the Pin<&mut T> and assumed a stable location of T.

Without the existance of those methods it should be safe, and I guess if those might not have been added if Pin existed back then. But since those are stabilized and safe, the pin methods can't be safe anymore.

These are the pinned versions of get_mut; double check the signatures.

You get a Pin<Arc<T>> by using Arc::pin, which creates a new allocation. You're correct in that it's unsound to have both Pin<Arc<T>> and Arc<T> own the same location, but that's never the case (in sound code, even with these added functions).

get_mut is a projection from &mut Arc<T> to &mut T, if the arc is unique. get_pinned_mut is a projection from &mut Pin<Arc<T>> to Pin<&mut T>, if the arc is unique. The value remains pinned; we just get a pin mut ref to work with.

1 Like

Thanks! I indeed missed that those methods take &mut Pin<Rc<T>> instead of &mut Rc<T>. Things should then be safe, since Rc<T>::get_mut() could only be called if T: Unpin - and under that condition the full project also seems safe.