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

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

Open
aajtodd opened this issue Feb 8, 2018 · 30 comments
Open

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

aajtodd opened this issue Feb 8, 2018 · 30 comments
Labels
breaking-change requires making a breaking API change generator-proto-option involves generators respecting a proto option to control generated source output proposal proposal-hold
Milestone

Comments

@aajtodd
Copy link

aajtodd commented Feb 8, 2018

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.

@dsnet dsnet added the proposal label Feb 13, 2018
@dsnet dsnet changed the title Proposal - option to remove type name from generated enum proposal: protoc-gen-go: option to remove type name from generated enum Feb 13, 2018
@johanbrandhorst
Copy link
Member

johanbrandhorst commented Feb 18, 2018

This option exists in gogo/protobuf.

@neild
Copy link
Contributor

neild commented Feb 21, 2018

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

Why not give the user the option to choose?

@neild
Copy link
Contributor

neild commented Feb 21, 2018

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

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

aajtodd commented Feb 21, 2018

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

neild commented Feb 21, 2018

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

dsnet commented Feb 22, 2018

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

aajtodd commented Feb 22, 2018

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

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

dsnet commented Mar 9, 2018

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.

@dsnet dsnet added the generator-proto-option involves generators respecting a proto option to control generated source output label Mar 9, 2018
@dsnet dsnet changed the title proposal: protoc-gen-go: option to remove type name from generated enum proposal: protoc-gen-go: remove type name from generated enum Mar 14, 2018
@dsnet dsnet added the breaking-change requires making a breaking API change label Mar 14, 2018
@dsnet dsnet changed the title proposal: protoc-gen-go: remove type name from generated enum protoc-gen-go: remove type name from generated enum Mar 4, 2020
@bogdandrutu
Copy link

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

@dsnet dsnet added this to the unplanned milestone Mar 29, 2021
gopherbot pushed a commit to protocolbuffers/protobuf-go that referenced this issue May 24, 2021
The Go generator has historically always prefixed an enum value
with the name of the enum type, when it was unnecessary to do so.

For example:
	enum Status {
		STATUS_FAILED = 0;
		STATUS_PASSED = 1;
	}
would be generated as:
	type Status int32
	const (
		Status_STATUS_FAILED Status = 0
		Status_STATUS_PASSED Status = 1
	)

It is common for the enum values to be manually prefixed by the
enum type since protobuf enums use C++ namespace rules where
enum types and enum values are in the same namespace scope.
Thus, having the Go generator add a prefix is redundant.
See golang/protobuf#513.

Some custom Go generators like protoc-gen-gogo allow removing
the prefix with the gogoproto.goproto_enum_prefix feature.
However, this leads to interoperability issues between
protoc-gen-go and protoc-gen-gogo, where the enum value names
cannot be accurately inferred.

Avoid this problem by just hard-coding the enum value number
for values declared in other packages. This provides benefits
in interoperability at the small cost of enum values possibly
being stale if their value were ever changed in a remote package.
However, this would only occur with use of proto2 enums and
default values, which seems to be an exceptionally rare situation.

Before:
	Default_MyMessage_MyField = remotepb.FooEnum_FOO_ENUM
After:
	Default_MyMessage_MyField = remotepb.FooEnum(4) // remotepb.FooEnum_FOO_ENUM

Before:
	func (x *MyMessage) GetField() remotepb.FooEnum {
		...
		return remotepb.FooEnum_FOO_ZERO
	}
After:
	func (x *MyMessage) GetField() remotepb.FooEnum {
		...
		return remotepb.FooEnum(0) // always 0 for proto3 and often 0 for proto2
	}

Change-Id: I3a06cd553f2eaf6124666f6c36c196d500d35718
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/319649
Trust: Joe Tsai <[email protected]>
Reviewed-by: Damien Neil <[email protected]>
@panbanda
Copy link

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

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

belolap commented Aug 13, 2022

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

@kluevandrew
Copy link

Any updates?

1 similar comment
@rmsz005
Copy link

rmsz005 commented May 11, 2023

Any updates?

@kluevandrew
Copy link

Any body home?

@hypnoglow
Copy link

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

jalaziz commented Sep 5, 2023

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

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

@mgingios
Copy link

I am having the same issue.

@timurnkey
Copy link

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

samborkent commented Oct 22, 2023

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.

@Jonathan-Landeed
Copy link

Jonathan-Landeed commented Oct 4, 2024

This is one thing I really miss about Rust.

I added a protopatch Buf repo so it can be used as a dependency, though it still needs to be used as a local plugin. bufbuild/plugins#858 (comment)

@ydnar
Copy link

ydnar commented Oct 5, 2024

I'm happy to donate the protopatch code to this project to improve the ergonomics of the generated Go.

@stapelberg
Copy link

With the release of Protobuf Editions (see https://protobuf.dev/editions/overview/), introducing an option is now much better supported than before.

Specifically, we could:

  • Introduce a Go-specific editions feature for edition 2024 (to be released in early 2025). You would add something like option features.(pb.go).strip_enum_prefix = STRIP; to your enum declaration to opt into stripping the prefix.
  • The default value would be option features.(pb.go).strip_enum_prefix = KEEP; to not change the current behavior and keep existing code working.
  • To facilitate migrating code, we can support option features.(pb.go).strip_enum_prefix = GENERATE_BOTH; as an intermediate step. This allows you to write new code without the prefix, while keeping existing code working.
  • When we change the default (for edition 2025) to STRIP, the prototiller tool can automatically update existing .proto files while keeping the existing semantics (i.e. adding option features.(pb.go).strip_enum_prefix = KEEP; explicitly to existing .proto files). You can then remove these options piece-by-piece.

I’ll ask the Protobuf team for feedback on this and will report back in a few days.

@stapelberg
Copy link

As discussed, I talked to folks from Protobuf team about this and have gotten positive feedback. We’re discussing the details of the change over at https://go.dev/cl/618979 in case folks are interested.

@ydnar
Copy link

ydnar commented Oct 12, 2024

Why not go the rest of the way and implement idiomatic Go const form?

@stapelberg
Copy link

Why not go the rest of the way and implement idiomatic Go const form?

In my mind, .proto files are a separate domain from Go code, and when foreign constants are made available in Go, it is common to keep the spelling of the foreign domain. Examples from the standard library include os.O_RDWR (where the foreign domain is the OS file API) or syslog.LOG_EMERG (where the foreign domain is the syslog API).

Hence, because Protobuf enum values are canonically spelled uppercase in their domain (see https://protobuf.dev/programming-guides/style/#enums), it is okay to keep them uppercase in the Protobuf generated Go code.

I’m not sure whether the case for switching enum values to camel case is as strong as it is for stripping the prefix, where it seems consensus that stripping the prefix is welcome.

If you want to pursue this, I suggest opening a separate, new feature request.

gopherbot pushed a commit to protocolbuffers/protobuf-go that referenced this issue Oct 23, 2024
This change required updating editions_defaults.binpb with
--edition_defaults_maximum=2024 so that we can use edition 2024 in our
testdata/enumprefix.proto test file.

For end users, this feature will only be available once:
1. protoc declares support for edition 2024 (expected early 2025).
2. protoc-gen-go declares support for edition 2024.

related to golang/protobuf#513

Change-Id: Ib8daeecae39ef32eff942279707d256c312f2a53
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/618979
Reviewed-by: Lasse Folger <[email protected]>
Reviewed-by: Mike Kruskal <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Damien Neil <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change requires making a breaking API change generator-proto-option involves generators respecting a proto option to control generated source output proposal proposal-hold
Projects
None yet
Development

No branches or pull requests