[Rust]: enum as Option<T>

[vglavnyy]

This PR resolves #5467 and #5581 issues.
Current status: ready for review and testing.

This PR isn't finished yet due to an issue with the num-derive crate (rust-num/num-derive#35).
As soon as the issue is solved, the final commit will be prepared.
Thanks @jean-airoldie. He did most of the work.

[jean-airoldie]

Nice work.

Did you add a test with a None enum? I can't see it.

Also it think you should mark your PR with WIP so that its not accidentally merged.

[jean-airoldie]

@rw Now I'm thinking that the flatbuffers test suite is missing some backward & forward dependency tests that could have could detected this issue in the first place.

[rw]

@jean-airoldie Can you say more? cc @aardappel

[jean-airoldie]

Essentially the tests are compiled and ran at the same time, so the generated code is always up date with the binary format. Maybe it would be worthwhile to check whether new generated code works well against a old schema serialized file. In the case of rust, this would have detected the undefined behavior with the enum evolution.

[vglavnyy]

I added a test with enum evolution

{  name: "FromFuture",
  // Race=-127 is unknown Race, not defined in the monster_test.fbs schema.
  signed_enum: -127,
}

Assume that -127 is a value from a future version of Race enum.

[rw]

Is there a way to remove the dependency on num_traits? It looks like this PR doesn't use much from that library. You may have noticed that, in a comment elsewhere in the Rust FlatBuffers codebase, I mention that I didn't want to use num_traits originally because it does too much for my taste.

[jean-airoldie]

I see two potential ways to remove the num_traits dependency:

• We extract the FromPrimitive and ToPrimitive in a separate proc macro crate hosted on crates.io. It should be relatively simple to do.
• We try to implement the functionality directly in flatbuffers. @vglavnyy mentionned that he was thinking about doing this, but it think it would be much more complex.

[rw]

@jean-airoldie AFAICT the num_traits crate is just used by this PR to convert between different numeric types. Is it doing more that I don't see? Because we have access to the code generator, we can emit whatever code we want at compile time (within reason).

[jean-airoldie]

AFAICT the num_traits crate is just used by this PR to convert between different numeric types.

Yes. Its kinda trivial really.

#[repr(u8)]
struct Enum {
  A = 1,
  B = 2,
}

# We need to generate this
fn from_u8(int: u8) -> Option<Enum> {
  match int {
    x if x == A as u8 => Some(Enum::A),
    x if x == B as u8 => Some(Enum::B),
    _ => None,
  }
}

fn to_u8(e: Enum) -> u8 {
  e as u8
}

Because we have access to the code generator, we can emit whatever code we want at compile time (within reason).

Yes, however I'm not familiar with the code generator. But i know this is trivial to implement as a procedural rust macro.

[jcrevier]

Any update on this? I’m currently working around this at the moment. Still waiting on the issue with num derive? Or is the thought to use the flatbuffers code generator? I’d also like to see something similar for unions as per @jean-airoldie they exhibit similar undefined behavior. I can open a separate issue for that as I assume this fix is just for enums

[jean-airoldie]

@jcrevier From what I recall @rw would prefer not have the num_traits dependency since its kinda bloated for our needs. You probably would want to use the flatbuffers code generator instead. I'm not working on it currently since I still don't have any free time.

[vglavnyy]

I can open a separate issue for that as I assume this fix is just for enums

@jcrevier
This PR fixes both enums and unions (monster_test_generated.rs).
I can close this PR if you are already working around the issue. Or I can finalize it, add necessary code into the code generator to remove the dependency on num_derive.

[vglavnyy]

@jean-airoldie, @rw
Could you review Rust generated code and tests?
I removed dependencies on num_derive from generated code.

[jcrevier]

This PR fixes both enums and unions (monster_test_generated.rs).
I can close this PR if you are already working around the issue. Or I can finalize it, add necessary code into the code generator to remove the dependency on num_derive.

@vglavnyy Ah, yes, I can see the generated code works for unions as well. Having those From and TryFrom implementations available to users in addition to the Option is really nice too. Looking forward to seeing your PR merged.

[jean-airoldie]

Nice, the generated code looks good.

