cmd/protoc-gen-go: reduce technical debt
The following TODOs were addressed:
* Consistently collect all enums, messages, and extensions in a breadth-first order.
The practical affect of this is that the declaration order in a Go file may change.
This simplifies reflection generation, which relies on consistent ordering.
* Removal of placeholder declarations (e.g., "var _ = proto.Marshal") since
protogen is intelligent about including imports as necessary.
* Always generate a default variable or constant for explicit empty strings.
The practical effect of this is the addition of new declarations in some cases.
However, it simplifies our logic such that it matches the protobuf data model.
* Generate the registration statements in a consistent order.
Change-Id: I627bb72589432bb65d53b50965ea88e5f7983977
Reviewed-on: https://go-review.googlesource.com/c/152778
Reviewed-by: Damien Neil <dneil@google.com>
diff --git a/cmd/protoc-gen-go/internal_gengo/main.go b/cmd/protoc-gen-go/internal_gengo/main.go
index d64b2da..7b129d4 100644
--- a/cmd/protoc-gen-go/internal_gengo/main.go
+++ b/cmd/protoc-gen-go/internal_gengo/main.go
@@ -40,11 +40,12 @@
type fileInfo struct {
*protogen.File
descriptorVar string // var containing the gzipped FileDescriptorProto
- allEnums []*protogen.Enum
- allMessages []*protogen.Message
- allExtensions []*protogen.Extension
- fileReflect fileReflect
+ allEnums []*protogen.Enum
+ allEnumsByPtr map[*protogen.Enum]int // value is index into allEnums
+ allMessages []*protogen.Message
+ allMessagesByPtr map[*protogen.Message]int // value is index into allMessages
+ allExtensions []*protogen.Extension
}
// protoPackage returns the package to import, which is either the protoPackage
@@ -75,20 +76,30 @@
File: file,
}
- // The different order for enums and extensions is to match the output
- // of the previous implementation.
- //
- // TODO: Eventually make this consistent (and remove fileReflect).
- f.allEnums = append(f.allEnums, f.File.Enums...)
- walkMessages(f.Messages, func(message *protogen.Message) {
- f.allMessages = append(f.allMessages, message)
- f.allEnums = append(f.allEnums, message.Enums...)
- f.allExtensions = append(f.allExtensions, message.Extensions...)
+ // Collect all enums, messages, and extensions in a breadth-first order.
+ f.allEnums = append(f.allEnums, f.Enums...)
+ f.allMessages = append(f.allMessages, f.Messages...)
+ f.allExtensions = append(f.allExtensions, f.Extensions...)
+ walkMessages(f.Messages, func(m *protogen.Message) {
+ f.allEnums = append(f.allEnums, m.Enums...)
+ f.allMessages = append(f.allMessages, m.Messages...)
+ f.allExtensions = append(f.allExtensions, m.Extensions...)
})
- f.allExtensions = append(f.allExtensions, f.File.Extensions...)
- // Initialize data structures needed for reflection.
- f.fileReflect.init(f)
+ // Derive a reverse mapping of enum and message pointers to their index
+ // in allEnums and allMessages.
+ if len(f.allEnums) > 0 {
+ f.allEnumsByPtr = make(map[*protogen.Enum]int)
+ for i, e := range f.allEnums {
+ f.allEnumsByPtr[e] = i
+ }
+ }
+ if len(f.allMessages) > 0 {
+ f.allMessagesByPtr = make(map[*protogen.Message]int)
+ for i, m := range f.allMessages {
+ f.allMessagesByPtr[m] = i
+ }
+ }
// Determine the name of the var holding the file descriptor:
//
@@ -118,15 +129,7 @@
// This section exists to generate output more consistent with
// the previous version of protoc-gen-go, to make it easier to
// detect unintended variations.
- //
- // TODO: Eventually remove this.
if !isDescriptor(file) {
- g.P("// Reference imports to suppress errors if they are not otherwise used.")
- g.P("var _ = ", protoPackage.Ident("Marshal"))
- g.P("var _ = ", fmtPackage.Ident("Errorf"))
- g.P("var _ = ", mathPackage.Ident("Inf"))
- g.P()
-
g.P("// This is a compile-time assertion to ensure that this generated file")
g.P("// is compatible with the proto package it is being compiled against.")
g.P("// A compilation error at this line likely means your copy of the")
@@ -145,7 +148,7 @@
for _, message := range f.allMessages {
genMessage(gen, g, f, message)
}
- for _, extension := range f.Extensions {
+ for _, extension := range f.allExtensions {
genExtension(gen, g, f, extension)
}
@@ -197,7 +200,7 @@
}
enums = append(enums, message.Enums...)
for _, field := range message.Fields {
- if !fieldHasDefault(field) {
+ if !field.Desc.HasDefault() {
continue
}
defVar := protogen.GoIdent{
@@ -260,8 +263,6 @@
w.Close()
b = buf.Bytes()
- g.P("func init() {", f.protoPackage().Ident("RegisterFile"), "(", strconv.Quote(f.Desc.Path()), ", ", f.descriptorVar, ") }")
- g.P()
g.P("var ", f.descriptorVar, " = []byte{")
g.P("// ", len(b), " bytes of a gzipped FileDescriptorProto")
for len(b) > 0 {
@@ -436,7 +437,6 @@
tags = append(tags, `json:"-"`)
g.P(f.protoPackage().Ident("XXX_InternalExtensions"), " `", strings.Join(tags, " "), "`")
}
- // TODO XXX_InternalExtensions
g.P("XXX_unrecognized []byte `json:\"-\"`")
g.P("XXX_sizecache int32 `json:\"-\"`")
g.P("}")
@@ -519,7 +519,7 @@
// Constants and vars holding the default values of fields.
for _, field := range message.Fields {
- if !fieldHasDefault(field) {
+ if !field.Desc.HasDefault() {
continue
}
defVarName := "Default_" + message.GoIdent.GoName + "_" + field.GoName
@@ -606,9 +606,6 @@
if len(message.Oneofs) > 0 {
genOneofWrappers(gen, g, f, message)
}
- for _, extension := range message.Extensions {
- genExtension(gen, g, f, extension)
- }
}
// fieldGoType returns the Go type used for a field.
@@ -670,7 +667,7 @@
if field.Desc.Cardinality() == protoreflect.Repeated {
return "nil"
}
- if fieldHasDefault(field) {
+ if field.Desc.HasDefault() {
defVarName := "Default_" + message.GoIdent.GoName + "_" + field.GoName
if field.Desc.Kind() == protoreflect.BytesKind {
return "append([]byte(nil), " + defVarName + "...)"
@@ -691,26 +688,6 @@
}
}
-// fieldHasDefault returns true if we consider a field to have a default value.
-//
-// For consistency with the previous generator, it returns false for fields with
-// [default=""], preventing the generation of a default const or var for these
-// fields.
-//
-// TODO: Drop this special case.
-func fieldHasDefault(field *protogen.Field) bool {
- if !field.Desc.HasDefault() {
- return false
- }
- switch field.Desc.Kind() {
- case protoreflect.StringKind:
- return field.Desc.Default().String() != ""
- case protoreflect.BytesKind:
- return len(field.Desc.Default().Bytes()) > 0
- }
- return true
-}
-
func fieldJSONTag(field *protogen.Field) string {
return string(field.Desc.Name()) + ",omitempty"
}
@@ -785,6 +762,7 @@
}
g.P("func init() {")
+ g.P(f.protoPackage().Ident("RegisterFile"), "(", strconv.Quote(f.Desc.Path()), ", ", f.descriptorVar, ")")
for _, enum := range f.allEnums {
name := enum.GoIdent.GoName
g.P(f.protoPackage().Ident("RegisterEnum"), fmt.Sprintf("(%q, %s_name, %s_value)", enumRegistryName(enum), name, name))
@@ -794,10 +772,6 @@
continue
}
- for _, extension := range message.Extensions {
- genRegisterExtension(gen, g, f, extension)
- }
-
name := message.GoIdent.GoName
g.P(f.protoPackage().Ident("RegisterType"), fmt.Sprintf("((*%s)(nil), %q)", name, message.Desc.FullName()))
@@ -819,17 +793,13 @@
g.P(f.protoPackage().Ident("RegisterMapType"), fmt.Sprintf("((%v)(nil), %q)", goType, typeName))
}
}
- for _, extension := range f.Extensions {
- genRegisterExtension(gen, g, f, extension)
+ for _, extension := range f.allExtensions {
+ g.P(f.protoPackage().Ident("RegisterExtension"), "(", extensionVar(f.File, extension), ")")
}
g.P("}")
g.P()
}
-func genRegisterExtension(gen *protogen.Plugin, g *protogen.GeneratedFile, f *fileInfo, extension *protogen.Extension) {
- g.P(f.protoPackage().Ident("RegisterExtension"), "(", extensionVar(f.File, extension), ")")
-}
-
// deprecationComment returns a standard deprecation comment if deprecated is true.
func deprecationComment(deprecated bool) string {
if !deprecated {
diff --git a/cmd/protoc-gen-go/internal_gengo/reflect.go b/cmd/protoc-gen-go/internal_gengo/reflect.go
index 9200910..6b6ea68 100644
--- a/cmd/protoc-gen-go/internal_gengo/reflect.go
+++ b/cmd/protoc-gen-go/internal_gengo/reflect.go
@@ -42,48 +42,12 @@
// TODO: Add support for proto options.
-// fileReflect is embedded in fileInfo to maintain state needed for reflection.
-//
-// TODO: Remove this when we have the freedom to change the order of
-// fileInfo.{allEnums,allMessages,allExtensions} to be a breadth-first search
-// to ensure that all declarations are coalesced together.
-type fileReflect struct {
- allEnums []*protogen.Enum
- allEnumsByPtr map[*protogen.Enum]int // value is index into allEnums
- allMessages []*protogen.Message
- allMessagesByPtr map[*protogen.Message]int // value is index into allMessages
-}
-
-func (r *fileReflect) init(f *fileInfo) {
- r.allEnums = append(r.allEnums, f.Enums...)
- r.allMessages = append(r.allMessages, f.Messages...)
- walkMessages(f.Messages, func(m *protogen.Message) {
- r.allEnums = append(r.allEnums, m.Enums...)
- r.allMessages = append(r.allMessages, m.Messages...)
- })
-
- // Derive a reverse mapping of enum and message pointers to their index
- // in allEnums and allMessages.
- if len(r.allEnums) > 0 {
- r.allEnumsByPtr = make(map[*protogen.Enum]int)
- for i, e := range r.allEnums {
- r.allEnumsByPtr[e] = i
- }
- }
- if len(r.allMessages) > 0 {
- r.allMessagesByPtr = make(map[*protogen.Message]int)
- for i, m := range r.allMessages {
- r.allMessagesByPtr[m] = i
- }
- }
-}
-
func genReflectInitFunction(gen *protogen.Plugin, g *protogen.GeneratedFile, f *fileInfo) {
if !enableReflection(f.File) {
return
}
- if len(f.fileReflect.allEnums)+len(f.fileReflect.allMessages)+len(f.allExtensions)+len(f.Services) == 0 {
+ if len(f.allEnums)+len(f.allMessages)+len(f.allExtensions)+len(f.Services) == 0 {
return
}
@@ -103,20 +67,20 @@
// Populate all declarations for messages and enums.
// These are not declared in the literals to avoid an initialization loop.
if enums := f.Enums; len(enums) > 0 {
- i := f.fileReflect.allEnumsByPtr[enums[0]]
+ i := f.allEnumsByPtr[enums[0]]
g.P(fileDescVar, ".Enums = ", enumDescsVar, "[", i, ":", i+len(enums), "]")
}
if messages := f.Messages; len(messages) > 0 {
- i := f.fileReflect.allMessagesByPtr[messages[0]]
+ i := f.allMessagesByPtr[messages[0]]
g.P(fileDescVar, ".Messages = ", messageDescsVar, "[", i, ":", i+len(messages), "]")
}
- for i, message := range f.fileReflect.allMessages {
+ for i, message := range f.allMessages {
if enums := message.Enums; len(enums) > 0 {
- j := f.fileReflect.allEnumsByPtr[enums[0]]
+ j := f.allEnumsByPtr[enums[0]]
g.P(messageDescsVar, "[", i, "].Enums = ", enumDescsVar, "[", j, ":", j+len(enums), "]")
}
if messages := message.Messages; len(messages) > 0 {
- j := f.fileReflect.allMessagesByPtr[messages[0]]
+ j := f.allMessagesByPtr[messages[0]]
g.P(messageDescsVar, "[", i, "].Messages = ", messageDescsVar, "[", j, ":", j+len(messages), "]")
}
}
@@ -127,11 +91,11 @@
// v2 protobuf reflection interfaces. The EnumTypeOf and MessageTypeOf
// helper functions checks for compliance and derives a v2 type from the
// legacy v1 enum or message if necessary.
- for i, message := range f.fileReflect.allMessages {
+ for i, message := range f.allMessages {
for j, field := range message.Fields {
fieldSel := fmt.Sprintf("[%d].Fields[%d]", i, j)
if et := field.EnumType; et != nil {
- idx, ok := f.fileReflect.allEnumsByPtr[et]
+ idx, ok := f.allEnumsByPtr[et]
if ok {
// Locally defined enums are found in the type array.
g.P(messageDescsVar, fieldSel, ".EnumType = ", enumTypesVar, "[", idx, "]")
@@ -141,7 +105,7 @@
}
}
if mt := field.MessageType; mt != nil {
- idx, ok := f.fileReflect.allMessagesByPtr[mt]
+ idx, ok := f.allMessagesByPtr[mt]
if ok {
if mt.Desc.IsMapEntry() {
// Map entry types have no Go type generated for them.
@@ -209,11 +173,11 @@
g.P("}")
// Generate literals for enum descriptors.
- if len(f.fileReflect.allEnums) > 0 {
+ if len(f.allEnums) > 0 {
enumTypesVar := enumTypesVarName(f)
enumDescsVar := enumDescsVarName(f)
- g.P("var ", enumTypesVar, " = [", len(f.fileReflect.allEnums), "]", protoreflectPackage.Ident("EnumType"), "{")
- for i, enum := range f.fileReflect.allEnums {
+ g.P("var ", enumTypesVar, " = [", len(f.allEnums), "]", protoreflectPackage.Ident("EnumType"), "{")
+ for i, enum := range f.allEnums {
g.P(prototypePackage.Ident("GoEnum"), "(")
g.P(enumDescsVar, "[", i, "].Reference(),")
g.P("func(_ ", protoreflectPackage.Ident("EnumType"), ", n ", protoreflectPackage.Ident("EnumNumber"), ") ", protoreflectPackage.Ident("ProtoEnum"), " {")
@@ -223,8 +187,8 @@
}
g.P("}")
- g.P("var ", enumDescsVar, " = [", len(f.fileReflect.allEnums), "]", prototypePackage.Ident("Enum"), "{")
- for _, enum := range f.fileReflect.allEnums {
+ g.P("var ", enumDescsVar, " = [", len(f.allEnums), "]", prototypePackage.Ident("Enum"), "{")
+ for _, enum := range f.allEnums {
g.P("{")
g.P("Name: ", strconv.Quote(string(enum.Desc.Name())), ",")
g.P("Values: []", prototypePackage.Ident("EnumValue"), "{")
@@ -238,11 +202,11 @@
}
// Generate literals for message descriptors.
- if len(f.fileReflect.allMessages) > 0 {
+ if len(f.allMessages) > 0 {
messageTypesVar := messageTypesVarName(f)
messageDescsVar := messageDescsVarName(f)
- g.P("var ", messageTypesVar, " = [", len(f.fileReflect.allMessages), "]", protoimplPackage.Ident("MessageType"), "{")
- for i, message := range f.fileReflect.allMessages {
+ g.P("var ", messageTypesVar, " = [", len(f.allMessages), "]", protoimplPackage.Ident("MessageType"), "{")
+ for i, message := range f.allMessages {
if message.Desc.IsMapEntry() {
// Map entry types have no Go type generated for them.
g.P("{ /* no message type for ", message.GoIdent, " */ },")
@@ -257,8 +221,8 @@
}
g.P("}")
- g.P("var ", messageDescsVar, " = [", len(f.fileReflect.allMessages), "]", prototypePackage.Ident("Message"), "{")
- for _, message := range f.fileReflect.allMessages {
+ g.P("var ", messageDescsVar, " = [", len(f.allMessages), "]", prototypePackage.Ident("Message"), "{")
+ for _, message := range f.allMessages {
g.P("{")
g.P("Name: ", strconv.Quote(string(message.Desc.Name())), ",")
if fields := message.Desc.Fields(); fields.Len() > 0 {
@@ -337,7 +301,7 @@
g.P("type ", shadowType, " ", enum.GoIdent)
g.P()
- idx := f.fileReflect.allEnumsByPtr[enum]
+ idx := f.allEnumsByPtr[enum]
typesVar := enumTypesVarName(f)
g.P("func (e ", enum.GoIdent, ") ProtoReflect() ", protoreflectPackage.Ident("Enum"), " {")
g.P("return (", shadowType, ")(e)")
@@ -359,7 +323,7 @@
g.P("type ", shadowType, " struct{m *", message.GoIdent, "}")
g.P()
- idx := f.fileReflect.allMessagesByPtr[message]
+ idx := f.allMessagesByPtr[message]
typesVar := messageTypesVarName(f)
g.P("func (m *", message.GoIdent, ") ProtoReflect() ", protoreflectPackage.Ident("Message"), " {")
g.P("return ", shadowType, "{m}")