protoc-gen-go: remove type name from generated enum

[aajtodd]

Given a proto definition

enum Foo {
    BAR = 0;
    BAZ = 1;
}

generates the following

type Foo int32
const (
    Foo_BAR Foo = 0
    Foo_BAZ Foo = 1
)

Protobuf enums already follow C++ scoping rules though so this seems unnecessary as you should be guaranteed of not having a collision within a single package.

protoc rejects the following already:

syntax = "proto3";
package testpkg;
option go_package = "testpkg";

enum Foo {
    BAR = 0;
    BAZ = 1;
}

enum Foo2 {
    BAR = 1;
}

test.proto:31:5: "BAR" is already defined in "testpkg".
test.proto:31:5: Note that enum values use C++ scoping rules, meaning that enum values are siblings of their type, not children of it.  Therefore, "BAR" must be unique within "testpkg", not just within "Foo2".

This may seem small but in practice we have found it to make code harder to read and follow with longer enums (both longer type name and longer enum values).

Unless I'm missing something that makes this required behavior it would be nice to have an option to turn this off.

[johanbrandhorst]

This option exists in gogo/protobuf.

[neild]

Since these are just constants, it would be possible to generate both Type_VALUE and VALUE for backwards compatibility at the cost of somewhat more confusing generated output.

[awalterschulze]

Why not give the user the option to choose?

[neild]

A forced choice between one or the other makes migration difficult. Having the option to use both during a transitional period would make it more practical to change from one to the other.

[awalterschulze]

I am not an expert on type aliases, but I think the whole reason type aliases got in the language was to support transitioning. So can we use it for this use case?

Another way to support transitioning is to copy the Type_ enums once into a non generated file and then remove them once the transition is complete.

Also is transitioning such a big use case?

[aajtodd]

To be clear I do not think it should be added as a new default which would cause breaking changes. I would think the common use case would be to use it one way or the other which is why I suggested it be an option.

Personally I don't see a huge benefit to supporting both at same time via type aliases or otherwise through the protobuf compiler.

[neild]

We don't need type aliases in this case; the existing code is something like:

type TypeName int32

const (
  TypeName_ONE TypeName = 1
  TypeName_TWO TypeName = 2
)

The type name is fine; it's just the values which have extra junk added to them. We'd like this to read:

const (
  ONE TypeName = 1
  TWO TypeName = 2
)

Since these are constants, there's no technical reason we couldn't include both forms of the constants in the generated file; TypeName_ONE and ONE would be entirely interchangeable. This would make it possible to migrate a code base from the former style to the latter--generate both forms, mark the old one as deprecated, update all uses over time, and eventually drop the deprecated form.

It's honestly a bit tempting to say that the right knob to support would be "generate both forms" or "only generate the short form", given that the short one seems strictly superior, but a tristate selecting between one, the other, or both is probably more practical.

(None of this is to say that we are or aren't doing this; I'm just speculating on the technical feasibility at the moment.)

[dsnet]

Protobuf enums already follow C++ scoping rules though so this seems unnecessary as you should be guaranteed of not having a collision within a single package.

This is not strictly true. In the following proto file:

enum Enum { foo = 0; }
message Foo {}

The foo enum must be renamed as Foo in Go and will conflict the with the Foo struct name.

This occurs because:

1. Names are case-sensitive in proto
2. Exported identifiers must be uppercase in Go

However, a collision is rare, and is already possible today for certain situations:

message Foo_Bar{}
message Foo{ message Bar{} }

---

That being said:

1. If we agree that dropping the prefix is desired behavior, then it should be the default behavior.
2. We cannot make it the default today or it will break a ton of stuff today.
3. This issue is not the only generated API issue we would like to solve. It's not clear to me that a single go_option for this is the right behavior, where you opt-in or opt-out. For now, I'm going to put this on hold until we know what we want to do in #526.

[aajtodd]

Good point, I hadn't thought of the case sensitive conflict between a message and an enum specific to Go. That being said I still think I prefer pushing this issue to the user and removing the prefix.

That plan works for me, thanks.

[awalterschulze]

On a meta topic. I think it is really hard for protoc to tell which proto definitions do not cause a naming conflict in any language, so protoc does not do any of this detection. This in turn makes it really hard to define a proto file once, without changing it, when you add another target language.

But anyway in gogoprotobuf you can use the gogoproto.goproto_enum_prefix = false extension turn enum prefix generation on or off.

On the topic of transition in general, the gogoproto.goproto_unrecognized extension could be useful to keep the dev branch compatible. This could allow turning on and off the generation of the XXX_fields that "break" compatibility on the dev branch at the moment.

Not saying that extensions is necessarily the perfect solution, but using feature flags of some sort (comments, command line parameters, vanity binaries or extensions), gives users flexibility and allows you to make breaking changes to keep things moving forward without breaking users' current setup.

[dsnet]

