Re: [PATCH v2 1/3] rust: add UnsafePinned type

[Benno Lossin]

On Thu May 1, 2025 at 9:11 PM CEST, Christian Schrefl wrote:
> On 01.05.25 8:51 PM, Benno Lossin wrote:
>> On Wed Apr 30, 2025 at 7:30 PM CEST, Christian Schrefl wrote:
>>> On 30.04.25 11:45 AM, Benno Lossin wrote:
>>>> On Wed Apr 30, 2025 at 10:36 AM CEST, Christian Schrefl wrote:
>>>>> +/// This implementation works because of the "`!Unpin` hack" in rustc, which allows (some kinds of)
>>>>> +/// mutual aliasing of `!Unpin` types. This hack might be removed at some point, after which only
>>>>> +/// the `core::pin::UnsafePinned` type will allow this behavior. In order to simplify the migration
>>>>> +/// to future rust versions only this polyfill of this type should be used when this behavior is
>>>>> +/// required.
>>>>> +///
>>>>> +/// In order to disable niche optimizations this implementation uses [`UnsafeCell`] internally,
>>>>> +/// the upstream version however will not. So the fact that [`UnsafePinned`] contains an
>>>>> +/// [`UnsafeCell`] must not be relied on (Other than the niche blocking).
>>>>
>>>> I would make this last paragraph a normal comment, I don't think we
>>>> should expose it in the docs.
>>>
>>> I added this as docs since I wanted it to be a bit more visible,
>>> but I can replace the comment text (about `UnsafeCell`) with this paragraph
>>> and drop it from the docs if you want.
>> 
>> I think we shouldn't talk about these implementation details in the
>> docs.
>
> Alright, what do you think of:
>
> // As opposed to the upstream Rust type this contains a `PhantomPinned`` and `UnsafeCell<T>`

There are two '`' after PhantomPinned.

> // - `PhantomPinned` to avoid needing a `impl<T> !Unpin for UnsafePinned<T>`

s/ a / an /

I find the phrasing 'avoid needing <negative impl>' a bit weird, I'd
just say "`PhantomPinned` to ensure the struct always is `!Unpin` and
thus enables the `!Unpin` hack".

If you have a link to somewhere that explains that hack, then I'd also
put it there. I forgot if it's written down somewhere.

> //   Required to use the `!Unpin hack`.
> // - In order to disable niche optimizations this implementation uses `UnsafeCell` internally,
> //   the upstream version however currently does not. This will most likely change in the future
> //   but for now we don't expose this in the documentation, since adding the guarantee is simpler
> //   than removing it. Meaning that for now the fact that `UnsafePinned` contains an `UnsafeCell`
> //   must not be relied on (Other than the niche blocking).

---
Cheers,
Benno