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

[Andreas Hindborg <a.hindborg@kernel.org>]

"Dirk Behme" <dirk.behme@de.bosch.com> writes:

> 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.

This last one is wrong (I know it's probably my fault, sorry). It should be:

  /// * Uniqueness invariant - [`Opaque<T>`] allows aliasing of **exclusive** references.

> +///
> +/// Further, [`Opaque<T>`] is `!Unpin` and will not run the drop method of the contained `T`
> +/// when dropped.

Could you link `Unpin` here?

>  ///
> -/// * 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`).

Link `Sync` here. You might need a [`!Sync`]: `Sync` at the bottom for
it to work.

> +/// * The destructor of [`Opaque<T>`] does *not* run the destructor of `T`, as `T` may
> +///   be uninitialized, as described above.


Best regards,
Andreas Hindborg