From 45a49e61994d9a0eda33d947bfffa4cfdeac0637 Mon Sep 17 00:00:00 2001 From: liamjdavis Date: Wed, 28 May 2025 00:31:50 -0700 Subject: [PATCH] pin-init: enhance safety documentation for macro-generated code Add comprehensive safety documentation to pin-init macros, particularly for the #[pin_data] macro which generates unsafe accessor functions. The enhanced documentation clarifies: - Safety requirements for slot pointer validity and alignment - Pinning invariants that must be upheld by implementers - Memory safety contracts for macro-generated unsafe functions - Proper usage patterns for the pin-init API This addresses gaps in the safety documentation that made it difficult for users to understand the safety contracts when implementing custom initializers or working with the generated code. Also updates test expectations to match the new macro-generated documentation. Signed-off-by: Liam Davis --- src/__internal.rs | 39 ++++++++++++---- src/lib.rs | 27 ++---------- src/macros.rs | 44 ++++++++++++++++--- .../ui/compile-fail/init/invalid_init.stderr | 4 +- tests/ui/expand/many_generics.expanded.rs | 17 +++++++ tests/ui/expand/pin-data.expanded.rs | 11 +++++ tests/ui/expand/pinned_drop.expanded.rs | 11 +++++ 7 files changed, 112 insertions(+), 41 deletions(-) diff --git a/src/__internal.rs b/src/__internal.rs index 90f18e9a..8a7db341 100644 --- a/src/__internal.rs +++ b/src/__internal.rs @@ -51,11 +51,17 @@ where /// /// # Safety /// -/// Only the `init` module is allowed to use this trait. +/// This `unsafe trait` should only be implemented by the `#[pin_data]` macro. The implementer must ensure: +/// - `Self::PinData` is the correct metadata type for `Self` and implements `PinData` +/// - `__pin_data()` returns a valid instance that accurately reflects the structural pinning and field layout of `Self` +/// - The metadata provides sound field accessors that uphold their safety contracts +/// - Incorrect metadata can lead to undefined behavior when used by the pin-init system pub unsafe trait HasPinData { type PinData: PinData; - #[expect(clippy::missing_safety_doc)] + /// # Safety + /// + /// This method should only be called by macros in the pin-init crate. unsafe fn __pin_data() -> Self::PinData; } @@ -63,7 +69,12 @@ pub unsafe trait HasPinData { /// /// # Safety /// -/// Only the `init` module is allowed to use this trait. +/// This `unsafe trait` should only be implemented by the `#[pin_data]` macro for its generated +/// metadata struct. The implementer must ensure: +/// - `Self` is `Copy` +/// - `Self::Datee` is the correct struct type implementing `HasPinData` +/// - Generated field accessors uphold their safety contracts and correctly use the appropriate +/// initialization methods pub unsafe trait PinData: Copy { type Datee: ?Sized + HasPinData; @@ -81,11 +92,16 @@ pub unsafe trait PinData: Copy { /// /// # Safety /// -/// Only the `init` module is allowed to use this trait. +/// This `unsafe trait` should only be implemented by the `pin-init` macro system. The implementer must ensure: +/// - `Self::InitData` accurately represents the initialization structure of `Self` +/// - `__init_data()` returns a valid metadata instance for this type +/// - The metadata correctly reflects the field layout and initialization requirements pub unsafe trait HasInitData { type InitData: InitData; - #[expect(clippy::missing_safety_doc)] + /// # Safety + /// + /// This method should only be called by macros in the pin-init crate. unsafe fn __init_data() -> Self::InitData; } @@ -93,7 +109,10 @@ pub unsafe trait HasInitData { /// /// # Safety /// -/// Only the `init` module is allowed to use this trait. +/// This `unsafe trait` should only be implemented by the `pin-init` macro system. The implementer must ensure: +/// - `Self` is `Copy` +/// - `Self::Datee` correctly corresponds to the type `Self` represents for initialization purposes +/// - The trait is used consistently within the pin-init system pub unsafe trait InitData: Copy { type Datee: ?Sized + HasInitData; @@ -116,12 +135,16 @@ impl Clone for AllData { impl Copy for AllData {} -// SAFETY: TODO. +// SAFETY: This implementation upholds the `InitData` invariants that `AllData` is `Copy`, and +// `Self::Datee` is `T`, which correctly represents the type `AllData` is concerned with +// for initialization, as used by the `pin_init!` macros. The `make_closure` method is inherited +// and is a safe identity function, fulfilling trait expectations. unsafe impl InitData for AllData { type Datee = T; } -// SAFETY: TODO. +// SAFETY: `__init_data` returns `AllData` which is a correct `InitData` implementation +// for type `T`. The function itself performs no unsafe memory operations. unsafe impl HasInitData for T { type InitData = AllData; diff --git a/src/lib.rs b/src/lib.rs index 3e5fe84a..ae750cf6 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -763,7 +763,10 @@ macro_rules! stack_try_pin_init { /// /// let init = pin_init!(&this in Buf { /// buf: [0; 64], -/// // SAFETY: TODO. +/// // SAFETY: `this.as_ptr()` (within `pin_init!(&this in ...)` context) yields a `*mut Buf` +/// // that the macro guarantees is valid for initialization (non-null, aligned, points to memory for `Buf`). +/// // This makes it dereferenceable for `addr_of_mut!`, which safely creates a raw pointer to the `buf` field, +/// // even if uninitialized, as it avoids creating a reference. /// ptr: unsafe { addr_of_mut!((*this.as_ptr()).buf).cast() }, /// pin: PhantomPinned, /// }); @@ -1409,28 +1412,6 @@ unsafe impl PinInit for T { } } -// SAFETY: when the `__init` function returns with -// - `Ok(())`, `slot` was initialized and all pinned invariants of `T` are upheld. -// - `Err(err)`, slot was not written to. -unsafe impl Init for Result { - unsafe fn __init(self, slot: *mut T) -> Result<(), E> { - // SAFETY: `slot` is valid for writes by the safety requirements of this function. - unsafe { slot.write(self?) }; - Ok(()) - } -} - -// SAFETY: when the `__pinned_init` function returns with -// - `Ok(())`, `slot` was initialized and all pinned invariants of `T` are upheld. -// - `Err(err)`, slot was not written to. -unsafe impl PinInit for Result { - unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E> { - // SAFETY: `slot` is valid for writes by the safety requirements of this function. - unsafe { slot.write(self?) }; - Ok(()) - } -} - /// Smart pointer containing uninitialized memory and that can write a value. pub trait InPlaceWrite { /// The type `Self` turns into when the contents are initialized. diff --git a/src/macros.rs b/src/macros.rs index 9ced6307..e013e8aa 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -518,7 +518,11 @@ macro_rules! __pinned_drop { } ), ) => { - // SAFETY: TODO. + // SAFETY: This macro generates the `unsafe impl PinnedDrop`. This is safe as it's + // an internal part of the `#[pinned_drop]` proc-macro, which ensures correct + // transformation of the user's `impl`. The added `OnlyCallFromDrop` token restricts + // `PinnedDrop::drop` calls to the actual drop process (via the `Drop::drop` impl + // generated by `#[pin_data(PinnedDrop)]`) where `self` is pinned and valid. unsafe $($impl_sig)* { // Inherit all attributes and the type/ident tokens for the signature. $(#[$($attr)*])* @@ -878,7 +882,12 @@ macro_rules! __pin_data { } } - // SAFETY: TODO. + // SAFETY: The `__pin_data` macro ensures this `impl PinData` for `__ThePinData` is + // safe by guaranteeing that `__ThePinData` is `Copy`, `PinData::Datee` is correctly + // set to `$name` (which itself implements `HasPinData`), and + // that all generated field accessor methods on `__ThePinData` are sound, uphold their + // individual safety contracts (such as ensuring valid `slot` pointers), and correctly + // use either `$crate::PinInit::__pinned_init` or `$crate::Init::__init`. unsafe impl<$($impl_generics)*> $crate::__internal::PinData for __ThePinData<$($ty_generics)*> where $($whr)* @@ -1000,23 +1009,34 @@ macro_rules! __pin_data { { $( $(#[$($p_attr)*])* + /// # Safety + /// + /// The caller must ensure that `slot` is a valid pointer to uninitialized memory + /// that is properly aligned and large enough to hold a value of type `$p_type`. + /// `slot` is a pointer valid for writes and any value written to it is pinned. $pvis unsafe fn $p_field( self, slot: *mut $p_type, init: impl $crate::PinInit<$p_type, E>, ) -> ::core::result::Result<(), E> { - // SAFETY: TODO. + // SAFETY: `slot` points to valid, uninitialized memory for a `$p_type`. unsafe { $crate::PinInit::__pinned_init(init, slot) } } )* $( $(#[$($attr)*])* + /// # Safety + /// + /// The caller must ensure that `slot` is a valid pointer to uninitialized memory + /// that is properly aligned and large enough to hold a value of type `$type`. + /// The memory at `slot` must also be valid for writes. + /// `slot` is valid for writes. $fvis unsafe fn $field( self, slot: *mut $type, init: impl $crate::Init<$type, E>, ) -> ::core::result::Result<(), E> { - // SAFETY: TODO. + // SAFETY: `slot` points to valid, uninitialized memory for a `$type`. unsafe { $crate::Init::__init(init, slot) } } )* @@ -1132,7 +1152,11 @@ macro_rules! __init_internal { struct __InitOk; // Get the data about fields from the supplied type. // - // SAFETY: TODO. + // SAFETY: The `$get_data()` call (which resolves to either `HasPinData::__pin_data()` + // or `HasInitData::__init_data()`) is safe because this macro is part of the pin-init + // crate, satisfying the safety requirement that these methods should only be called by + // macros in the pin-init crate (as documented in their respective `# Safety` sections). + // The `paste!` macro is used for re-tokenization and does not affect this safety argument. let data = unsafe { use $crate::__internal::$has_data; // Here we abuse `paste!` to retokenize `$t`. Declarative macros have some internal @@ -1188,7 +1212,12 @@ macro_rules! __init_internal { let init = move |slot| -> ::core::result::Result<(), $err> { init(slot).map(|__InitOk| ()) }; - // SAFETY: TODO. + // SAFETY: The `init` closure, generated by this macro, upholds the contract of + // `$crate::$construct_closure`. It ensures `slot` is fully initialized on `Ok(())` + // or properly cleaned (via `DropGuard`s) on `Err(err)`, leaving `slot` safe to + // deallocate. Pinning invariants are maintained using `PinData`/`InitData` methods + // for field initialization, and `slot` is not moved (for `!Unpin` types) as it's + // accessed via a pointer. let init = unsafe { $crate::$construct_closure::<_, $err>(init) }; init }}; @@ -1338,7 +1367,8 @@ macro_rules! __init_internal { // Since we are in the closure that is never called, this will never get executed. // We abuse `slot` to get the correct type inference here: // - // SAFETY: TODO. + // SAFETY: This is unreachable code that is used solely for compile-time type checking, + // it is never executed. unsafe { // Here we abuse `paste!` to retokenize `$t`. Declarative macros have some internal // information that is associated to already parsed fragments, so a path fragment diff --git a/tests/ui/compile-fail/init/invalid_init.stderr b/tests/ui/compile-fail/init/invalid_init.stderr index b80864de..7731bc31 100644 --- a/tests/ui/compile-fail/init/invalid_init.stderr +++ b/tests/ui/compile-fail/init/invalid_init.stderr @@ -10,7 +10,5 @@ error[E0277]: the trait bound `impl pin_init::PinInit: Init` is not sa | |______the trait `Init` is not implemented for `impl pin_init::PinInit` | required by a bound introduced by this call | - = help: the following other types implement trait `Init`: - ChainInit - Result + = help: the trait `Init` is implemented for `ChainInit` = note: this error originates in the macro `$crate::__init_internal` which comes from the expansion of the macro `init` (in Nightly builds, run with -Z macro-backtrace for more info) diff --git a/tests/ui/expand/many_generics.expanded.rs b/tests/ui/expand/many_generics.expanded.rs index f22ef125..d85136e4 100644 --- a/tests/ui/expand/many_generics.expanded.rs +++ b/tests/ui/expand/many_generics.expanded.rs @@ -46,6 +46,11 @@ const _: () = { where T: Bar<'a, 1>, { + /// # Safety + /// + /// The caller must ensure that `slot` is a valid pointer to uninitialized memory + /// that is properly aligned and large enough to hold a value of type `$p_type`. + /// `slot` is a pointer valid for writes and any value written to it is pinned. unsafe fn _pin( self, slot: *mut PhantomPinned, @@ -53,6 +58,12 @@ const _: () = { ) -> ::core::result::Result<(), E> { unsafe { ::pin_init::PinInit::__pinned_init(init, slot) } } + /// # Safety + /// + /// The caller must ensure that `slot` is a valid pointer to uninitialized memory + /// that is properly aligned and large enough to hold a value of type `$type`. + /// The memory at `slot` must also be valid for writes. + /// `slot` is valid for writes. unsafe fn array( self, slot: *mut [u8; 1024 * 1024], @@ -60,6 +71,12 @@ const _: () = { ) -> ::core::result::Result<(), E> { unsafe { ::pin_init::Init::__init(init, slot) } } + /// # Safety + /// + /// The caller must ensure that `slot` is a valid pointer to uninitialized memory + /// that is properly aligned and large enough to hold a value of type `$type`. + /// The memory at `slot` must also be valid for writes. + /// `slot` is valid for writes. unsafe fn r( self, slot: *mut &'b mut [&'a mut T; SIZE], diff --git a/tests/ui/expand/pin-data.expanded.rs b/tests/ui/expand/pin-data.expanded.rs index 58b8f532..a409fd9b 100644 --- a/tests/ui/expand/pin-data.expanded.rs +++ b/tests/ui/expand/pin-data.expanded.rs @@ -17,6 +17,11 @@ const _: () = { #[allow(dead_code)] #[expect(clippy::missing_safety_doc)] impl __ThePinData { + /// # Safety + /// + /// The caller must ensure that `slot` is a valid pointer to uninitialized memory + /// that is properly aligned and large enough to hold a value of type `$p_type`. + /// `slot` is a pointer valid for writes and any value written to it is pinned. unsafe fn _pin( self, slot: *mut PhantomPinned, @@ -24,6 +29,12 @@ const _: () = { ) -> ::core::result::Result<(), E> { unsafe { ::pin_init::PinInit::__pinned_init(init, slot) } } + /// # Safety + /// + /// The caller must ensure that `slot` is a valid pointer to uninitialized memory + /// that is properly aligned and large enough to hold a value of type `$type`. + /// The memory at `slot` must also be valid for writes. + /// `slot` is valid for writes. unsafe fn array( self, slot: *mut [u8; 1024 * 1024], diff --git a/tests/ui/expand/pinned_drop.expanded.rs b/tests/ui/expand/pinned_drop.expanded.rs index 107a885b..241d3af8 100644 --- a/tests/ui/expand/pinned_drop.expanded.rs +++ b/tests/ui/expand/pinned_drop.expanded.rs @@ -17,6 +17,11 @@ const _: () = { #[allow(dead_code)] #[expect(clippy::missing_safety_doc)] impl __ThePinData { + /// # Safety + /// + /// The caller must ensure that `slot` is a valid pointer to uninitialized memory + /// that is properly aligned and large enough to hold a value of type `$p_type`. + /// `slot` is a pointer valid for writes and any value written to it is pinned. unsafe fn _pin( self, slot: *mut PhantomPinned, @@ -24,6 +29,12 @@ const _: () = { ) -> ::core::result::Result<(), E> { unsafe { ::pin_init::PinInit::__pinned_init(init, slot) } } + /// # Safety + /// + /// The caller must ensure that `slot` is a valid pointer to uninitialized memory + /// that is properly aligned and large enough to hold a value of type `$type`. + /// The memory at `slot` must also be valid for writes. + /// `slot` is valid for writes. unsafe fn array( self, slot: *mut [u8; 1024 * 1024],