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

[Boqun Feng <boqun.feng@gmail.com>]

On Mon, Nov 27, 2023 at 08:37:12PM +0000, Benno Lossin wrote:
> 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

Right, I was missing that T is ForeignOwnable.

> 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 :)
> 

One thing is that it seems impossible to impl Into<&T> for ArcBorrow,
for example, if we have an Wrapper struct representting ArcBorrow:

	struct Wrapper<'a, T>(&'a T);

`From` side doesn't work:

    impl<'a, T> From<Wrapper<'a, T>> for &'a T {
        fn from(value: Wrapper<'a, T>) -> Self {
             value.0
        }
    }

IIUC, it's because of the orphan rules.

`Into` side doesn't work either:

    impl<'a, T> Into<&'a T> for Wrapper<'a, T> {
        fn into(self) -> &'a T {
            self.0
        }
    }

I'm still trying to understand the reason:

error[E0119]: conflicting implementations of trait `Into<&_>` for type `Wrapper<'_, _>`
 --> src/lib.rs:9:1
  |
9 | impl<'a, T> Into<&'a T> for Wrapper<'a, T> {
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: conflicting implementation in crate `core`:
          - impl<T, U> Into<U> for T
            where U: From<T>;
  = note: downstream crates may implement trait `std::convert::From<Wrapper<'_, _>>` for type `&_`

Playground link:

	https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=36a6a94740c00ba8ca47f773a423eb9e

Interesting enough, `Into<Option<&'a T>>` works:

    impl<'a, T> Into<Option<&'a T>> for Wrapper<'a, T> {
        fn into(self) -> Option<&'a T> {
            Some(self.0)
        }
    }

Of course, we can always implement our own Into-like trait, e.g.
IntoRef.

Regards,
Boqun

> -- 
> Cheers,
> Benno
> 
>