As an aside this makes me realize a flatbuffer enum that would previously be expressed as:

enum Color: uint8 {
    None,
    Red,
    Blue,
    Green,
}

Can now be simply be expressed as:

enum Color: uint8 {
    Red = 1,
    Blue,
    Green,
}

which removes the need for special logic previously required by the Color::None case.

[rw]

Thanks!

To help my review, can you please explain this PR?

1. The changes you made to the code generator,
2. the "from future" stuff,
3. the changes to the user-facing API, and
4. does this change the "default value" semantics of flatbuffers values?

Adding code comments along these lines would be very helpful, as well.

[vglavnyy]

Important
This PR adds breaking changes into generated Rust code.
User code will be broken due to changes in generated enum fields.
Without these changes, currently generated enums don't guarantee forward compatibility of a schema and could cause UB.

History

• Rust: Generated enums can cause undefined behavior
• Rust: Behavior considered undefined

Potential UB could happen if an old code receives a message generated on an updated schema with new discriminants added to a schema.
Let's generate a message using the future monster test.fbs schema which will have Race discriminant with -127 value: { "name": "FromFuture", "signed_enum": -127, }.
Add a test code. Result of the test execution with the current implementation of enum:

thread 'binary_data_from_future_source' has overflowed its stack
error: test failed, to rerun pass '--test integration_test'
Caused by:
  process didn't exit successfully: `E:\github\flatbuffers\tests\rust_usage_test\target\debug\deps\integration_test-6e2446f87a4c78ed.exe --quiet` (exit code: 0xc0000005, STATUS_ACCESS_VIOLATION)
./RustTest.sh: line 25: 16004 Segmentation fault      cargo test $TARGET_FLAG -- --quiet

The breaking changes to the user-facing API

1. Generated accessor method to a field of enum type returns Option<enum> instead of enum value.
2. Generated struct (Vec3 for example) holds underlying type values instead of enum type.

The changes in the code generator

1. Generated enum implements std::convert::TryFrom/From traits.
2. The Follow and Push implementations updated to use generated try_from/from.

Default semantic of flatbuffers values
The semantic didn't change.

The changes in the base code
The new method get_opt() added to Table type (table.rs). This method the same as the get() but returns T::Inner instead of Option<T::Inner>. It was added to preserve semantic of default: Option<T::Inner> if T::Inner is already Option<>.

All unit-tests touched by PR were updated.
The new test was added to test forward compatibility of the Race enum evolution in the Monster schema.

P.S. The PR was rebased to the master to pass the tests.

[vglavnyy]

@jean-airoldie

Can now be simply be expressed as:
enum Color: uint8 { Red=1, Blue=2, Green=8, }
which removes the need for special logic previously required by the Color::None case.

In Rust generated code the Color enum doesn't declare None discriminant.
It has NONE in C++ to implement all-flags-clear state (bit_flags).
This not implemented in Rust yet (bit_flags in Rust).

While union enumerators have explicit 'NONE' value to indicate an empty variant. For example Monster's Any, AnyUniqueAliases, AnyAmbiguousAliases unions.
For a union field we have two cases:

1. The field isn't set in a message or empty (now it is marked as NONE=0).
2. This is an unknown union from the future schema.
   Should we distinguish these cases?
   If not then we can remove NONE and replace it by Option::None.

[jean-airoldie]

In Rust generated code the Color enum doesn't declare None discriminant.

I mean't that you don't need an explicit value of the default (or unset) in an enum anymore.

Should we distinguish these cases?
If not then we can remove NONE and replace it by Option::None.

I don't think we should distinguish these cases and I think it would make sense to remove NONE. This way the behavior of the union and the enum would be consistent.

[vglavnyy]

@jean-airoldie I will remove generation of NONE after review.
@rw Could you review the changes?

[CasperN]

Hello, are we still working on this? I noticed bit_flag support wasn't implemented while working on #6070 and would like these change to get in first so I can work off of them. Suggestion: Instead of returning Option<MyEnum> we should return Result<MyEnum, u8> and expose the unknown discriminant. Sometimes, people want to log that stuff.

[vglavnyy]

No, I'm not working on it.