Compilation fails using array of flatbuffer tables

reyk commented 4 years ago

With the following definition:

table MyTable {                                                                                                                                                                                             
    entries: [Entry];                                                                                                                                                                                       
}

I get different results with flatc and butte. The latter causes a compile-time error when I try to use it:

let entries = builder.create_vector(&[entry]);
let table = MyTable::create(&mut builder, &MyTableArgs { entries });

Error:

expected struct butte::primitives::WIPOffset, found struct butte::primitives::ForwardsUOffset

The generated code seems to be the problem (ForwardsUOffset vs. WIPOffset), flatc:

pub struct MyTableArgs<'a> {                                                                                                                                                                                
    pub entries: Option<flatbuffers::WIPOffset<flatbuffers::Vector<'a , flatbuffers::ForwardsUOffset<Entry<'a >>>>>,                                                                                        
}

butte:

pub struct MyTableArgs<'a> {                                                                                                                                                                                
    #[allow(clippy::type_complexity)]                                                                                                                                                                       
    pub entries: Option<butte::WIPOffset<butte::Vector<'a, butte::WIPOffset<Entry<&'a [u8]>>>>>,                                                                                                            
}

magnet commented 4 years ago

Thanks for the bug report. I starting looking into this, but it will likely require to clean-up the code gen for types quite a bit, and add more code-gen tests to avoid breaking other places. Worked on #42 to do so.

reyk commented 4 years ago

I have a potential fix in PR https://github.com/butte-rs/butte/pull/55

It includes a code-gen test under butte-examples because I think that the best way to test this is to use the full thing in an integration test-like way.

magnet commented 4 years ago

It includes a code-gen test under butte-examples because I think that the best way to test this is to use the full thing in an integration test-like way.

For reference, I had a very similar idea going forward with tests in butte. When I started the end-to-end tests in #42 I thought I would add the (pre) generated test cases in test/ rather than butte-examples. The test/ directory is designed for integration tests because these are unit tests that can refer to the current crate (e.g butte-build here, though we can also refer to butte-build's dependencies).

In that PR, I planned to move butte-build/src/codegen/tests/expected/test_table_simple.rs to butte-build/test/expected/test_table_simple.rs or something similar, and add a unit test that uses the generated code and does a few asserts, and unit tests of the generation function (in one place).

In effect, this code would be compiled there, there would be tests calling into it to make sure the values are expected, and butte_build would run tests that the generated schemas always match the code in test. This gets us full coverage that "generated Rust from schema" == "committed code" and "committed code compiles and can be used".

Compared to butte-examples this lets us have this as a part of cargo test -p butte-build and have a bazillion test cases that don't need to be in examples (which is the place users go to look for common things, not corner cases)

reyk commented 4 years ago

For reference, I had a very similar idea going forward with tests in butte.

I saw your work and it seems to be the right direction. I also agree that tests/ in the individual crates would be a better place.

Your PR is a draft and I wanted to submit a fix for the array issue with a related test case. My proposal is to keep this PR as it is, eventually merge it if the maintainers approve, and to replace my test with the proper test framework once it is ready.

butte-rs / butte

Compilation fails using array of flatbuffer tables #39