Re: [PATCH v3 2/2] rust: types: `Opaque` doc: Add some destructor description

[Boqun Feng <boqun.feng@gmail.com>]

On Wed, Mar 05, 2025 at 06:34:38AM +0100, Dirk Behme wrote:
> In the discussion [1] some `Opaque` documentation updates have
> been proposed. In that discussion it was clarified that `Opaque`
> is intended to be used for (partial) uninitialized or changing C
> structs. And which consequences this has for using raw pointers
> or the destruction. Improve the `Opaque` documentation by adding
> these conclusions from that discussion.
> 
> Suggested-by: Daniel Almeida <daniel.almeida@collabora.com>
> Link: https://lore.kernel.org/rust-for-linux/F8AB1160-F8CF-412F-8B88-4C79D65B53A1@collabora.com/ [1]
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> 
> ---
> Changes in v3:
>    * Add the "terse" proposals.
>    * Move the non-linkage artifact from patch 1/2 to here.
> 
>  rust/kernel/types.rs | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index af30e9c0ebccb..f370cdb48a648 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -271,17 +271,33 @@ fn drop(&mut self) {
>  
>  /// Stores an opaque value.
>  ///
> -/// [`Opaque<T>`] is meant to be used with FFI objects that are never interpreted by Rust code.
> +/// [`Opaque<T>`] opts out of the following Rust language invariants for the contained `T`
> +/// by using [`UnsafeCell`]:
>  ///
> -/// It is used to wrap structs from the C side, like for example `Opaque<bindings::mutex>`.
> -/// It gets rid of all the usual assumptions that Rust has for a value:
> +/// * Initialization invariant - the contained value is allowed to be uninitialized and
> +///   contain invalid bit patterns.
> +/// * Immutability invariant - [`Opaque<T>`] allows interior mutability.
> +/// * Uniqueness invariant - [`Opaque<T>`] allows aliasing of shared references.
> +///
> +/// Further, [`Opaque<T>`] is `!Unpin` and will not run the drop method of the contained `T`
> +/// when dropped.

I would move "will not run the drop method of contained `T` when
dropped" to the "Initialization invariant" above because they are
logically related.

Regards,
Boqun

>  ///
> -/// * The value is allowed to be uninitialized (for example have invalid bit patterns: `3` for a
> -///   [`bool`]).
> +/// [`Opaque<T>`] is meant to be used with FFI objects that are never interpreted by Rust code.
> +/// It is used to wrap structs from the C side, like for example `Opaque<bindings::mutex>`.
> +/// This is useful for C structs that are not fully initialized (yet) or might change their
> +/// content from C side at runtime. [`Opaque<T>`] gets rid of all the usual assumptions that
> +/// Rust has for a value of type `T`:
> +///
> +/// * The value is allowed to be uninitialized or invalid (for example have invalid bit patterns:
> +///   `3` for a [`bool`]).
> +/// * By dereferencing a raw pointer to the value you are unsafely asserting that the value is
> +///   valid *right now*.
>  /// * The value is allowed to be mutated, when a `&Opaque<T>` exists on the Rust side.
>  /// * No uniqueness for mutable references: it is fine to have multiple `&mut Opaque<T>` point to
>  ///   the same value.
>  /// * The value is not allowed to be shared with other threads (i.e. it is `!Sync`).
> +/// * The destructor of [`Opaque<T>`] does *not* run the destructor of `T`, as `T` may
> +///   be uninitialized, as described above.
>  ///
>  /// This has to be used for all values that the C side has access to, because it can't be ensured
>  /// that the C side is adhering to the usual constraints that Rust needs.
> -- 
> 2.48.0
>