Re: [PATCH v2 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 v2:
>  * Split patch into two (Miguel)
>  * Improve commit message and subject (Miguel)
>
>  rust/kernel/types.rs | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 8ed802444c594..08ff4949d2a61 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -274,14 +274,20 @@ fn drop(&mut self) {
>  /// [`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>`.
> -/// It gets rid of all the usual assumptions that Rust has for a value of type `T`:
> -///
> -/// * The value is allowed to be uninitialized (for example have invalid bit patterns: `3` for a
> -///   [`bool`]).
> +/// 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.

Thanks for clarifying the docs. What you write is correct, but I would
prefer a more terse and formal language, at least for the first
paragraph. Perhaps you can include the following in some form:

  > Opaque opts out of the following rust language invariants for the
  > contained `T`:
  > 
  > - Initialization invariant - the contained value is allowed to be
  >   uninitialized and contain invalid bit patterns.
  > - Immutability invariant - `Opaque` allows interior mutability.
  > - Uniqueness invariant - `Opaque` allows aliasing of shared references.
  > 
  > Further, `Opaque` is `!Unpin`.

And then after that paragraph, you could elaborate with examples and so forth?

Best regards,
Andreas Hindborg