[protobuf] cmd/protoc-gen-go: avoid referencing remote enum values by name
Joe Tsai (Gerrit)
Joe Tsai has uploaded this change for review.
cmd/protoc-gen-go: avoid referencing remote enum values by name
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 https://github.com/golang/protobuf/issues/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
---
M cmd/protoc-gen-go/internal_gengo/main.go
M cmd/protoc-gen-go/testdata/import_public/a.pb.go
M cmd/protoc-gen-go/testdata/import_public/b.pb.go
M internal/testprotos/conformance/test_messages_proto3.pb.go
M internal/testprotos/fieldtrack/fieldtrack.pb.go
M internal/testprotos/test/test.pb.go
M internal/testprotos/test/test.proto
M internal/testprotos/textpb2/test.pb.go
M types/known/apipb/api.pb.go
9 files changed, 1,410 insertions(+), 1,313 deletions(-)
To view, visit change 319649. To unsubscribe, or for help writing mail filters, visit settings.
Ingo Oeser (Gerrit)
Attention is currently required from: Joe Tsai.
1 comment:
Patchset:
I am not sure this makes things better.
Now a compile error that an developer expects when the enums are changed incompatibly are not detected by the compiler anymore.
Optionally (or always) generating additional prefix-less variants of the enum values (if they don't conflict) might be an alternative fix.
To view, visit change 319649. To unsubscribe, or for help writing mail filters, visit settings.
Joe Tsai (Gerrit)
Attention is currently required from: Ingo Oeser, Joe Tsai.
1 comment:
Patchset:
Now a compile error that an developer expects when the enums are changed incompatibly are not detected by the compiler anymore.
Can you explain with a concrete example?
In proto3, there are no default values and the first enum value must be zero. Thus, it is always safe to hard-code zero.
In proto2, you may have default values (which is rare), and the first enum value might not be zero (which is also rare). In the rare situation someone changes the meaning of the enums values, then we might have hardcoded a stale value, but the changes of this happening are pretty low.
Optionally (or always) generating additional prefix-less variants of the enum values (if they don't conflict) might be an alternative fix.
Probably, but that seems like an orthogonal step to take in conjunction with this one. Even if we generated the prefix-less enums, it doesn't change the fact that we don't know if we're allowed to reference them or not (which is what this CL aims to reduce friction on).
To view, visit change 319649. To unsubscribe, or for help writing mail filters, visit settings.
Ingo Oeser (Gerrit)
Attention is currently required from: Joe Tsai.
Patch set 1:Code-Review +1
1 comment:
Patchset:
Thanks for the explanation that this is about non-zero default values only. I clearly misjudged the scope of this change.
To view, visit change 319649. To unsubscribe, or for help writing mail filters, visit settings.
Joe Tsai (Gerrit)
Attention is currently required from: Ingo Oeser, Joe Tsai.
1 comment:
Patchset:
Thanks for the explanation that this is about non-zero default values only. […]
I still appreciate the review. Thanks!
To view, visit change 319649. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Attention is currently required from: Ingo Oeser, Joe Tsai.
Patch set 1:Code-Review +2
Joe Tsai (Gerrit)
Joe Tsai submitted this change.
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/319649
Trust: Joe Tsai <joe...@digital-static.net>
Reviewed-by: Damien Neil <dn...@google.com>
---
M cmd/protoc-gen-go/internal_gengo/main.go
M cmd/protoc-gen-go/testdata/import_public/a.pb.go
M cmd/protoc-gen-go/testdata/import_public/b.pb.go
M internal/testprotos/conformance/test_messages_proto3.pb.go
M internal/testprotos/fieldtrack/fieldtrack.pb.go
M internal/testprotos/test/test.pb.go
M internal/testprotos/test/test.proto
M internal/testprotos/textpb2/test.pb.go
M types/known/apipb/api.pb.go
9 files changed, 1,410 insertions(+), 1,313 deletions(-)
To view, visit change 319649. To unsubscribe, or for help writing mail filters, visit settings.