Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return references from map_ref ? #2

Open
to-mas-kral opened this issue Jul 2, 2023 · 6 comments
Open

Return references from map_ref ? #2

to-mas-kral opened this issue Jul 2, 2023 · 6 comments

Comments

@to-mas-kral
Copy link

Hi, thank you for making this crate.
I'd find it very useful to return refernces from map_ref, like this:

struct Bar {
    inner: u32,
}

#[derive(EnumPtr)]
#[repr(C, usize)]
enum Foo {
    A(Box<Bar>),
    B(Box<u64>),
}

let boxed = Foo::A(Box::new(Bar { inner: 1u32 }));
let compact_box: Compact<Foo> = boxed.into();

let reference = compact_box.map_ref(|foo| match foo {
    Foo::A(a) => &a.inner,
    Foo::B(_) => todo!(),
});

assert_eq!(*reference, 1u32);

I don't know if this is possible to achieve without UB, but it would be nice for sure.

@QuarticCat
Copy link
Owner

QuarticCat commented Jul 3, 2023

I'm afraid this is a bit hard. When you hold a Compact<Foo>, there is no Foo in the memory, but only its compact form. So to borrow a Foo out from it, we have to create a temporary Foo. The reference you want to return comes from that temporary object. That's why I implemented map_ref rather than as_ref, I need to make sure that there's somewhere to store it (on the stack) and mem::forget this object afterward (otherwise boxed objects will be dropped).

For this problem, I have some ideas:

  • Generate a proxy struct that represents a reference to Foo and return it. For example, enum FooRef { A(&Box<Bar>), B(&Box<u64>) } + fn as_ref(&self) -> FooRef.
  • Directly borrow it from the compact form. Probably take_ref!(compact_box, Foo::A). I haven't come up with a clear implementation.

What do you think?

I just found that both of them are not achievable since there's no Box<Bar> in the memory so &Box<Bar> is not valid. If you have any ideas, please let me know.

@to-mas-kral
Copy link
Author

Sadly, it seems to me that there's no feasible solution to this.
One thing I tried is creating references to inner and transmuting them to the lifetime of Compact.
Miri with Stacked Borrows complains about this. Interestingly, it doesn't seem to be UB with Tree Borrows for the test case I tried.
But there's no way of creating a safe interface for this even if it somehow wasn't UB.

@QuarticCat
Copy link
Owner

QuarticCat commented Jul 4, 2023

But there's no way of creating a safe interface for this even if it somehow wasn't UB.

Maybe we can implement something like RefCell's Ref as well as RefMut.

struct Ref<'a, T> {
    inner: ManuallyDrop<T>,
    marker: PhantomData<&'a T>,
}

impl<T: Deref> Deref for Ref<'_, T> {
    type Target = <T as Deref>::Target;

    fn deref(&self) -> &Self::Target {
        self.inner.deref()
    }
}

And then generate:

#[repr(C, usize)]
enum FooRef {
    A(Ref<Box<Bar>>),
    B(Ref<Box<u64>>),
}

Thinking for a while...

Personally I don't like generating structs in user code. That's somewhat invasive.

@QuarticCat
Copy link
Owner

QuarticCat commented Jul 5, 2023

I tried to implement the FooRef solution. It was much more complex than I thought and I decided to give up this route.

However, the take_ref! solution might work. Here's a small example that passes MIRI under both stacked borrows and tree borrows.

use enum_ptr::{Compact, EnumPtr};

// used to specify lifetime
unsafe fn take_ref_helper<'a, T, U>(
    compact: &'a Compact<T>,
    f: impl FnOnce(&T) -> Option<&'a U>,
) -> Option<&'a U>
where
    T: From<Compact<T>>,
    Compact<T>: From<T>,
{
    compact.map_ref(f)
}

macro_rules! take_ref {
    ($compact:expr, $($variant:tt)*) => {
        unsafe {
            take_ref_helper($compact, |extracted| match extracted {
                $($variant)*(inner) => Some(&*(::std::ops::Deref::deref(inner) as *const _)),
                _ => None,
            })
        }
    };
}

#[test]
fn main() {
    #[derive(Debug, EnumPtr)]
    #[repr(C, usize)]
    enum Foo<'a> {
        A(&'a i32),
        B(Box<i64>),
    }

    let compact_foo = Compact::from(Foo::A(&42));
    dbg!(take_ref!(&compact_foo, Foo::A));
    let compact_foo = Compact::from(Foo::B(Box::new(43)));
    dbg!(take_ref!(&compact_foo, Foo::B));
}

One drawback of this solution is that it cannot handle named variants. But I have already decided to remove named variant support since 0.2.0. So it's not a big problem.

Another drawback is that this API is troublesome when you deal with multiple branches at once. I'm still searching for a better solution.

I won't put this API into my crate any time soon. I need more time to think.

@QuarticCat
Copy link
Owner

QuarticCat commented Jul 7, 2023

I'm sorry to find that take_ref! has some issues as well. First of all, Deref doesn't actually offer all the guarantees we need. For example, ManuallyDrop<T> can deref to &T, which points to its own memory, although ManuallyDrop<T> cannot be used in EnumPtr. Secondly, it doesn't work for Option<Box<T>>, for which we usually want an Option<&T>. This problem is way harder than I thought.

I have come up with a solution that might work for all pointers excluding Option<T>, where FieldDeref is some sort of stricter Deref. You can find it here https://github.com/QuarticCat/enum-ptr/blob/13f4cc46e44e3a597a6b14f55ac2b88f0a300115/playground/tests/borrow.rs. But to support Option<T>, I need Rust to relax GAT constraints (see rust-lang/rust#87479) or maybe use HRTBs.

@QuarticCat
Copy link
Owner

Finally, I made it.

I released a beta version, check it out. https://crates.io/crates/enum-ptr/0.2.0-beta.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants