From c96272ff2cddbba3881141e162092ce6e1800fdc Mon Sep 17 00:00:00 2001 From: Devin Jeanpierre Date: Mon, 24 Nov 2025 14:09:38 -0800 Subject: [PATCH] Revamp the `Reconstruct`/`ReconstructUnchecked` traits, and add a `construct`. After this CL, the state of affairs is that there's a safe and unsafe way to reconstruct a value in-place (without running assignment operators): **Unsafe:** `ctor::reconstruct(bar, c);`\ **Safe:** `*var = ctor::construct(c);` Where, in the unsafe version, `var : Pin<&mut T>`, and in the safe version,`var: &mut T`. This corresponds with the notion that `&mut T` is always safe to Rust-move to/from, but `Pin<&mut T>` has potentially difficult safety preconditions, which depend on the exact value in question. --- `Reconstruct` and `ReconstructUnchecked` were added during the design process for `ctor`, and don't make as much sense now. `Reconstruct` was designed, initially, so that you could safely reconstruct types which are never potentially-overlapping. Before C++20, it would be safe to do this for any `final` type. But as of C++20, the cat is completely out of the bag, and it is a property of _values_, not types. Even `final` types can be potentially-overlapping. So there is no completely safe interface dealing with `Pin<&mut T>`. (The completely safe interface uses `&mut T`, and it's called "rust assignment"! :]) So: it no longer makes sense as an `unsafe` trait. It should be removed from the trait definition. That means that `unsafe` should likely go on the method itself. That's what the `ReconstructUnchecked` trait does. It's the moral equivalent of `*p.into_inner_unchecked() = x`, but where `x` is a lazily-constructed value. So: this CL deletes `Reconstruct`, and for that matter removes `ReconstructUnchecked` and replaces it with a bare function. Also, this adds a basic `construct()` function, which you need to use for safe code dealing with bare values, and makes it easier to document what `reconstruct` does. In effect, `reconstruct` is the pinful version of `*p.as_mut().into_inner() = construct(c);`, which works even if the value is not Rust-movable. PiperOrigin-RevId: 836358194 --- .../test/struct/nonunpin/nonunpin_test.rs | 4 +- support/ctor.rs | 121 +++++------------- support/ctor_test.rs | 12 +- 3 files changed, 47 insertions(+), 90 deletions(-) diff --git a/rs_bindings_from_cc/test/struct/nonunpin/nonunpin_test.rs b/rs_bindings_from_cc/test/struct/nonunpin/nonunpin_test.rs index b7fe01868..2c6999678 100644 --- a/rs_bindings_from_cc/test/struct/nonunpin/nonunpin_test.rs +++ b/rs_bindings_from_cc/test/struct/nonunpin/nonunpin_test.rs @@ -3,7 +3,7 @@ // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception #![feature(negative_impls)] -use ctor::{ctor, emplace, CtorNew, ReconstructUnchecked}; +use ctor::{ctor, emplace, CtorNew}; use googletest::prelude::*; use nonunpin::{Nonmovable, Nonunpin, NonunpinStruct, ReturnsNonmovable}; use std::pin::Pin; @@ -92,7 +92,7 @@ fn test_union_field() { })); assert_eq!(my_union.cxx_class.value(), 4); std::mem::ManuallyDrop::drop(&mut Pin::into_inner_unchecked(my_union.as_mut()).cxx_class); - my_union.as_mut().reconstruct_unchecked(ctor!(MyUnion { int: 2 })); + ctor::reconstruct(my_union.as_mut(), ctor!(MyUnion { int: 2 })); assert_eq!(my_union.int, 2); } } diff --git a/support/ctor.rs b/support/ctor.rs index b8e450d80..b87385cdc 100644 --- a/support/ctor.rs +++ b/support/ctor.rs @@ -208,7 +208,10 @@ pub unsafe trait Ctor: Sized { /// /// # Safety /// - /// `dest` is valid for writes, pinned, and uninitialized. + /// `dest` is valid for writes and uninitialized. + /// + /// This function pins `dest`, so unless `Output: Unpin`, dest must not be moved or otherwise + /// invalidated. unsafe fn ctor(self, dest: *mut Self::Output) -> Result<(), Self::Error>; /// Converts a `Ctor` with error type `Infallible` to one with the provided error type `E`. @@ -422,6 +425,18 @@ pub fn ctor_error(e: E) -> impl Ctor { CtorError { e, marker: PhantomData } } +/// Construct and return a Rust-movable value from a `Ctor`. +pub fn construct(ctor: impl Ctor) -> T { + let mut value = MaybeUninit::uninit(); + // SAFETY: `value` is valid for writes and uninitialized, and `T` is `Unpin`. + unsafe { + ctor.ctor(value.as_mut_ptr()).unwrap(); + } + + // SAFETY: `ctor.ctor()` initialized `value`. + unsafe { value.assume_init() } +} + /// Trait for smart pointer types which support initialization via `Ctor`. /// /// A typical example would be `Box`, allows emplacing a `Ctor` into @@ -1278,6 +1293,8 @@ macro_rules! ctor { /// Destroy-then-reconstruct. Sidesteps `operator=`, instead reconstructing /// in-place. /// +/// For Rust-movable types, this is equivalent to `*p.as_mut().into_inner() = construct(ctor)`. +/// /// If the object cannot be destroyed/reconstructed in place (e.g. it is a base /// class subobject), the behavior is undefined. /// @@ -1289,76 +1306,32 @@ macro_rules! ctor { /// assignment. /// /// That means that e.g. instead of calling `x.assign(&*emplace!(foo))`, you can -/// directly call `x.reconstruct_unchecked(foo)` -- provided you are OK with the +/// directly call `reconstruct(x, foo)` -- provided you are OK with the /// differing constructor/destructor ordering, and satisfy safety criteria. /// /// # Safety /// -/// Dividing sources of UB by language ruleset: -/// -/// **C++**: The behavior is undefined if `self` is a base class subobject +/// The behavior is undefined if `p` is a base class subobject /// or `[[no_unique_address]]` member. /// /// See: http://eel.is/c++draft/basic.life#8 /// /// And see https://chat.google.com/room/AAAAl3r59xQ/auNOifgQa1c for discussion. /// -/// **Rust**: This is safe. Note that since this calls `drop()` on -/// the pinned pointer, it satisfies the pin guarantee, and is allowed to then -/// re-init it with something else. In effect, this is just the in-place Ctor -/// version of the existing method `Pin::set(T)`. -pub trait ReconstructUnchecked: Sized { - /// # Safety - /// See trait documentation. - unsafe fn reconstruct_unchecked( - self: Pin<&mut Self>, - ctor: impl Ctor, - ) { - let self_ptr = Pin::into_inner_unchecked(self) as *mut _; - core::ptr::drop_in_place(self_ptr); - abort_on_unwind(move || { - ctor.ctor(self_ptr).unwrap(); - }); - } +/// If `p` refers to an object stored anywhere else, such as a (non-`no_unique_address`) +/// field of a struct, or a local variable, then this is safe. +/// +/// Note that since this calls `drop()` on the pinned pointer, it satisfies the pin +/// guarantee, and is allowed to then re-init it with something else. In effect, this +/// is just the in-place Ctor version of the existing method `Pin::set(T)`. +pub unsafe fn reconstruct(p: Pin<&mut T>, ctor: impl Ctor) { + let raw_ptr = Pin::into_inner_unchecked(p) as *mut _; + core::ptr::drop_in_place(raw_ptr); + abort_on_unwind(move || { + ctor.ctor(raw_ptr).unwrap(); + }); } -impl ReconstructUnchecked for T {} - -/// Destroy-then-reconstruct. Sidesteps `operator=`, instead reconstructing -/// in-place. -/// -/// If `ctor` unwinds, the process will crash. -/// -/// This is a bit more fragile than, and a lot less common than, `operator=`, -/// but allows for taking advantage of copy/move elision more aggressively, -/// rather than requiring materialization into a temporary before triggering -/// assignment. -/// -/// That means that e.g. instead of calling `x.assign(&*emplace!(foo))`, you can -/// directly call `x.reconstruct(foo)` -- provided you are OK with the differing -/// constructor/destructor ordering. -/// -/// # Implementation safety -/// -/// Implementors must ensure that it is safe to destroy and reconstruct the -/// object. Most notably, if `Self` is a C++ class, it must not be a base class. -/// See http://eel.is/c++draft/basic.life#8 -/// -/// # Safety -/// -/// Note that this is not safe to call on `[[no_unique_address]]` member -/// variables, but the interface is marked safe, because those can only be -/// produced via unsafe means. -pub unsafe trait Reconstruct: ReconstructUnchecked { - fn reconstruct(self: Pin<&mut Self>, ctor: impl Ctor) { - unsafe { self.reconstruct_unchecked(ctor) }; - } -} - -/// Safety: anything implementing `Unpin` is Rust-assignable, and -/// Rust-assignment is inherently destroy+reconstruct. -unsafe impl Reconstruct for T {} - /// Run f, aborting if it unwinds. /// /// Because this aborts on unwind, f is not required to be unwind-safe. @@ -1427,15 +1400,7 @@ where T: Unpin + CtorNew<&'a T, Error = Infallible>, { fn assign(mut self: Pin<&mut Self>, src: &'a Self) { - if core::mem::needs_drop::() { - let mut constructed = MaybeUninit::uninit(); - unsafe { - T::ctor_new(src).ctor(constructed.as_mut_ptr()).unwrap(); - *self = constructed.assume_init(); - } - } else { - self.reconstruct(T::ctor_new(src)); - } + *self = construct(T::ctor_new(src)); } } @@ -1444,15 +1409,7 @@ where T: Unpin + CtorNew, Error = Infallible>, { fn assign(mut self: Pin<&mut Self>, src: RvalueReference<'a, Self>) { - if core::mem::needs_drop::() { - let mut constructed = MaybeUninit::uninit(); - unsafe { - T::ctor_new(src).ctor(constructed.as_mut_ptr()).unwrap(); - *self = constructed.assume_init(); - } - } else { - self.reconstruct(T::ctor_new(src)); - } + *self = construct(T::ctor_new(src)); } } @@ -1461,15 +1418,7 @@ where T: Unpin + CtorNew, Error = Infallible>, { fn assign(mut self: Pin<&mut Self>, src: ConstRvalueReference<'a, Self>) { - if core::mem::needs_drop::() { - let mut constructed = MaybeUninit::uninit(); - unsafe { - T::ctor_new(src).ctor(constructed.as_mut_ptr()).unwrap(); - *self = constructed.assume_init(); - } - } else { - self.reconstruct(T::ctor_new(src)); - } + *self = construct(T::ctor_new(src)); } } diff --git a/support/ctor_test.rs b/support/ctor_test.rs index 97c5d5324..f9596d4d0 100644 --- a/support/ctor_test.rs +++ b/support/ctor_test.rs @@ -8,7 +8,7 @@ use ctor::{ copy, ctor, emplace, mov, try_emplace, Assign, Ctor, CtorNew, Emplace, FnCtor, - ManuallyDropCtor, Reconstruct, RecursivelyPinned, RvalueReference, Slot, UnreachableCtor, + ManuallyDropCtor, RecursivelyPinned, RvalueReference, Slot, UnreachableCtor, }; use googletest::gtest; use std::cell::RefCell; @@ -18,6 +18,12 @@ use std::pin::Pin; use std::rc::Rc; use std::sync::{Arc, Mutex}; +#[gtest] +fn test_construct() { + let x = ctor::construct(u32::ctor_new(())); + assert_eq!(x, 0); +} + /// Only really need one test for the new super-let syntax, as it uses the same /// building blocks as the old syntax. #[gtest] @@ -259,7 +265,9 @@ fn test_ctor_macro_union() { // First, should drop myunion.x. unsafe { ManuallyDrop::drop(&mut Pin::into_inner_unchecked(my_union.as_mut()).x) } // Second, we can overwrite. - my_union.as_mut().reconstruct(ctor!(MyUnion { y: 24 })); + unsafe { + ctor::reconstruct(my_union.as_mut(), ctor!(MyUnion { y: 24 })); + } assert_eq!(unsafe { my_union.y }, 24); }