Re: [PATCH 1/9] rust: list: add ListArc

[Alice Ryhl]

On Wed, Apr 3, 2024 at 5:51 PM Benno Lossin <benno.lossin@xxxxxxxxx> wrote:
>
> On 02.04.24 14:16, Alice Ryhl wrote:
> > +impl<T: ListArcSafe<ID>, const ID: u64> ListArc<T, ID> {
> > +    /// Constructs a new reference counted instance of `T`.
> > +    pub fn try_new(contents: T) -> Result<Self, AllocError> {
> > +        Ok(Self::from_unique(UniqueArc::try_new(contents)?))
> > +    }
> > +
> > +    /// Use the given initializer to in-place initialize a `T`.
> > +    ///
> > +    /// If `T: !Unpin` it will not be able to move afterwards.
> > +    pub fn pin_init<E>(init: impl PinInit<T, E>) -> error::Result<Self>
> > +    where
> > +        Error: From<E>,
> > +    {
> > +        Ok(Self::from_pin_unique(UniqueArc::pin_init(init)?))
> > +    }
>
> pin-init has a general trait for this: InPlaceInit. I don't know if the
> other functions that it provides would help you.

I will use that.

> > +}
> > +
> > +impl<T, const ID: u64> ListArc<T, ID>
> > +where
> > +    T: ListArcSafe<ID> + ?Sized,
> > +{
> > +    /// Convert a [`UniqueArc`] into a [`ListArc`].
> > +    pub fn from_unique(mut unique: UniqueArc<T>) -> Self {
> > +        // SAFETY: We have a `UniqueArc`, so there is no `ListArc`.
> > +        unsafe { T::on_create_list_arc_from_unique(&mut unique) };
> > +        let arc = Arc::from(unique);
> > +        // SAFETY: We just called `on_create_list_arc_from_unique` on an arc without a `ListArc`,
> > +        // so we can create a `ListArc`.
> > +        unsafe { Self::transmute_from_arc(arc) }
> > +    }
> > +
> > +    /// Convert a pinned [`UniqueArc`] into a [`ListArc`].
> > +    pub fn from_pin_unique(unique: Pin<UniqueArc<T>>) -> Self {
> > +        // SAFETY: We continue to treat this pointer as pinned after this call, since `ListArc`
> > +        // implicitly pins its value.
>
> This is not sufficient, since you also rely on `Self::from_unique` to
> handle the parameter as if it were pinned, which it does not. Since it
> calls `T::on_create_list_arc_from_unique` which just gets a `&mut self`
> as a parameter and it could move stuff out.

I'll swap these so that from_unique calls from_pin_unique instead.
I'll also change on_create_list_arc_from_unique to take a Pin<&mut
Self>.

> > +        Self::from_unique(unsafe { Pin::into_inner_unchecked(unique) })
> > +    }
> > +
> > +    /// Like [`from_unique`], but creates two `ListArcs`.
> > +    ///
> > +    /// The two ids must be different.
> > +    ///
> > +    /// [`from_unique`]: ListArc::from_unique
> > +    pub fn pair_from_unique<const ID2: u64>(mut unique: UniqueArc<T>) -> (Self, ListArc<T, ID2>)
> > +    where
> > +        T: ListArcSafe<ID2>,
> > +    {
> > +        assert_ne!(ID, ID2);
>
> Can this be a `build_assert!`?

Most likely.

> > +
> > +        // SAFETY: We have a `UniqueArc`, so we can call this method.
>
> I liked the comment from above better:
>
>      // SAFETY: We have a `UniqueArc`, so there is no `ListArc`.
>
> Maybe use the variation "so there are no `ListArc`s for any ID.".

Will do.

> > +        unsafe { <T as ListArcSafe<ID>>::on_create_list_arc_from_unique(&mut unique) };
> > +        // SAFETY: We have a `UniqueArc`, so we can call this method. The two ids are not equal.
> > +        unsafe { <T as ListArcSafe<ID2>>::on_create_list_arc_from_unique(&mut unique) };
> > +
> > +        let arc1 = Arc::from(unique);
> > +        let arc2 = Arc::clone(&arc1);
> > +
> > +        // SAFETY: We just called `on_create_list_arc_from_unique` on an arc without a `ListArc`,
> > +        // so we can create a `ListArc`.
>
> I would mention the two different IDs again.

Sure.

> > +        unsafe {
> > +            (
> > +                Self::transmute_from_arc(arc1),
> > +                ListArc::transmute_from_arc(arc2),
> > +            )
> > +        }
> > +    }
> > +
> > +    /// Like [`pair_from_unique`], but uses a pinned arc.
> > +    ///
> > +    /// The two ids must be different.
> > +    ///
> > +    /// [`pair_from_unique`]: ListArc::pair_from_unique
> > +    pub fn pair_from_pin_unique<const ID2: u64>(
> > +        unique: Pin<UniqueArc<T>>,
> > +    ) -> (Self, ListArc<T, ID2>)
> > +    where
> > +        T: ListArcSafe<ID2>,
> > +    {
> > +        // SAFETY: We continue to treat this pointer as pinned after this call, since `ListArc`
> > +        // implicitly pins its value.
> > +        Self::pair_from_unique(unsafe { Pin::into_inner_unchecked(unique) })
> > +    }
> > +
> > +    /// Transmutes an [`Arc`] into a `ListArc` without updating the tracking inside `T`.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// * The value must not already have a `ListArc` reference.
> > +    /// * The tracking inside `T` must think that there is a `ListArc` reference.
> > +    #[inline]
> > +    unsafe fn transmute_from_arc(me: Arc<T>) -> Self {
> > +        // INVARIANT: By the safety requirements, the invariants on `ListArc` are satisfied.
> > +        // SAFETY: ListArc is repr(transparent).
> > +        unsafe { core::mem::transmute(me) }
>
> Why do you need a transmute here? Can't you just construct the struct?
>
>      Self { arc: me }

Yeah, I guess that works.

Alice