Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
tonicsoft opened this issue Nov 20, 2019 · 7 comments
Closed

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

tonicsoft opened this issue Nov 20, 2019 · 7 comments
Assignees
Labels
documentation inactive Denotes the issue/PR has not seen activity in the last 90 days.

Comments

@tonicsoft
Copy link

tonicsoft commented Nov 20, 2019

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:
#5425
https://stackoverflow.com/questions/27030332/solutions-to-resolve-enum-field-naming-restriction-in-google-protobuf-due-to-c
protobuf-net/protobuf-net#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
Copy link

reavertm commented Apr 30, 2021

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:

  • users are forced to make enum labels unique to adhere to this object-global naming rule nonsense - so usually appending enum type name
  • protoc (C++) automatically also appends enum type name because why not

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
Copy link

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.

Copy link

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 github-actions bot added the inactive Denotes the issue/PR has not seen activity in the last 90 days. label Mar 31, 2024
@Levi-Lesches
Copy link

This seems to still be relevant

@github-actions github-actions bot removed the inactive Denotes the issue/PR has not seen activity in the last 90 days. label Apr 1, 2024
@Logofile
Copy link
Member

Logofile commented Apr 1, 2024

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

Copy link

github-actions bot commented Jul 1, 2024

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 github-actions bot added the inactive Denotes the issue/PR has not seen activity in the last 90 days. label Jul 1, 2024
Copy link

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.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation inactive Denotes the issue/PR has not seen activity in the last 90 days.
Projects
None yet
Development

No branches or pull requests

6 participants