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

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

"Daniel Almeida" <daniel.almeida@collabora.com> writes:

> Hi Andreas,
>
>> On 19 Feb 2025, at 05:06, Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> "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
>
> IMHO, your “terseness” suggestion goes against the very intent of this patch.
>
> It takes something extremely clear, and places it behind some technical terms.
>
> In as much as I agree with making things formal and professional, our first priority should
> be making sure that the docs for this type are clear as day, so that people do not misuse it
> to introduce bugs.
>
> Note that this discussion sprung up because it’s hard to understand what exactly this type
> does and when exactly it should be used. The code is conceptually simple, but things like

Just to clarify, I do not suggest to yank the addition that this patch
provides. I think it is a valuable addition.

We need to strike a balance, which is why I suggested having the short,
to the point, more formal wording first - and then elaborate with the
more wordy, talkative, explanatory paragraph.

People who are very familiar with the rust language and/or relevant
invariants will immediately have the info they need after the first
"terse" paragraph.

People who need a bit more guidance can get that by reading further into
the docs.

>
>
>>> +/// * The destructor of [`Opaque<T>`] does *not* run the destructor of `T`, as `T` may
>>> +///   be uninitialized, as described above.
>
> are 100% a pitfall that should be warned against in bold letters and plain terms.
>

True. I would add it to the last line of my suggestion:

  Further, `Opaque` is `!Unpin` and will not run the drop method of the
  contained `T` when dropped.



Best regards,
Andreas Hindborg