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

RefInit: RAII wrapper for initialized MaybeUninit references #338

Closed
ajwock opened this issue Feb 19, 2024 · 2 comments
Closed

RefInit: RAII wrapper for initialized MaybeUninit references #338

ajwock opened this issue Feb 19, 2024 · 2 comments
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@ajwock
Copy link

ajwock commented Feb 19, 2024

Proposal

Problem statement

Values initialized by MaybeUninit ref APIs and MaybeUninit slice APIs will have their destructors leaked unless the user manually drops them.

Motivating examples or use cases

rust-lang/rust#80376 MaybeUninit::write_slice_cloned() makes it very easy to accidentally leak

https://users.rust-lang.org/t/is-there-a-way-to-copy-t-into-mut-maybeuninit-t-without-unsafe/51301/2

This is also an issue that has come up in discussion with this ACP: #156

A similar solution to the below is also used in MaybeUninit::clone_from_slice to Drop elements that are already initialized in the case of a panic: https://github.com/rust-lang/rust/blob/02438348b9c26c0d657cc7b990fd1f45a8c0c736/library/core/src/mem/maybe_uninit.rs#L1128

Solution sketch

Introducing: RefInit.
An RAII wrapper that wraps the now-initialized reference, and drops the elements inside of it when it is dropped.

A rough sketch demonstrating what a solution might look like for the current set of APIs (including pending ones from #156 )

struct RefInit<'a, T: ?Sized> {
    val: &'a mut T,
}

impl<'a, T: ?Sized> Drop for RefInit<'a, T> {
    fn drop(&mut self) {
        // Safety:  RefInit is only yielded from calls on MaybeUninit slices,
        // so Drop will not be run again when the source of the reference goes out of
        // scope.
        unsafe {
            std::ptr::drop_in_place(self.val);
        }
    }
}

impl<'a, T: ?Sized> Deref for RefInit<'a, T> {
    type Target = T;

    fn deref(&self) -> &Self::Target {
        self.val
    }
}

impl<'a, T: ?Sized> DerefMut for RefInit<'a, T> {
    fn deref_mut(&mut self) -> &mut Self::Target {
        self.val
    }
}

impl<'a, T> RefInit<'a, T> {
    pub unsafe fn assume_init_mut(uninit_ref: &'a mut MaybeUninit<T>) -> RefInit<'a, T> {
        RefInit {
            val: unsafe { MaybeUninit::assume_init_mut(uninit_ref) },
        }
    }

    pub fn write(uninit_ref: &'a mut MaybeUninit<T>, val: T) -> RefInit<'a, T> {
        RefInit {
            val: MaybeUninit::write(uninit_ref, val),
        }
    }

}

impl<'a, T: ?Sized> RefInit<'a, T> {
    pub fn leak(self) -> &'a mut T {
        let ret = unsafe {
            &mut *(self.val as *mut T)
        };
        let _ = std::mem::ManuallyDrop::new(self);
        ret
    }
}

impl<'a, T> RefInit<'a, [T]> {
    pub unsafe fn slice_assume_init_mut(slice: &'a mut [MaybeUninit<T>]) -> RefInit<'a, [T]> {
        RefInit {
            val: unsafe { MaybeUninit::slice_assume_init_mut(slice) },
        }
    }

    pub fn copy_from_slice(slice: &'a mut [MaybeUninit<T>], src: &[T]) -> RefInit<'a, [T]>
    where
        T: Copy,
    {
        RefInit {
            val: MaybeUninit::copy_from_slice(slice, src),
        }
    }

    pub fn clone_from_slice(slice: &'a mut [MaybeUninit<T>], src: &[T]) -> RefInit<'a, [T]>
    where
        T: Clone,
    {
        RefInit {
            val: MaybeUninit::clone_from_slice(slice, src),
        }
    }

    pub fn fill(slice: &'a mut [MaybeUninit<T>], value: T) -> RefInit<'a, [T]>
    where
        T: Clone,
    {
        RefInit {
            val: MaybeUninit::fill(slice, value),
        }
    }

    pub fn fill_from<I>(
        slice: &'a mut [MaybeUninit<T>],
        it: I,
    ) -> (RefInit<'a, [T]>, &'a mut [MaybeUninit<T>])
    where
        I: IntoIterator<Item = T>,
    {
        let (initialized, remainder) = MaybeUninit::fill_from(slice, it);
        (RefInit { val: initialized }, remainder)
    }

    pub fn fill_with<F>(slice: &'a mut [MaybeUninit<T>], f: F) -> RefInit<'a, [T]>
    where
        F: FnMut() -> T,
    {
        RefInit {
            val: MaybeUninit::fill_with(slice, f),
        }
    }
}

Alternatives

Alternatives are rather lacking here. Manually dropping is an alternative, albeit not a safe alternative.

Linking relevant APIs

rust-lang/rust#63567

rust-lang/rust#79995

#156

@ajwock ajwock added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Feb 19, 2024
@ajwock
Copy link
Author

ajwock commented Feb 20, 2024

One more thought.

Would it make sense to change most of the APIs listed above in MaybeUninit to return RefInit rather than having this be an option on the side?

All such APIs in their current state will cause a destructor leak without future use of unsafe, and with RefInit's leak function the old behavior can be preserved if desired while also making it more explicit that destructors will be leaked unless other measures are taken.

For assume_init_mut / slice_assume_init_mut there should be a separate method for the InitRef version though, because in those you aren't necessarily initializing elements on the spot.

@ajwock ajwock changed the title RefInit: Dropper for initialized MaybeUninit references RefInit: RAII wrapper for initialized MaybeUninit references Feb 20, 2024
@traviscross
Copy link

We talked about this in the libs-api call today. People were uncertain about whether the motivation was strong enough. We found ourselves speculating a bit about use cases.

Since this can be implemented as a library, perhaps you could do that, and then add some examples in that library that demonstrate the use cases clearly. That would be the best next step here to help with future consideration of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

3 participants