protocolbuffers / protobuf

Protocol Buffers - Google's data interchange format
http://protobuf.dev
Other
64.95k stars 15.4k forks source link

C style enum name scoping rules should be documented. #6912

Closed tonicsoft closed 1 month ago

tonicsoft commented 4 years ago

What language does this apply to? All

Describe the problem you are trying to solve. There is some confusion about enum key naming rules around sibling enums not being allowed values with the same name. Some google searching helps (and the protoc error message iirc), but it's not very clear: https://github.com/protocolbuffers/protobuf/issues/5425 https://stackoverflow.com/questions/27030332/solutions-to-resolve-enum-field-naming-restriction-in-google-protobuf-due-to-c https://github.com/protobuf-net/protobuf-net/issues/60

Describe the solution you'd like The language guide (https://developers.google.com/protocol-buffers/docs/proto3#enum) should explain that defining two enums in the same package with values that have the same name (e.g. UNKNOWN) is invalid, and whether the same is true or not when one or both of the enums are nested inside a message.

It would also be nice if the code examples for enum were valid with respect to this rule, and followed the style guide with XXX_UNSPECIFIED rather than UNKNOWN. One of the current code examples is:

enum EnumAllowingAlias {
  option allow_alias = true;
  UNKNOWN = 0;
  STARTED = 1;
  RUNNING = 1;
}
enum EnumNotAllowingAlias {
  UNKNOWN = 0;
  STARTED = 1;
  // RUNNING = 1;  // Uncommenting this line will cause a compile error inside Google and a warning message outside.
}
reavertm commented 3 years ago

This restriction is completely bogus and imho it should be removed.

protoc for:

message Class {
  enum Enum {
    Value1 = 1;
    Value2 = 2;
  }
  enum AnotherEnum {
    Value1 = 1;
    Value2 = 2;
  }
}

in C++ generator will happily prefix all enum labels with enum names automatically in .pb.h:

enum Class_Enum : int {
  Class_Enum_Value1 = 1;
  Class_Enum_Value2 = 2;
}

so if AnotherEnum was allowed to be generated, there would be no name collision. Python generator may behave differently.

Own this design mistake guys - either remove this restriction and update generators to generate enum labels in unique fashion or update generators to actually adhere to it - do no append enum type name to generated enum value labels (like in C++ one).

What we have now is the worst of two worlds:

Which results with generated labels like: Class_Enum_Enum_Value1.

About inevitable "do not append enum type name to enum label". What should be appended then to make it unique? In real world environment not all .proto files can be handcrafted and many will be generated from model defined elsewhere, leaving no room for handcrafting (like in AnotherEnum use differently called labels), only prefixing, suffixing labels is possible.

enum Enum {
  E1_Value1 = 1;
  E1_Value2 = 2;
}
enum AnotherEnum {
  E2_Value1 = 1;  // E2 because this is second enum in scope of this message that defines Value1 enum, to satisfy protobuf per-message uniqueness
  E2_Value2 = 2;  // E2 because this is second enum in scope of this message that defines Value2 enum, to satisfy protobuf per-message uniqueness
}

Using fake message container for namespace reasons is a workaround. How about making a fix instead for a change?

NickolasLapp commented 1 year ago

Just wondering if there is any update on this request. From the C-side, being forced to use unique enum labels in the same module is quite cumbersome. It would be good to know if there is any planned resolution on this.

github-actions[bot] commented 4 months ago

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please add a comment.

This issue is labeled inactive because the last activity was over 90 days ago.

Levi-Lesches commented 4 months ago

This seems to still be relevant

Logofile commented 4 months ago

Thanks for keeping this issue open, @Levi-Lesches .

github-actions[bot] commented 1 month ago

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please add a comment.

This issue is labeled inactive because the last activity was over 90 days ago.

github-actions[bot] commented 1 month ago

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please reopen it.

This issue was closed and archived because there has been no new activity in the 14 days since the inactive label was added.