rust: Add Follow with a different lifetime

[bsilver8192]

This adds a FollowWith trait, which allows accessing a table type with a different lifetime in generic code. This is necessary for writing generic containers that own flatbuffers.

I'm using this (with a flatbuffers fork that already includes this change) at
https://github.com/frc971/971-Robot-Code/blob/f4cae52cf7d990572e157ced324aee43cf89c523/aos/flatbuffers.rs. I would be happy to move this to flatbuffers itself if there's interested, but I think generating the small amount of code in this commit to allow writing wrappers like that is independently valuable.

I think this is what was discussed in #5749, for reference.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[CasperN]

I would be happy to move this to flatbuffers itself if there's interested, but I think generating the small amount of code in this commit to allow writing wrappers like that is independently valuable.

I think we should discuss merging your fork instead of just merging this part.

---

impl<'a, 'b> flatbuffers::FollowWith<'a> for Monster<'b> {
  type Inner = Monster<'a>;
}

Does this need a constraint 'a: 'b? Otherwise you'd be able to follow your way into a reference that could get invalidated.

Also can you give a minimal example of what is not possible with Follow but is possible with FollowWith? I don't have context on your fork or its design

[bsilver8192]

I would be happy to move this to flatbuffers itself if there's interested, but I think generating the small amount of code in this commit to allow writing wrappers like that is independently valuable.

I think we should discuss merging your fork instead of just merging this part.

Sure. I'm working on merging pieces that make sense. We've got a few hacks that need to be redone more generally first, but I'm all for merging the rest of it.

Also, to be clear, the wrapper I wrote isn't actually part of the fork, it can be independently implemented by anybody with just the changes in this PR. Do you want to the wrapper also?

impl<'a, 'b> flatbuffers::FollowWith<'a> for Monster<'b> {
  type Inner = Monster<'a>;
}

Does this need a constraint 'a: 'b? Otherwise you'd be able to follow your way into a reference that could get invalidated.

Once you have the type Monster<'b>, all the normal lifetime rules apply to creating an instance of it, which prevent creating invalid instances (except with unsafe).

I think adding that bound makes this API strictly less flexible, for no benefit. However, my current use case always has 'a as 'static because it's not used anywhere, which means this bound wouldn't be a problem. What do you think?

Also can you give a minimal example of what is not possible with Follow but is possible with FollowWith? I don't have context on your fork or its design

It lets you implement something like this:

// The idea is for `'self` to be the lifetime of the struct, to create
// an interior pointer, but that's not valid Rust.
struct X<T: Follow<'self>> {
    t: T,
    storage: Vec<u8>,
}

but without the interior pointer that Rust doesn't support. Instead, you can write it like this:

struct X<T>
where
    for<'a> T: FollowWith<'a>,
    for<'a> <T as FollowWith<'a>>::Inner: Follow<'a>,
{
    storage: Vec<u8>,
    _marker: PhantomData<T>,
}

impl<T> X<T>
where
    for<'a> T: FollowWith<'a>,
    for<'a> <T as FollowWith<'a>>::Inner: Follow<'a>,
{
    pub unsafe fn t<'this>(&'this self) -> <<T as FollowWith<'this>>::Inner as Follow<'this>>::Inner {
        flatbuffers::root_unchecked::<<T as FollowWith<'this>>::Inner>(self.storage.as_ref())
    }
}

Another point of comparison: I think this is the same idea expressed by yoke. Yokeable is the analog to FollowWith. However, flatbuffers table type values are effectively fat references (pointer+offset), vs yoke works with the references directly, so I don't think yoke is usable with flatbuffers tables.

[CasperN]

Thanks the comparison with Yoke was helpful. I'm starting to see the point:

1. We want to generalize the storage to be from more than just &'a[u8], e.g. Rc<[u8]>
2. We want to attach the storage to a particular flatbuffer so you can pass it around, e.g. verify a Vec<u8> contains a monster, move it, and then use it.

I think adding that bound makes this API strictly less flexible, for no benefit. However, my current use case always has 'a as 'static because it's not used anywhere, which means this bound wouldn't be a problem. What do you think?

I think I didn't understand enough of the motivation originally and thought that FollowWith could somehow be used to "launder" a longer lifetime out of a reference. It seems the core motivation is the following trait method, and "laundering" is required for lack of a 'self lifetime:

pub trait Flatbuffer<T>
where
    for<'a> T: FollowWith<'a>,
    for<'a> <T as FollowWith<'a>>::Inner: Follow<'a>,
{
    fn message<'this>(&'this self) -> <<T as FollowWith<'this>>::Inner as Follow<'this>>::Inner;
}

Ok, let me play around with the lifetimes a bit, I'm not sure if this is the best/only way to get what you want

[bsilver8192]

