From 769425a4eaca0dff46b22df5ab38580f5d64428f Mon Sep 17 00:00:00 2001 From: Kevin Reid Date: Thu, 14 Nov 2024 10:43:04 -0800 Subject: [PATCH 1/3] Expand `CloneToUninit` documentation. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Clarify relationship to `dyn` after #133003. * Add an example of using it with `dyn` as #133003 enabled. * Add an example of implementing it. * Add links to Rust Reference for the mentioned concepts. * Mention that its method should rarely be called. * Replace parameter name `dst` with `dest` to avoids confusion between “DeSTination” and “Dynamically-Sized Type”. * Various small corrections. --- library/core/src/clone.rs | 159 +++++++++++++++++++++++++++++++------- 1 file changed, 131 insertions(+), 28 deletions(-) diff --git a/library/core/src/clone.rs b/library/core/src/clone.rs index c777dd995a656..05462cea2f5b2 100644 --- a/library/core/src/clone.rs +++ b/library/core/src/clone.rs @@ -262,34 +262,137 @@ pub struct AssertParamIsCopy { _field: crate::marker::PhantomData, } -/// A generalization of [`Clone`] to dynamically-sized types stored in arbitrary containers. +/// A generalization of [`Clone`] to [dynamically-sized types][DST] stored in arbitrary containers. /// -/// This trait is implemented for all types implementing [`Clone`], and also [slices](slice) of all -/// such types. You may also implement this trait to enable cloning trait objects and custom DSTs -/// (structures containing dynamically-sized fields). +/// This trait is implemented for all types implementing [`Clone`], [slices](slice) of all +/// such types, and other dynamically-sized types in the standard library. +/// You may also implement this trait to enable cloning custom DSTs +/// (structures containing dynamically-sized fields), or use it as a supertrait to enable +/// cloning a [trait object]. +/// +/// This trait is normally used via operations on container types which support DSTs, +/// so you should not typically need to call `.clone_to_uninit()` explicitly except when +/// implementing such a container or otherwise performing explicit management of an allocation, +/// or when implementing `CloneToUninit` itself. /// /// # Safety /// -/// Implementations must ensure that when `.clone_to_uninit(dst)` returns normally rather than -/// panicking, it always leaves `*dst` initialized as a valid value of type `Self`. +/// Implementations must ensure that when `.clone_to_uninit(dest)` returns normally rather than +/// panicking, it always leaves `*dest` initialized as a valid value of type `Self`. +/// +/// # Examples +/// +// FIXME(#126799): when `Box::clone` allows use of `CloneToUninit`, rewrite these examples with it +// since `Rc` is a distraction. +/// +/// If you are defining a trait, you can add `CloneToUninit` as a supertrait to enable cloning of +/// `dyn` values of your trait: +/// +/// ``` +/// #![feature(clone_to_uninit)] +/// use std::rc::Rc; +/// +/// trait Foo: std::fmt::Debug + std::clone::CloneToUninit { +/// fn modify(&mut self); +/// fn value(&self) -> i32; +/// } +/// +/// impl Foo for i32 { +/// fn modify(&mut self) { +/// *self *= 10; +/// } +/// fn value(&self) -> i32 { +/// *self +/// } +/// } +/// +/// let first: Rc = Rc::new(1234); +/// +/// let mut second = first.clone(); +/// Rc::make_mut(&mut second).modify(); // make_mut() will call clone_to_uninit() +/// +/// assert_eq!(first.value(), 1234); +/// assert_eq!(second.value(), 12340); +/// ``` +/// +/// The following is an example of implementing `CloneToUninit` for a custom DST. +/// (It is essentially a limited form of what `derive(CloneToUninit)` would do, +/// if such a derive macro existed.) /// -/// # See also +/// ``` +/// #![feature(clone_to_uninit)] +/// use std::clone::CloneToUninit; +/// use std::mem::offset_of; +/// use std::rc::Rc; +/// +/// #[derive(PartialEq)] +/// struct MyDst { +/// flag: bool, +/// contents: T, +/// } /// -/// * [`Clone::clone_from`] is a safe function which may be used instead when `Self` is a [`Sized`] +/// unsafe impl CloneToUninit for MyDst { +/// unsafe fn clone_to_uninit(&self, dest: *mut u8) { +/// // The offset of `self.contents` is dynamic because it depends on the alignment of T +/// // which can be dynamic (if `T = dyn SomeTrait`). Therefore, we have to obtain it +/// // dynamically by examining `self`, rather than using `offset_of!`. +/// let offset_of_contents = +/// (&raw const self.contents).byte_offset_from_unsigned(&raw const *self); +/// +/// // Since `flag` implements `Copy`, we can just copy it. +/// // We use `pointer::write()` instead of assignment because the destination must be +/// // assumed to be uninitialized, whereas an assignment assumes it is initialized. +/// dest.add(offset_of!(Self, flag)).cast::().write(self.flag); +/// +/// // Note: if `flag` owned any resources (i.e. had a `Drop` implementation), then we +/// // must prepare to drop it in case `self.contents.clone_to_uninit()` panics. +/// // In this simple case, where we have exactly one field for which `mem::needs_drop()` +/// // might be true (`contents`), we don’t need to care about cleanup or ordering. +/// self.contents.clone_to_uninit(dest.add(offset_of_contents)); +/// +/// // All fields of the struct have been initialized, therefore the struct is initialized, +/// // and we have satisfied our `unsafe impl CloneToUninit` obligations. +/// } +/// } +/// +/// fn main() { +/// // Construct MyDst<[u8; 4]>, then coerce to MyDst<[u8]>. +/// let first: Rc> = Rc::new(MyDst { +/// flag: true, +/// contents: [1, 2, 3, 4], +/// }); +/// +/// let mut second = first.clone(); +/// // make_mut() will call clone_to_uninit(). +/// for elem in Rc::make_mut(&mut second).contents.iter_mut() { +/// *elem *= 10; +/// } +/// +/// assert_eq!(first.contents, [1, 2, 3, 4]); +/// assert_eq!(second.contents, [10, 20, 30, 40]); +/// } +/// ``` +/// +/// # See Also +/// +/// * [`Clone::clone_from`] is a safe function which may be used instead when [`Self: Sized`](Sized) /// and the destination is already initialized; it may be able to reuse allocations owned by -/// the destination. +/// the destination, whereas `clone_to_uninit` cannot, since its destination is assumed to be +/// uninitialized. /// * [`ToOwned`], which allocates a new destination container. /// /// [`ToOwned`]: ../../std/borrow/trait.ToOwned.html +/// [DST]: https://doc.rust-lang.org/reference/dynamically-sized-types.html +/// [trait object]: https://doc.rust-lang.org/reference/types/trait-object.html #[unstable(feature = "clone_to_uninit", issue = "126799")] pub unsafe trait CloneToUninit { - /// Performs copy-assignment from `self` to `dst`. + /// Performs copy-assignment from `self` to `dest`. /// - /// This is analogous to `std::ptr::write(dst.cast(), self.clone())`, - /// except that `self` may be a dynamically-sized type ([`!Sized`](Sized)). + /// This is analogous to `std::ptr::write(dest.cast(), self.clone())`, + /// except that `Self` may be a dynamically-sized type ([`!Sized`](Sized)). /// - /// Before this function is called, `dst` may point to uninitialized memory. - /// After this function is called, `dst` will point to initialized memory; it will be + /// Before this function is called, `dest` may point to uninitialized memory. + /// After this function is called, `dest` will point to initialized memory; it will be /// sound to create a `&Self` reference from the pointer with the [pointer metadata] /// from `self`. /// @@ -297,8 +400,8 @@ pub unsafe trait CloneToUninit { /// /// Behavior is undefined if any of the following conditions are violated: /// - /// * `dst` must be [valid] for writes for `size_of_val(self)` bytes. - /// * `dst` must be properly aligned to `align_of_val(self)`. + /// * `dest` must be [valid] for writes for `size_of_val(self)` bytes. + /// * `dest` must be properly aligned to `align_of_val(self)`. /// /// [valid]: crate::ptr#safety /// [pointer metadata]: crate::ptr::metadata() @@ -307,11 +410,11 @@ pub unsafe trait CloneToUninit { /// /// This function may panic. (For example, it might panic if memory allocation for a clone /// of a value owned by `self` fails.) - /// If the call panics, then `*dst` should be treated as uninitialized memory; it must not be + /// If the call panics, then `*dest` should be treated as uninitialized memory; it must not be /// read or dropped, because even if it was previously valid, it may have been partially /// overwritten. /// - /// The caller may also need to take care to deallocate the allocation pointed to by `dst`, + /// The caller may also need to take care to deallocate the allocation pointed to by `dest`, /// if applicable, to avoid a memory leak, and may need to take other precautions to ensure /// soundness in the presence of unwinding. /// @@ -319,15 +422,15 @@ pub unsafe trait CloneToUninit { /// that might have already been created. (For example, if a `[Foo]` of length 3 is being /// cloned, and the second of the three calls to `Foo::clone()` unwinds, then the first `Foo` /// cloned should be dropped.) - unsafe fn clone_to_uninit(&self, dst: *mut u8); + unsafe fn clone_to_uninit(&self, dest: *mut u8); } #[unstable(feature = "clone_to_uninit", issue = "126799")] unsafe impl CloneToUninit for T { #[inline] - unsafe fn clone_to_uninit(&self, dst: *mut u8) { + unsafe fn clone_to_uninit(&self, dest: *mut u8) { // SAFETY: we're calling a specialization with the same contract - unsafe { ::clone_one(self, dst.cast::()) } + unsafe { ::clone_one(self, dest.cast::()) } } } @@ -335,10 +438,10 @@ unsafe impl CloneToUninit for T { unsafe impl CloneToUninit for [T] { #[inline] #[cfg_attr(debug_assertions, track_caller)] - unsafe fn clone_to_uninit(&self, dst: *mut u8) { - let dst: *mut [T] = dst.with_metadata_of(self); + unsafe fn clone_to_uninit(&self, dest: *mut u8) { + let dest: *mut [T] = dest.with_metadata_of(self); // SAFETY: we're calling a specialization with the same contract - unsafe { ::clone_slice(self, dst) } + unsafe { ::clone_slice(self, dest) } } } @@ -346,21 +449,21 @@ unsafe impl CloneToUninit for [T] { unsafe impl CloneToUninit for str { #[inline] #[cfg_attr(debug_assertions, track_caller)] - unsafe fn clone_to_uninit(&self, dst: *mut u8) { + unsafe fn clone_to_uninit(&self, dest: *mut u8) { // SAFETY: str is just a [u8] with UTF-8 invariant - unsafe { self.as_bytes().clone_to_uninit(dst) } + unsafe { self.as_bytes().clone_to_uninit(dest) } } } #[unstable(feature = "clone_to_uninit", issue = "126799")] unsafe impl CloneToUninit for crate::ffi::CStr { #[cfg_attr(debug_assertions, track_caller)] - unsafe fn clone_to_uninit(&self, dst: *mut u8) { + unsafe fn clone_to_uninit(&self, dest: *mut u8) { // SAFETY: For now, CStr is just a #[repr(trasnsparent)] [c_char] with some invariants. // And we can cast [c_char] to [u8] on all supported platforms (see: to_bytes_with_nul). // The pointer metadata properly preserves the length (so NUL is also copied). // See: `cstr_metadata_is_length_with_nul` in tests. - unsafe { self.to_bytes_with_nul().clone_to_uninit(dst) } + unsafe { self.to_bytes_with_nul().clone_to_uninit(dest) } } } From 2cc999d0d4721655036b8cd89534eb3872d085cc Mon Sep 17 00:00:00 2001 From: Kevin Reid Date: Mon, 10 Mar 2025 13:54:07 -0700 Subject: [PATCH 2/3] Rewrite comments about dropping and leaking. --- library/core/src/clone.rs | 42 ++++++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/library/core/src/clone.rs b/library/core/src/clone.rs index 05462cea2f5b2..f6f4e2079156e 100644 --- a/library/core/src/clone.rs +++ b/library/core/src/clone.rs @@ -336,19 +336,29 @@ pub struct AssertParamIsCopy { /// // The offset of `self.contents` is dynamic because it depends on the alignment of T /// // which can be dynamic (if `T = dyn SomeTrait`). Therefore, we have to obtain it /// // dynamically by examining `self`, rather than using `offset_of!`. -/// let offset_of_contents = -/// (&raw const self.contents).byte_offset_from_unsigned(&raw const *self); -/// -/// // Since `flag` implements `Copy`, we can just copy it. -/// // We use `pointer::write()` instead of assignment because the destination must be -/// // assumed to be uninitialized, whereas an assignment assumes it is initialized. -/// dest.add(offset_of!(Self, flag)).cast::().write(self.flag); -/// -/// // Note: if `flag` owned any resources (i.e. had a `Drop` implementation), then we -/// // must prepare to drop it in case `self.contents.clone_to_uninit()` panics. -/// // In this simple case, where we have exactly one field for which `mem::needs_drop()` -/// // might be true (`contents`), we don’t need to care about cleanup or ordering. -/// self.contents.clone_to_uninit(dest.add(offset_of_contents)); +/// // +/// // SAFETY: `self` by definition points somewhere before `&self.contents` in the same +/// // allocation. +/// let offset_of_contents = unsafe { +/// (&raw const self.contents).byte_offset_from_unsigned(self) +/// }; +/// +/// // Clone each field of `self` into `dest`. +/// // +/// // Since `flag` is `Sized`, we could also clone it as +/// // dest.add(offset_of!(Self, flag)).cast::().write(self.flag.clone()); +/// // Since it is `Copy` (and therefore does not have a destructor), we could even write +/// // *dest.add(offset_of!(Self, flag)) = self.flag; +/// // but that must not be used for types with destructors, since it would read the place +/// // in order to drop the old value. We have chosen to do neither of those, to demonstrate +/// // the most general pattern. +/// // +/// // SAFETY: The caller must provide a `dest` such that these offsets are valid +/// // to write to. +/// unsafe { +/// self.flag.clone_to_uninit(dest.add(offset_of!(Self, flag))); +/// self.contents.clone_to_uninit(dest.add(offset_of_contents)); +/// } /// /// // All fields of the struct have been initialized, therefore the struct is initialized, /// // and we have satisfied our `unsafe impl CloneToUninit` obligations. @@ -370,6 +380,7 @@ pub struct AssertParamIsCopy { /// /// assert_eq!(first.contents, [1, 2, 3, 4]); /// assert_eq!(second.contents, [10, 20, 30, 40]); +/// assert_eq!(second.flag, true); /// } /// ``` /// @@ -414,9 +425,8 @@ pub unsafe trait CloneToUninit { /// read or dropped, because even if it was previously valid, it may have been partially /// overwritten. /// - /// The caller may also need to take care to deallocate the allocation pointed to by `dest`, - /// if applicable, to avoid a memory leak, and may need to take other precautions to ensure - /// soundness in the presence of unwinding. + /// The caller may wish to to take care to deallocate the allocation pointed to by `dest`, + /// if applicable, to avoid a memory leak (but this is not a requirement). /// /// Implementors should avoid leaking values by, upon unwinding, dropping all component values /// that might have already been created. (For example, if a `[Foo]` of length 3 is being From 96814ae55fd6201159ca00d79b79d9aae54cba0a Mon Sep 17 00:00:00 2001 From: Kevin Reid Date: Mon, 10 Mar 2025 13:54:07 -0700 Subject: [PATCH 3/3] Rewrite example to not deal with `Copy` at all. It also now demonstrates how to avoid memory leaks. --- library/core/src/clone.rs | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/library/core/src/clone.rs b/library/core/src/clone.rs index f6f4e2079156e..e0ac0bfc5289f 100644 --- a/library/core/src/clone.rs +++ b/library/core/src/clone.rs @@ -327,7 +327,7 @@ pub struct AssertParamIsCopy { /// /// #[derive(PartialEq)] /// struct MyDst { -/// flag: bool, +/// label: String, /// contents: T, /// } /// @@ -343,24 +343,26 @@ pub struct AssertParamIsCopy { /// (&raw const self.contents).byte_offset_from_unsigned(self) /// }; /// -/// // Clone each field of `self` into `dest`. -/// // -/// // Since `flag` is `Sized`, we could also clone it as -/// // dest.add(offset_of!(Self, flag)).cast::().write(self.flag.clone()); -/// // Since it is `Copy` (and therefore does not have a destructor), we could even write -/// // *dest.add(offset_of!(Self, flag)) = self.flag; -/// // but that must not be used for types with destructors, since it would read the place -/// // in order to drop the old value. We have chosen to do neither of those, to demonstrate -/// // the most general pattern. -/// // -/// // SAFETY: The caller must provide a `dest` such that these offsets are valid +/// // Clone the *sized* fields of `self` (just one, in this example). +/// // (By cloning this first and storing it temporarily in a local variable, we avoid +/// // leaking it in case of any panic, using the ordinary automatic cleanup of local +/// // variables. Such a leak would be sound, but undesirable.) +/// let label = self.label.clone(); +/// +/// // SAFETY: The caller must provide a `dest` such that these field offsets are valid /// // to write to. /// unsafe { -/// self.flag.clone_to_uninit(dest.add(offset_of!(Self, flag))); +/// // Clone the unsized field directly from `self` to `dest`. /// self.contents.clone_to_uninit(dest.add(offset_of_contents)); -/// } /// -/// // All fields of the struct have been initialized, therefore the struct is initialized, +/// // Now write all the sized fields. +/// // +/// // Note that we only do this once all of the clone() and clone_to_uninit() calls +/// // have completed, and therefore we know that there are no more possible panics; +/// // this ensures no memory leaks in case of panic. +/// dest.add(offset_of!(Self, label)).cast::().write(label); +/// } +/// // All fields of the struct have been initialized; therefore, the struct is initialized, /// // and we have satisfied our `unsafe impl CloneToUninit` obligations. /// } /// } @@ -368,7 +370,7 @@ pub struct AssertParamIsCopy { /// fn main() { /// // Construct MyDst<[u8; 4]>, then coerce to MyDst<[u8]>. /// let first: Rc> = Rc::new(MyDst { -/// flag: true, +/// label: String::from("hello"), /// contents: [1, 2, 3, 4], /// }); /// @@ -380,7 +382,7 @@ pub struct AssertParamIsCopy { /// /// assert_eq!(first.contents, [1, 2, 3, 4]); /// assert_eq!(second.contents, [10, 20, 30, 40]); -/// assert_eq!(second.flag, true); +/// assert_eq!(second.label, "hello"); /// } /// ``` ///