Skip to content

Commit

Permalink
cmd/protoc-gen-go: avoid referencing remote enum values by name
Browse files Browse the repository at this point in the history
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]>
  • Loading branch information
dsnet committed May 24, 2021
1 parent 50a8591 commit acaef6a
Show file tree
Hide file tree
Showing 9 changed files with 1,392 additions and 1,295 deletions.
24 changes: 20 additions & 4 deletions cmd/protoc-gen-go/internal_gengo/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -447,7 +447,15 @@ func genMessageDefaultDecls(g *protogen.GeneratedFile, f *fileInfo, m *messageIn
case protoreflect.EnumKind:
idx := field.Desc.DefaultEnumValue().Index()
val := field.Enum.Values[idx]
consts = append(consts, fmt.Sprintf("%s = %s", name, g.QualifiedGoIdent(val.GoIdent)))
if val.GoIdent.GoImportPath == f.GoImportPath {
consts = append(consts, fmt.Sprintf("%s = %s", name, g.QualifiedGoIdent(val.GoIdent)))
} else {
// If the enum value is declared in a different Go package,
// reference it by number since the name may not be correct.
// See https://github.com/golang/protobuf/issues/513.
consts = append(consts, fmt.Sprintf("%s = %s(%d) // %s",
name, g.QualifiedGoIdent(field.Enum.GoIdent), val.Desc.Number(), g.QualifiedGoIdent(val.GoIdent)))
}
case protoreflect.FloatKind, protoreflect.DoubleKind:
if f := defVal.Float(); math.IsNaN(f) || math.IsInf(f, 0) {
var fn, arg string
Expand Down Expand Up @@ -550,7 +558,7 @@ func genMessageGetterMethods(g *protogen.GeneratedFile, f *fileInfo, m *messageI

// Getter for message field.
goType, pointer := fieldGoType(g, f, field)
defaultValue := fieldDefaultValue(g, m, field)
defaultValue := fieldDefaultValue(g, f, m, field)
g.Annotate(m.GoIdent.GoName+".Get"+field.GoName, field.Location)
leadingComments := appendDeprecationSuffix("",
field.Desc.Options().(*descriptorpb.FieldOptions).GetDeprecated())
Expand Down Expand Up @@ -672,7 +680,7 @@ func fieldProtobufTagValue(field *protogen.Field) string {
return tag.Marshal(field.Desc, enumName)
}

func fieldDefaultValue(g *protogen.GeneratedFile, m *messageInfo, field *protogen.Field) string {
func fieldDefaultValue(g *protogen.GeneratedFile, f *fileInfo, m *messageInfo, field *protogen.Field) string {
if field.Desc.IsList() {
return "nil"
}
Expand All @@ -691,7 +699,15 @@ func fieldDefaultValue(g *protogen.GeneratedFile, m *messageInfo, field *protoge
case protoreflect.MessageKind, protoreflect.GroupKind, protoreflect.BytesKind:
return "nil"
case protoreflect.EnumKind:
return g.QualifiedGoIdent(field.Enum.Values[0].GoIdent)
val := field.Enum.Values[0]
if val.GoIdent.GoImportPath == f.GoImportPath {
return g.QualifiedGoIdent(val.GoIdent)
} else {
// If the enum value is declared in a different Go package,
// reference it by number since the name may not be correct.
// See https://github.com/golang/protobuf/issues/513.
return g.QualifiedGoIdent(field.Enum.GoIdent) + "(" + strconv.FormatInt(int64(val.Desc.Number()), 32) + ")"
}
default:
return "0"
}
Expand Down
2 changes: 1 addition & 1 deletion cmd/protoc-gen-go/testdata/import_public/a.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion cmd/protoc-gen-go/testdata/import_public/b.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions internal/testprotos/conformance/test_messages_proto3.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion internal/testprotos/fieldtrack/fieldtrack.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2,642 changes: 1,359 additions & 1,283 deletions internal/testprotos/test/test.pb.go

Large diffs are not rendered by default.

5 changes: 5 additions & 0 deletions internal/testprotos/test/test.proto
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ syntax = "proto2";

package goproto.proto.test;

import "google/protobuf/descriptor.proto";
import "internal/testprotos/test/test_import.proto";
import public "internal/testprotos/test/test_public.proto";
import weak "internal/testprotos/test/weak1/test_weak.proto";
Expand Down Expand Up @@ -381,3 +382,7 @@ service TestDeprecatedService {
message WeirdDefault {
optional bytes weird_default = 1 [default = "hello, \"world!\"\ndead\xde\xad\xbe\xefbeef`"];
}

message RemoteDefault {
optional google.protobuf.FieldDescriptorProto.Type type = 1 [default = TYPE_ENUM];
}
2 changes: 1 addition & 1 deletion internal/testprotos/textpb2/test.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions types/known/apipb/api.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit acaef6a

Please sign in to comment.