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

Tracking Issue for core::clone::CloneToUninit trait #126799

Open
1 of 5 tasks
dtolnay opened this issue Jun 21, 2024 · 6 comments
Open
1 of 5 tasks

Tracking Issue for core::clone::CloneToUninit trait #126799

dtolnay opened this issue Jun 21, 2024 · 6 comments
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@dtolnay
Copy link
Member

dtolnay commented Jun 21, 2024

Feature gate: #![feature(clone_to_uninit)]

This is a tracking issue for CloneToUninit and its impls. This trait backs the behavior of Rc::make_mut and Arc::make_mut, and likely in the future also Box::clone.

Public API

// core::clone

pub unsafe trait CloneToUninit {
    unsafe fn clone_to_uninit(&self, dst: *mut Self);
}

unsafe impl<T: Clone> CloneToUninit for T;
unsafe impl<T: Clone> CloneToUninit for [T];

// TODO:
// unsafe impl CloneToUninit for str;
// unsafe impl CloneToUninit for CStr;
// unsafe impl CloneToUninit for OsStr;
// unsafe impl CloneToUninit for Path;

Steps / History

Unresolved Questions

Footnotes

  1. https://std-dev-guide.rust-lang.org/feature-lifecycle/stabilization.html

@dtolnay dtolnay added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Jun 21, 2024
@dtolnay
Copy link
Member Author

dtolnay commented Jun 22, 2024

@kpreid #116113 (comment)

I see a big reason to be cautious about stabilizing CloneToUninit: it is a trait both unsafe to implement and unsafe to call, yet it provides something that many slice-like (as opposed to dyn-like) DSTs would want to implement. We wouldn't want to lock in a doubly unsafe interface as a key interop trait, rather than some fully safe “placement clone” (in the same sense as “placement new”) that can solve this problem.

I thought about what a safer primitive could be that still enables what we need in make_mut. I think it might (almost?) exists already:

impl<T, A> Rc<T, A>
where
    T: ?Sized,
    A: Allocator,
    Box<T, RcAllocator<A>>: Clone,
{
    pub fn make_mut(this: &mut Self) -> &mut T {...}
}

where RcAllocator<A: Allocator> is an allocator whose every allocation points to the value part of a MaybeUninit<RcBox<T>>. When you ask it to allocate with the Layout of some type T, it delegates to the underlying allocator to allocate the Layout of RcBox<T> and offsets that pointer.

#[repr(C)]
struct RcBox<T: ?Sized> {
strong: Cell<usize>,
weak: Cell<usize>,
value: T,
}

Then make_mut works by turning the pointer held by the Rc<T, A> into a Box<T, RcAllocator<A>>, cloning it, and turning the Box<T, RcAllocator<A>> back into Rc<T, A>.

Instead of implementing an unsafe trait CloneToUninit, types plug into make_mut support by implementing the safe trait Clone for Box<MyType, A> where A: Allocator. The concrete allocator type RcAllocator itself is not public; your impls are written using a generic A as they already universally are. Notice no unsafe in this impl!

impl<T: Clone, A: Allocator + Clone> Clone for Box<[T], A> {
fn clone(&self) -> Self {
let alloc = Box::allocator(self).clone();
self.to_vec_in(alloc).into_boxed_slice()
}

What I don't know is whether Box's use of Unique throws a wrench in the works. As I currently understand it, I think it does not. Casting Rc's NonNull<RcBox<T>> into Box<T, RcAllocator<A>> in order to clone it would not be sound, but I also think that's not necessary for the above to work. If we can rearrange Rc's layout a bit to achieve a guarantee that Rc<T, A> has the same layout as Box<T, RcAllocator<A>>, then we'd only be casting &Rc<T, A> to &Box<T, RcAllocator<A>> to pass into the clone impl, and as such, Unique never comes into play. Obviously I can have as many &Box<T> aliasing one another's allocations as I want. An actual owned Box<T, RcAllocator<A>> would never exist except for the value that becomes the return value of the clone, during which it is unique. I think it all works out as required, but I wouldn't be surprised if I am missing something.

@kpreid
Copy link
Contributor

kpreid commented Jun 22, 2024

Box<T, RcAllocator<A>>: Clone,

One thing that this doesn't help with, that the current CloneToUninit does, is cloning into a different container type. I'm not sure if that's useful enough to be worthwhile, but it feels to me like it might be an important piece of a future Rust where there are fewer obstacles to using DSTs, which is one of the things I hoped to do by having (a safer version of) CloneToUninit be public.

@the8472
Copy link
Member

the8472 commented Jun 22, 2024

Tangentially, a generic way to build a container type and then clone or write into its uninitialized payload slot would fit nicely into a solution for rust-lang/wg-allocators#90

@dtolnay
Copy link
Member Author

dtolnay commented Jun 24, 2024

https://doc.rust-lang.org/nightly/std/clone/trait.CloneToUninit.html#implementors

I think we should consider adding #[doc(hidden)] to the T: Copy specializations. They are just distracting when it comes to documenting the public API of this trait.

@the8472
Copy link
Member

the8472 commented Jun 24, 2024

The std-dev-guide says we should use private specialization traits, which we do in most places. The implementation PR promoted a private trait to a public one, so it should have changed the specialization to a subtrait.

@GrigorenkoPV
Copy link
Contributor

The std-dev-guide says we should use private specialization traits, which we do in most places. The implementation PR promoted a private trait to a public one, so it should have changed the specialization to a subtrait.

I have implemented this suggestion in #126877 (scope creep, yay).

Also, this has an additional benefit of no longer needing any #[doc(hidden)] to hide the mess:

I think we should consider adding #[doc(hidden)] to the T: Copy specializations. They are just distracting when it comes to documenting the public API of this trait.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants