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:32:11PM +0100, Andreas Hindborg wrote:
> "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?
> 

I think my biggest problem is the word "allows", the phrase "allows
aliasing" sounds like Opaque<T> provide something *optional*, however,
as I understand it, Opaque<T> here just disallows certain optimization
from compiler, in other words, programmers want to use `Opaque<T>` to
forbid something, hence "allows" may not be the good word to use here?

How about:

  * Uniqueness invariant - [`Opaque<T>`] disallow alias assumptions
    made by compiler on an *exclusive* references.

Thoughts?

Regards,
Boqun

> 
> Best regards,
> Andreas Hindborg
> 
>