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

feat-request(rust): support for getting offset for tables #8194

Closed
cstrahan opened this issue Dec 23, 2023 · 4 comments
Closed

feat-request(rust): support for getting offset for tables #8194

cstrahan opened this issue Dec 23, 2023 · 4 comments
Labels

Comments

@cstrahan
Copy link

cstrahan commented Dec 23, 2023

We have a need for indexing into an unfinished message, something like so:

let mut map = HashMap::new();
for i in 0..children.len() {
    let child = children.get(i);
    // on the following line, get_offset!(child) gives us a WIPOffset<ViewProto<'_>>
    map.insert(Self::unique_id2(child), get_offset!(child));
}
// ... `map` is then used to create and push more values to `fbb`

where ViewProto is a table:

table ViewProto {
  // redacted
}

Our code currently does some unsound unsafe stuff allowing for our get_offset macro to construct the WIPOffset<ViewProto<'_>>. I'm currently in the process of trying to resolve our unsound code (and, more generally, remove as much use of unsafe as possible).

What I would like is something like this in the generated code:

pub struct ViewProto<'a> {
  pub _tab: flatbuffers::Table<'a>,
}

impl<'a> ViewProto<'a> {
    /// Return the offset into the FlatBufferBuilder
    pub fn offset<'bldr>(&self, fbb: &flatbuffers::FlatBufferBuilder<'bldr>) -> flatbuffers::WIPOffset<ViewProto<'bldr>> {
        // TODO: maybe have a debug assert here that sanity checks that
        // self._tab.buf is the same as fbb.unfinished_data().
        flatbuffers::WIPOffset::new((self._tab.buf.len() - self._tab.loc) as flatbuffers::UOffsetT)
    }
}

The challenge I'm trying to overcome here is that the 'a lifetime in something like ViewProto<'a> represents the lifetime of the underlying buffer, while 'bldr in FlatBufferBuilder<'bldr> is (as I understand it) is there to avoid using offsets from one FBB with another FBB. But, going from WIPOffset<ViewProto<'bldr> to ViewProto<'a> (where 'a is the lifetime of our self.fbb) loses the original 'bldr lifetime. So my idea was to have something that would be both convenient and more type safe than inlining the flatbuffers::WIPOffset::new((self._tab.buf.len() - self._tab.loc) as flatbuffers::UOffsetT) everywhere -- at least the offset function ensures that given a T we end up with a WIPOffset<T>, whereas the aforementioned expression will happily give a WIPOffset<U> (for some other unintended type U).

With the above in place, the code at the start of this issue could look like:

let mut map = HashMap::new();
for i in 0..children.len() {
    let child = children.get(i);
    map.insert(Self::unique_id2(child), child.offset(&self.fbb));
}
// ... `map` is then used to create and push more values to `fbb`

Would something along these lines be accepted?

@cstrahan
Copy link
Author

FWIW, I believe the proposal here would suffice for me to implement a generic offset! macro for my purposes (using FollowWith to adapt the SomeTableT<'a> to WIPOffset<SomeTableT<'fbb>>): #7540

@cstrahan
Copy link
Author

cstrahan commented Jan 2, 2024

FWIW, here's what I'm currently using, inspired a bit by #7540:

/// This trait serves as a type level function:
///
///     ∀ 'a. ∀ 'b. ∀ T<'a>: FollowWith<'b>. T<'a> -> T<'b>
///
/// In other words, this just allows us to substitute the lifetime param in T.
// Inspired by: https://github.com/google/flatbuffers/pull/7540
pub trait FollowWith<'b> {
    type Inner: Follow<'b>;
}

macro_rules! follow_with {
    ($x:ident $(,)?) => (
        impl<'a, 'b> FollowWith<'b> for $x<'a> {
            type Inner = $x<'b>;
        }
    );
    ($x:ident, $($y:ident),+ $(,)?) => (
        follow_with!($x);
        follow_with!($($y),+);
    );
}

follow_with!(ViewProto, ViewStateProto,);