I think I didn't understand enough of the motivation originally and thought that FollowWith could somehow be used to "launder" a longer lifetime out of a reference. It seems the core motivation is the following trait method, and "laundering" is required for lack of a 'self lifetime:

pub trait Flatbuffer<T>
where
    for<'a> T: FollowWith<'a>,
    for<'a> <T as FollowWith<'a>>::Inner: Follow<'a>,
{
    fn message<'this>(&'this self) -> <<T as FollowWith<'this>>::Inner as Follow<'this>>::Inner;
}

Ok, let me play around with the lifetimes a bit, I'm not sure if this is the best/only way to get what you want

Yes, that is the main motivation. Thanks for taking a look, I'd love to see a better solution.

For reference, I think this can also be done with the recently-stabilized part of generic associated types by just adding a generic associated type to Follow. I wrote this before there was a clear timeline to stabilize that, doing it today I'd look very carefully at that. Not sure about your thoughts on increasing the required Rust version or using a proc-macro to make that type conditional (like rustversion).

[CasperN]

So I can get the following to work, the main drawback is that you have to name your storage type,
e.g. FlatbufferWithStorage<MyBuffer, Arc<[u8]>>::new(my_arc).
Rust can't figure out S = Arc<[u8]> even if my_arc: Arc<[u8]>.
Is this sufficiently generic?

struct FlatbufferWithStorage<'a, T: flatbuffers::Follow<'a>, S: AsRef<[u8]>> {
    data: S,
    phantom: std::marker::PhantomData<&'a T>,
}

impl<'a, T: flatbuffers::Follow<'a>, S: AsRef<[u8]>> FlatbufferWithStorage<'a, T, S> {
    fn new(data: S) -> Self {
        // TODO: verify here
        Self { data, phantom: std::marker::PhantomData::default() }
    }
    fn get<'b: 'a>(&'b self) -> T::Inner {
        unsafe { flatbuffers::root_unchecked::<T>(self.data.as_ref()) }
    }
}

[bsilver8192]

So I can get the following to work, the main drawback is that you have to name your storage type, e.g. FlatbufferWithStorage<MyBuffer, Arc<[u8]>>::new(my_arc). Rust can't figure out S = Arc<[u8]> even if my_arc: Arc<[u8]>. Is this sufficiently generic?

struct FlatbufferWithStorage<'a, T: flatbuffers::Follow<'a>, S: AsRef<[u8]>> {
    data: S,
    phantom: std::marker::PhantomData<&'a T>,
}

impl<'a, T: flatbuffers::Follow<'a>, S: AsRef<[u8]>> FlatbufferWithStorage<'a, T, S> {
    fn new(data: S) -> Self {
        // TODO: verify here
        Self { data, phantom: std::marker::PhantomData::default() }
    }
    fn get<'b: 'a>(&'b self) -> T::Inner {
        unsafe { flatbuffers::root_unchecked::<T>(self.data.as_ref()) }
    }
}

I started with something similar, but I don't know how to make it work with a trait. I think it works without a trait due to Rust's subtyping rules via the PhantomData, but traits (via generics or trait objects) don't have those, so it's not usable. As a simple example, I can't make this work:

trait SomeFlatbuffer<'a, T: flatbuffers::Follow<'a>> {
   fn get<'b: 'a>(&'b self) -> T::Inner;
}
fn takes_some<'a, 'b: 'a>(f: impl SomeFlatbuffer<'a, my_game::example::Monster<'a>> + 'b) {
    f.get().hp();
}

[dbaileychess]

@CasperN @bsilver8192 Status of this now?

[CasperN]

I think we should think of a better design for reflection and perhaps generalize storage in the Table struct itself.

[bsilver8192]

I think we should think of a better design for reflection and perhaps generalize storage in the Table struct itself.

Yea, I can see that. I think you basically move the generic parameter in Follow (and some other traits) from the trait itself to each method of it.

[dbaileychess]

Can we close this PR then?

[bsilver8192]

The alternative touches most of the public Rust APIs. That would be a very breaking change. Is doing that without any support for a migration period an option, or would there need to be further design to support both at once?

[CasperN]

The alternative touches most of the public Rust APIs. That would be a very breaking change. Is doing that without any support for a migration period an option, or would there need to be further design to support both at once?

I agree that it will be a breaking change for our generated code and the flatbuffers crate, but that should be fine.
The generated code's API is our stable public api boundary, so long as we don't break users who only rely on the generated code's public APIs, we should be good. E.g., if someone named fn my_function<'b>(foo: FooTable<'b>) before, they shouldn't have to change their code. We might have to generate aliases type FooTable<'b> = FooTableWithStorage<'b, Storage=&'b[u8]> but maybe there are better solutions out there.

Also, we release our generated code and the flatbuffers crate together, so there isn't a need to think about multiple version support between those "units".