I proposed go_name in #555 as an alternative to goproto_enum_prefix. It's more work specifying it on each value, but it is more generalizable to a number of different use cases.

This issue will be primarily about getting the generator to one day always dropping the type prefix.

[bogdandrutu]

Any update on this? It is very annoying especially because the style guide recommends https://developers.google.com/protocol-buffers/docs/style#enums using as a prefix the enum name, and in go this will duplicate the type.

@neild: Having options like:

• new only
• both - default

[panbanda]

I'll jump in here and say our use case uses buf and the tool enforces the type in the name in the enum itself. This is valid:

enum ACL {
  ACL_UNSPECIFIED = 0;
  ACL_PRIVATE = 1;
  ACL_PUBLIC_READ = 2;
}

Our js and other languages that we generate do this fine, but our go generates like this:

  ACL_ACL_UNSPECIFIED
  ACL_ACL_PRIVATE
  ACL_ACL_PUBLIC_READ

[puellanivis]

It might make sense that when generating, we recognize if we’re stuttering the type name, because it’s already included in the enum value names? Though, unfortunately, this is still likely to break existing code as well. 🤦‍♀️ (This is why we can’t have nice things.)

[belolap]

Is it possible to make some option and keep default behavior if option is not set?

[kluevandrew]

Any updates?

[rmsz005]

Any updates?

[kluevandrew]

Any body home?

[hypnoglow]

If you follow the recommendation to prefix all enum values like so:

// Enumerates available operation statuses.
enum OperationStatus {
  // Operation status not provided or unknown.
  OPERATION_STATUS_UNSPECIFIED = 0;

  // Operation succeeded.
  OPERATION_STATUS_SUCCEEDED = 1;

  // Operation failed.
  OPERATION_STATUS_FAILED = 2;
}

And you find generated Go code weird with double prefixes:

// Enumerates available operation statuses.
type OperationStatus int32

const (
	// Operation status not provided or unknown.
	OperationStatus_OPERATION_STATUS_UNSPECIFIED OperationStatus = 0
	// Operation succeeded.
	OperationStatus_OPERATION_STATUS_SUCCEEDED OperationStatus = 1
	// Operation failed.
	OperationStatus_OPERATION_STATUS_FAILED OperationStatus = 2
)

Try the following approach: use wrapper messages for enums.

// Wraps operation status enumeration.
message OperationStatus {
  // Enumerates available operation statuses.
  enum Enum {
    // Operation status not provided or unknown.
    UNSPECIFIED = 0;

    // Operation succeeded.
    SUCCEEDED = 1;

    // Operation failed.
    FAILED = 2;
  }
}

Generated code:

// Enumerates available operation statuses.
type OperationStatus_Enum int32

const (
	// Operation status not provided or unknown.
	OperationStatus_UNSPECIFIED OperationStatus_Enum = 0
	// Operation succeeded.
	OperationStatus_SUCCEEDED OperationStatus_Enum = 1
	// Operation failed.
	OperationStatus_FAILED OperationStatus_Enum = 2
)

[jalaziz]

Given that it doesn't look like #526 is going to happen any time soon, it would be great if we could just have an option to disable the enum prefixing. This wouldn't break any existing code, but would help folks who are just adopting protobuf with Golang rather than forcing them to inherit tech debt from the start.

I can't promise I'll have time soon, but I could definitely try to put up a PR if that's the issue.

[samborkent]

Any update on this? It really is a major nuisance if you want to use protobuf to define your data structures in Go.

[mgingios]

I am having the same issue.

[timurnkey]

can anyone recommend a strategy for making this more readable? We only have this problem with the generated Go client code:

# Protobuf
enum ActivityType {
  ...
  ACTIVITY_TYPE_SET_ORGANIZATION_FEATURE = 50;
}

# Rust
ActivityType::SetOrganizationFeature

# Golang
ActivityTypeACTIVITYTYPESETORGANIZATIONFEATURE ActivityType = "ACTIVITY_TYPE_SET_ORGANIZATION_FEATURE"

Ideally we'd end up with something like:

ActivityType_SET_ORGANIZATION_FEATURE

[samborkent]

can anyone recommend a strategy for making this more readable? We only have this problem with the generated Go client code:

# Protobuf
enum ActivityType {
  ...
  ACTIVITY_TYPE_SET_ORGANIZATION_FEATURE = 50;
}

# Rust
ActivityType::SetOrganizationFeature

# Golang
ActivityTypeACTIVITYTYPESETORGANIZATIONFEATURE ActivityType = "ACTIVITY_TYPE_SET_ORGANIZATION_FEATURE"

Ideally we'd end up with something like:

ActivityType_SET_ORGANIZATION_FEATURE

There is this proto plugin: https://github.com/alta/protopatch However, you need to install it locally. We don't use it as we use buf.build, so locally installed plugins kind of defeat the purpose.

I'd argue that the desired generated enum type name is ActivityTypeSetOrganizationFeature to be more Go idiomatic.