flatbuffers icon indicating copy to clipboard operation
flatbuffers copied to clipboard

[Rust] Not importing all files

Open krojew opened this issue 3 years ago • 7 comments

When building multiple files from a given directory (let's call it a) which import files from another directory b, there is a mod.rs file produced which only imports mods from the last file in a and last file in b, yet all _generated.rs files are produced.

krojew avatar Aug 19 '21 05:08 krojew

Can you give more details to help reproduce this? Were there multiple invocations of flatc --rust? There should only be one invocation so flatc will know about all the _generated.rs and generate mod.rs correctly.

CasperN avatar Aug 19 '21 16:08 CasperN

In the example flatc was run once in a with *.fbs.

krojew avatar Aug 19 '21 16:08 krojew

Ok, I was able to reproduce this. My current implementation assumes that Parser contains the full type system by the end of the invocation of flatc. This is wrong because the Parser is reset between different .fbs files.

Workaround: Write a new .fbs file that imports all your other .fbs files and invoke flatc with that.

I'll have to think about how to get around this reset behavior without breaking something else. We, unfortunately, rely on some pretty non-trivial mutable state in Parser.

CasperN avatar Aug 23 '21 21:08 CasperN

Any news on this front?

krojew avatar Nov 08 '21 11:11 krojew

Unfortunately, I am not working on this and don't really have time to for the foreseeable future. :(

CasperN avatar Nov 08 '21 15:11 CasperN

I believe I just ran into this. I've tried so many different incantations of flatc that its not even funny. This one got me the closest, with all imports between the various files correct, and its kinda nice that each type seems to be put in its own file (even though thats not how the fbs files were written...). However, the toplevel mod.rs only generates a pub use statement for the last type in the last file passed to flatc.

flatc --rust --rust-module-root-file --gen-all -o protocol/rust/src/generated -I ./protocol/flatbuffers ./protocol/flatbuffers/server.fbs ./protocol/flatbuffers/firmware.fbs ./protocol/flatbuffers/commons/datatypes.fbs ./protocol/flatbuffers/commons/hardware_info.fbs ./protocol/flatbuffers/commons/misc.fbs

The repository/commit in question is here: https://github.com/SlimeVR/slimevr_protocol/tree/6c96aabcf17c095ac648583f674a45681533190a

I will try the approach of having a .fbs file that imports all the others. Although I'd really appreciate it if this sort of stuff could be solved! It seems firmly in the expected use of flatbuffers.

TheButlah avatar Apr 02 '22 08:04 TheButlah

Yeah, sorry. There's a lot of legacy cruft implementation in there and the original variations made problematic assumptions. I need to deprecate the foot guns

and its kinda nice that each type seems to be put in its own file (even though thats not how the fbs files were written...). Although I'd really appreciate it if this sort of stuff could be solved! It seems firmly in the expected use of flatbuffers.

This is required to correctly map flatbuffer's c++ style namespaces onto Rust modules... it's a long story. The old thing was broken too - it basically didn't support multiple .fbs files.


My current implementation assumes that Parser contains the full type system by the end of the invocation of flatc. This is wrong because the Parser is reset between different .fbs files.

~Now that I'm revisiting this, I think I can just delete this line~

https://github.com/google/flatbuffers/blob/35281dedb5583d8abf27021d10ddeb15139aabc0/src/idl_gen_rust.cpp#L367

~which will ignore the fact that Parser thinks the type is generated. This will remove the need for the magic "import all the rest" file.~

nvm that's wrong

CasperN avatar Apr 02 '22 15:04 CasperN

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[bot] avatar Mar 04 '23 01:03 github-actions[bot]

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

github-actions[bot] avatar Mar 18 '23 20:03 github-actions[bot]

Is this going to be addressed? My team find themselves in the position to manually write the mod.rs file.

vtstanescu avatar Jun 07 '23 10:06 vtstanescu

Does the "Only generate one .fbs that includes all your other .fbs files" solution not work for your team?

Flatbuffers is a volunteer-only project, I don't have the time in my life to provide maintenance anymore, and I don't know who else may be interested, so unfortunately, I recommend contributing a fix, or at least to documentation for workarounds.

Though there are a lot of considerations that influence what the right solution is, and I'm not sure how it all shakes out

  • The core of the problem is that in Flatbuffers, like C/C++, its possible to add types to namespaces from new files. Rust's module system does not like that - you cannot add to a module from outside of it.
    • Initially, Rust flatbuffers didn't properly support namespaces/modules.
    • Some history in #5589 and #5275.
  • I tried to fix this by first waiting for all by generating code in a file tree[^1], like Rust prefers. #6731
    • This solution generated a mod.rs file that imported everything.
    • The issue is that when you run flatc with multiple schema files, it will overwrite the mod.rs files, and if your last .fbs file doesn't include everything, then not every type will be in there.
    • I thought I fixed it by generating the root file after all the .fbs files were processed, but I guess I didn't leave tests that and it was undone by #7806
    • This is why the workaround is to include all your .fbs files in a single file and only run flatc with that -- it'll generate all the symbols and there won't be any overwriting.

A potential solution could be to generate a module namespace hierarchy file for each .fbs file, with appropriate imports/reexports. This will prevent library users from stomping on the mod.rs file but requires us to know which .fbs file every symbol comes from and what symbols are visible in each .fbs file including all their imports. This would be crazy complex to do with the existing code.

// file1.fbs
include "file2.fbs";
namespace a;
table Foo { bar: b.Bar; }
// my_dir/file2.fbs
namespace a.b;
table Bar { }

will generate

// file1.rs

pub mod a {
  generate_foo_here!();
  // Note that using `my_dir::file2::*` at root of this file will define `a` twice.
  // Therefore, we have to be specific with what we `use`.
  pub mod b {
    pub use my_dir::file2::a::b::Bar;
  }
}
// my_dir/file2.rs
pub mod a {
  pub mod b {
    generate_bar_here!();
  }
}

A usability downside of this approach is that all the symbols are locked behind the file1 and file2 modules rather than being global. file1::a::b::Bar and my_dir::file2::a::b::Bar are the same, but you can't do the following

use file1::*;
use my_dir::file2::*;
fn make_bar() -> a::b::Bar<'static> {
  todo!()
}

Rust will error and say a is ambiguous.

Also, what if you don't want to generate your schema files in the same crate? Then the import/reexport declarations need to be adjusted accordingly - should that be allowed, and if so, how would it be configured?

There are likely more complications to this approach that aren't thought about. I don't know what other solutions people are using either, or their implicit requirements...

[^1]: Another complication: Bazel needs to know, from the .fbs filenames, what output files will be generated, which means generating a file per flatbuffer symbol will make Bazel sad. I think there are workarounds within Bazel itself, including tarballs and "tree artifacts", but I haven't verified those solutions. Generating one .rs per .fbs file is compatible with Bazel, which is nice.

CasperN avatar Jun 07 '23 15:06 CasperN

Hi, @CasperN!

Sorry, I just now realize my reply might denote some degree of attitude.

Thank you for the through explanation, this helps us in understanding the issue at hand and also the possible workaround, as I missed your previous comment with the workaround.

Initially I got confused by the flat compiler, thinking if it goes the way of generating a mod.rs, why doesn't it do it properly and kind of expected to either not generate it and leave that to the user, or do it properly.

Understanding the issue in-depth now, we can take an action and know why we need to do it that way. We are going to either use suggested workaround of building one .fbs that includes all the others or handle the mod.rs ourselves however we see fit for our use case.

vtstanescu avatar Jun 09 '23 07:06 vtstanescu