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

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

"Boqun Feng" <boqun.feng@gmail.com> writes:

> On Wed, Mar 05, 2025 at 08:47:42AM +0100, Andreas Hindborg 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 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.
>>
>
> Hmm... are we trying to say "`&mut Opaque<T>` still cannot mean
> noalias" here? If so I feel the wording might be confusing. I would
> suggest we don't use "uniqueness" here. Maybe something like:
>
> * Always aliased invariant - `&mut` [`Opaque<T>`] is not necessarily a
>   unique pointer, and thus the compiler cannot just make aliasing
>   assumptions.
>
> (I use the wording from UnsafePinned[1], maybe there could be better
> wording)
>
> [1]: https://rust-lang.github.io/rfcs/3467-unsafe-pinned.html#summary

I like my wording better. It says the same with fewer words, and we have
a more wordy section below anyway. We could combine:

 * Uniqueness invariant - [`Opaque<T>`] allows aliasing of **exclusive**
   references. That is, `&mut` [`Opaque<T>`] is not necessarily a unique
   pointer.


How is that?


Best regards,
Andreas Hindborg