/// Reads the (possibly unfinished) value at `offset`.
pub fn follow_unfinished<'a, 'fbb, 'buf, T>(
    fbb: &'a FlatBufferBuilder<'fbb>,
    offset: WIPOffset<T>,
) -> <<T as FollowWith<'buf>>::Inner as Follow<'buf>>::Inner
where
    'a: 'buf,
    T: FollowWith<'buf> + 'fbb,
{
    let buf = fbb.unfinished_data();
    let t = <T as FollowWith<'buf>>::Inner::follow(buf, buf.len() - offset.value() as usize);
    t
}

/// Returns the `WIPOffset<T<'fbb>>`` of the given table.
///
/// The `fbb: FlatBufferBuilder<'fbb>` argument is only used to provides the lifetime
/// for the resulting value.
// TODO(charles): it would be nice if, at least in debug builds,
// we would assert that the table._tab.buf and the fbb.unfinished_data() are one in the same.
// That would help ensure that we aren't accidentally creating offsets into the wrong fbb.
#[macro_export]
macro_rules! offset_of {
    ($fbb:expr, $table:expr) => {
        _typed_offset(
            &$fbb,
            &$table,
            ($table._tab.buf.len() - $table._tab.loc) as UOffsetT,
        )
    };
}
pub use offset_of;

/// Helper function used by the `offset_of` macro.
pub fn _typed_offset<'buf, 'fbb, TIn>(
    _fbb: &'buf FlatBufferBuilder<'fbb>,
    _t: &TIn,
    offset: UOffsetT,
) -> WIPOffset<<TIn as FollowWith<'fbb>>::Inner>
where
    TIn: FollowWith<'fbb>,
{
    WIPOffset::new(offset as UOffsetT)
}

which lets me do things like:

let fbb: FlatBufferBuilder<'fbb> = todo!();

// suppose we have an existing offset
let view_proto_offset: WIPOffset<ViewProto<'fbb>> = todo!();

// we can get the table from the unfinished buffer
let view_proto: ViewProto<'buf> = follow_unfinished(&fbb, view_proto_offset)

// or go back the other direction: get the offset from the given table
let view_proto_offset: WIPOffset<ViewProto<'fbb>> = offset_of!(fbb, view_proto);

At this point, I could probably replace the offset_of macro with a trait that provides a offset_in function, so the code would look more like the suggestion in my original comment:

let fbb: FlatBufferBuilder<'fbb> = todo!();
let view_proto: ViewProto<'buf> = todo!();
let view_proto_offset: WIPOffset<ViewProto<'fbb>> = view_proto.offset_in(&fbb);

I could tweak the FollowWith trait like so:

/// This trait serves as a type level function:
///
///     ∀ 'a. ∀ 'b. ∀ T<'a>: FollowWith<'b>. T<'a> -> T<'b>
///
/// In other words, this just allows us to substitute the lifetime param in T.
// Inspired by: https://github.com/google/flatbuffers/pull/7540
pub trait FollowWith<'b> {
    type Inner: Follow<'b>;

    /// Returns the `WIPOffset<T<'b>>`` of the given table.
    ///
    /// The `fbb: FlatBufferBuilder<'b>` argument is only used to provides the lifetime
    /// for the resulting value.
    fn offset_in<'buf>(&self, fbb: &'buf FlatBufferBuilder<'b>) -> WIPOffset<Self::Inner>;
}

macro_rules! follow_with {
    ($x:ident $(,)?) => (
        impl<'a, 'b> FollowWith<'b> for $x<'a> {
            type Inner = $x<'b>;
            fn offset_in<'buf>(&self, _fbb: &'buf FlatBufferBuilder<'b>) -> WIPOffset<Self::Inner> {
                // TODO(charles): it would be nice if, at least in debug builds,
                // we would assert that the table._tab.buf and the fbb.unfinished_data() are one in the same.
                // That would help ensure that we aren't accidentally creating offsets into the wrong fbb.
                WIPOffset::new((self._tab.buf.len() - self._tab.loc) as u32)
            }
        }
    );
    ($x:ident, $($y:ident),+ $(,)?) => (
        follow_with!($x);
        follow_with!($($y),+);
    );
}

Copy link
Contributor

github-actions bot commented Jul 2, 2024

This issue is stale because it has been open 6 months with no activity. Please comment or label not-stale, or this will be closed in 14 days.

@github-actions github-actions bot added the stale label Jul 2, 2024
Copy link
Contributor

This issue was automatically closed due to no activity for 6 months plus the 14 day notice period.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant