Re: [PATCH v4] rust: xarray: Add an abstraction for XArray

[Benno Lossin <benno.lossin@proton.me>]

On 27.11.23 18:40, Boqun Feng wrote:
> On Mon, Nov 27, 2023 at 02:51:33PM +0100, Alice Ryhl wrote:
>> On Sun, Nov 26, 2023 at 2:13 PM Maíra Canal <mcanal@igalia.com> wrote:
>>> +// Convenience impl for `ForeignOwnable` types whose `Borrowed`
>>> +// form implements Deref.
>>> +impl<'a, T: ForeignOwnable> Deref for Guard<'a, T>
>>> +where
>>> +    T::Borrowed<'static>: Deref,
>>> +{
>>> +    type Target = <T::Borrowed<'static> as Deref>::Target;
>>> +
>>> +    fn deref(&self) -> &Self::Target {
>>> +        // SAFETY: See the `borrow()` method. The dereferenced `T::Borrowed` value
>>> +        // must share the same lifetime, so we can return a reference to it.
>>> +        unsafe { &*(T::borrow(self.0 as _).deref() as *const _) }
>>> +    }
>>> +}
>>
>> I don't think this is sound. Deref could return a reference into the
>> `T::Borrowed` itself, but it doesn't outlive this function. I would
>> either omit this impl, or provide a sub-trait for ForeignOwnable that
> 
> Agreed. FWIW, there was some discussion around this:
> 
> 	https://github.com/Rust-for-Linux/linux/pull/952#discussion_r1096791211
> 
> now think about it, how about the following?
> 
> 	impl<'a, T: ForeignOwnable> Deref for Guard<'a, T>
> 	where
> 	    for<'b> T::Borrowed<'b>: Into<&'b T>,
> 
> 	{
> 	    type Target = T;
> 
> 	    fn deref(&self) -> &Self::Target {
> 		// SAFETY: See the `borrow()` method.
> 		unsafe { T::borrow(self.0 as _) }.into()
> 	    }
> 	}

This one seems wrong, since `Borrowed` will be `&U` when `T = Box<U>`
and `&U: Into<&Box<U>>` is not satisfied. And if `T = Arc<U>`, then
`Borrowed = ArcBorrow`. If you want to take this, then you can try to
make Gary's suggestion work (from the Github discussion that Boqun posted):

     impl<'a, T: ForeignOwnable> Deref for Guard<'a, T>
     where
         T::Borrowed<'a>: Deref,
         for<'b> T::Borrowed<'b>: Into<&'b <T::Borrowed<'a> as Deref>::Target>,
     {
         type Target = <T::Borrowed<'a> as Deref>::Target;
     
         fn deref(&self) -> &Self::Target {
             self.borrow().into()
         }
     }

Also this is nicer, since there is no `unsafe` code :)

-- 
Cheers,
Benno