search

google / flatbuffers

FlatBuffers: Memory Efficient Serialization Library
https://flatbuffers.dev/
Apache License 2.0
23.08k stars 3.22k forks source link

Flatc converts enums within proto messages to wrong namespace in fbs [all languages, FlatBuffers 1.12, 2.0] #7109

Open ClemensLinnhoff opened 2 years ago

ClemensLinnhoff commented 2 years ago

We are trying out Flatbuffers instead of the currently used Protobuf in the ASAM Open Simulation Interface.

We are converting interface definitions from proto files to fbs using flatc. In the proto files we have nested messages with enums of the same name. See the following example from the osi_object.proto:

message MovingObject​
{​
  ...
  optional Type type = 3;
  ...
  optional VehicleClassification vehicle_classification = 6;
  enum Type​
  {​
    TYPE_UNKNOWN = 0;​
    TYPE_OTHER = 1;​
    TYPE_VEHICLE = 2;​
    TYPE_PEDESTRIAN = 3;​
    TYPE_ANIMAL = 4;​
  }​
  message VehicleClassification​
  {​
    enum Type​
    {​
    TYPE_UNKNOWN = 0;​
    TYPE_OTHER = 1;​
    TYPE_SMALL_CAR = 2;​
    TYPE_COMPACT_CAR = 3;​
    …​
    }​
  }​
}​​

The namespaces of the enums are correctly set by flatc in the fbs files:

namespace osi3.MovingObject_;​
enum Type : int {​
  TYPE_UNKNOWN = 0,​
  TYPE_OTHER = 1,​
  TYPE_VEHICLE = 2,​
  TYPE_PEDESTRIAN = 3,​
  TYPE_ANIMAL = 4,​
}​
namespace osi3.MovingObject_.VehicleClassification_;​
enum Type : int {​
  TYPE_UNKNOWN = 0,​
  TYPE_OTHER = 1,​
  TYPE_SMALL_CAR = 2,​
  TYPE_COMPACT_CAR = 3,​
  …​
}

The usage of the enums however is wrong:

table MovingObject {​
  ...
  type:osi3.StationaryObject_.Classification_.Type;​  //should be type:osi3.MovingObject_.Type;
  ...
  vehicle_classification:osi3.MovingObject_.VehicleClassification;​
}​

namespace osi3.MovingObject_;​
table VehicleClassification {​
  type:osi3.MovingObject_.Type;​  //should be type:osi3.MovingObject_.VehicleClassification_.Type;​
  ...
}

The namespaces of the enums "Type" are mixed up.

dbaileychess commented 2 years ago

Thanks for the report. Could you stripe down the proto to a bare minimum example and also post the command line arguments you are using? I can take a look.

dbaileychess commented 2 years ago

Can you look at #7121, I tried to recreate the issue you are seeing and it seems to be generating it correctly. Maybe there is some other tidbit that is different.

ClemensLinnhoff commented 2 years ago

Thank you for your reply. I did a little more testing and it seems that the order of defining the enums and defining the messages using the enums plays a role. I attached two minimal examples.

As a command to convert the proto file to fbs I simply use the following command: ./flatc --proto osi_object_minimal.proto

osi_object_minimal.zip

dbaileychess commented 2 years ago

Thanks, I'll take a look.

PhRosenberger commented 1 year ago

Hi @dbaileychess! Any progress on our issue here? Did you have a look? We would be very happy , if this could be fixed!

jdsika commented 1 year ago

Thanks, I'll take a look.

Hey @dbaileychess do you need any more information on this or support? Is there already a plan to integrate a fix in the next release? Could you outline a timeframe for this? Thank you very much!

le-michael commented 1 year ago

At an initial glance this bug seems to be the result of how we parse proto files. We can be reproduces this bug if the following criteria are met:

  1. The enum declaration is defined after the field that references it.
  2. There exists an enum of the same name that is defined above the field.

When we perform an enum look up we traverse up the namespace until we find a matching qualified name in the symbol table for defined enums. https://github.com/google/flatbuffers/blob/master/src/idl_parser.cpp#L141-L147

If our desired enum is defined after the field it won't exists in the symbols table so our look up function will travel up the namespace to find an enum with a similiar name.

To fix this issue, off the top of my head I think we can do a two pass on the proto file. On the initial pass we can populate our symbol tables and on the second pass we'll process the proto file like we currently do.

Here's another minimal example to reproduce this bug.

Example ``` syntax = "proto2"; package bug; enum Error { UNKNOWN = 0; INTERNAL = 1; } message Outer { optional Error outer_error = 1; enum Error { UNKNOWN = 0; INTERNAL = 1; } message Inner { optional Error inner_error = 1; enum Error { UNKNOWN = 0; INTERNAL = 1; } } } ``` ``` // Generated from minimal.proto namespace bug; enum Error : int { UNKNOWN = 0, INTERNAL = 1, } namespace bug.Outer_; enum Error : int { UNKNOWN = 0, INTERNAL = 1, } namespace bug.Outer_.Inner_; enum Error : int { UNKNOWN = 0, INTERNAL = 1, } namespace bug; table Outer { outer_error:bug.Error; } namespace bug.Outer_; table Inner { inner_error:bug.Outer_.Error; } ```
le-michael commented 1 year ago

Blocked by #7827

github-actions[bot] commented 1 year ago

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.

jdsika commented 1 year ago

Seems that this is not fixed yet?

le-michael commented 1 year ago

Not fixed yet. I'll take a look at this again.

github-actions[bot] commented 6 months ago

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.

ClemensLinnhoff commented 6 months ago

This issue still needs to be fixed.

github-actions[bot] commented 3 weeks ago

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.

jdsika commented 3 weeks ago

This is still no resolved but needed