Re: [PATCH v2 11/12] rust: init: add `{pin_}chain` functions to `{Pin}Init<T, E>`

[Martin Rodriguez Reboredo]

On 7/24/23 11:08, Benno Lossin wrote:
> On 21.07.23 02:23, Martin Rodriguez Reboredo wrote:
> > On 7/19/23 11:21, Benno Lossin wrote:
> > > +/// An initializer returned by [`PinInit::pin_chain`].
> > > +pub struct ChainPinInit<I, F, T: ?Sized, E>(I, F, __internal::Invariant<(E, Box<T>)>);
> > > +
> > > +// SAFETY: the `__pinned_init` function is implemented such that it
> > > +// - returns `Ok(())` on successful initialization,
> > > +// - returns `Err(err)` on error and in this case `slot` will be dropped.
> > > +// - considers `slot` pinned.
> > > +unsafe impl<T: ?Sized, E, I, F> PinInit<T, E> for ChainPinInit<I, F, T, E>
> > > +where
> > > +    I: PinInit<T, E>,
> > > +    F: FnOnce(Pin<&mut T>) -> Result<(), E>,
> > > +{
> > > +    unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E> {
> > > +        // SAFETY: all requirements fulfilled since this function is `__pinned_init`.
> > > +        unsafe { self.0.__pinned_init(slot)? };
> > > +        // SAFETY: The above call initialized `slot` and we still have unique access.
> > > +        let val = unsafe { &mut *slot };
> > > +        // SAFETY: `slot` is considered pinned
> > > +        let val = unsafe { Pin::new_unchecked(val) };
> > > +        (self.1)(val).map_err(|e| {
> > > +            // SAFETY: `slot` was initialized above.
> > > +            unsafe { core::ptr::drop_in_place(slot) };
> > > +            e
> >
> > I might stumble upon an error like EAGAIN if I call `pin_chain` but that
> > means `slot` will be dropped. So my recommendation is to either not drop
> > the value or detail in `pin_chain`'s doc comment that the closure will
> > drop on error.
>
> This is a bit confusing to me, because dropping the value on returning `Err`
> is a safety requirement of `PinInit`. Could you elaborate why this is
> surprising? I can of course add it to the documentation, but I do not see
> how it could be implemented differently. Since if you do not drop the value
> here, nobody would know that it is still initialized.

I knew about the requirement of dropping on `Err`, but what has caught my
attention is that `{pin_}chain` might not abide with it per the doc
comment as it says that `self` is initialized before calling `f`...

    /// First initializes the value using `self` then calls the function
    /// `f` with the initialized value.

But one can not know what would happen when `f` fails, specially if
such failure can be ignored or it's only temporarily.

So then, the best course IMO is to mention that in some way the value is
still being initialized, kinda setting it up, and that it will be dropped
when an error is returned. WDYT?