flatbuffers icon indicating copy to clipboard operation
flatbuffers copied to clipboard

rust: Introduce a trait for `get_fully_qualified_name`

Open bsilver8192 opened this issue 2 years ago • 8 comments

This allows retrieving the name of a generic flatbuffer table type, which can be handy.

bsilver8192 avatar Sep 22 '22 03:09 bsilver8192

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.

google-cla[bot] avatar Sep 22 '22 03:09 google-cla[bot]

Hello, can you help explain the design choice as to why this is a trait instead of using the existing method? Do you plan on using Box<dyn flatbuffers::FullyQualifiedName>? I can't see why that's useful

Also, the trait method should be implemented in terms of the existing method.

CasperN avatar Sep 22 '22 14:09 CasperN

Hello, can you help explain the design choice as to why this is a trait instead of using the existing method? Do you plan on using Box<dyn flatbuffers::FullyQualifiedName>? I can't see why that's useful

I use it for things like this:

    pub fn get_channel<T: FullyQualifiedName>(
        &self,
        name: &str,
    ) -> Result<&'event_loop Channel, ChannelLookupError> {
        self.get_raw_channel(name, T::get_fully_qualified_name())
    }

It's part of an IPC mechanism. It has a concept of "channels", which have a specific flatbuffer table type specified in a config file. Verifying the tables is redundant as long as both the sender and receiver are using the same type: the sender produces a valid table, and the receiver uses it. This API provides that safety by retrieving the name of the table generically, which needs a trait.

Also, the trait method should be implemented in terms of the existing method.

Done

bsilver8192 avatar Sep 22 '22 22:09 bsilver8192

Gotcha, so as it is right now, I'm not convinced this is useful enough to change the code generator for all our users. However, I see the value of a larger reflection stack that this could be a part of. WDYT?

CasperN avatar Sep 23 '22 13:09 CasperN

Gotcha, so as it is right now, I'm not convinced this is useful enough to change the code generator for all our users. However, I see the value of a larger reflection stack that this could be a part of. WDYT?

Up to you, I don't mind carrying small patches like this. I have a C++-based project that I'm adding Rust wrappers to, and so far no need to do reflection in Rust because all of the infrastructure that does complicated things like that is C++. Being able to link the generic Rust code with templated C++ code via assertions on the type names provides a convenient escape hatch for things like that.

If it makes a difference, this only affects generated code with --gen-name-strings enabled.

Another consideration: I do the same thing with C++ templates, which don't require any explicit unification of the concept across types. I think having a trait for Rust is necessary to have feature parity with the equivalent C++ APIs.

bsilver8192 avatar Sep 23 '22 20:09 bsilver8192

@CasperN @bsilver8192 Status on this one too?

dbaileychess avatar Oct 29 '22 00:10 dbaileychess

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

CasperN avatar Oct 31 '22 17:10 CasperN

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

I'm not sure that applies here? I think a trait for this makes sense regardless of how the rest of reflection works, because it needs to be a separate trait that isn't used when the flatc option is turned off.

bsilver8192 avatar Nov 03 '22 18:11 bsilver8192

Following up on this again, where is this at?

dbaileychess avatar Dec 15 '22 06:12 dbaileychess