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 protoc-gen-go: determine plan for improving generated APIs #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.