From ef6e524dca640df23a3003db76fcab5789ea9470 Mon Sep 17 00:00:00 2001 From: Joe Tsai Date: Wed, 21 Aug 2019 00:55:36 -0700 Subject: [PATCH] compiler/protogen: move name mangling logic to protogen The name mangling logic should be unified in a single place rather than being split between compiler/protogen and cmd/protoc-gen-go. Move it over compiler/protogen. Change-Id: Iea65e0b3fba45e0c95c76e3fc1f061e0fa8f84d7 Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/191117 Reviewed-by: Damien Neil --- cmd/protoc-gen-go/internal_gengo/main.go | 81 ++++----------------- cmd/protoc-gen-go/internal_gengo/reflect.go | 2 +- compiler/protogen/protogen.go | 79 ++++++++++++++++---- 3 files changed, 82 insertions(+), 80 deletions(-) diff --git a/cmd/protoc-gen-go/internal_gengo/main.go b/cmd/protoc-gen-go/internal_gengo/main.go index 2a200a66..0bb1f4f9 100644 --- a/cmd/protoc-gen-go/internal_gengo/main.go +++ b/cmd/protoc-gen-go/internal_gengo/main.go @@ -402,19 +402,6 @@ func genEnum(gen *protogen.Plugin, g *protogen.GeneratedFile, f *fileInfo, enum } } -// enumLegacyName returns the name used by the v1 proto package. -// -// Confusingly, this is .. This probably should have -// been the full name of the proto enum type instead, but changing it at this -// point would require thought. -func enumLegacyName(enum *protogen.Enum) string { - fdesc := enum.Desc.ParentFile() - if fdesc.Package() == "" { - return enum.GoIdent.GoName - } - return string(fdesc.Package()) + "." + enum.GoIdent.GoName -} - func genMessage(gen *protogen.Plugin, g *protogen.GeneratedFile, f *fileInfo, message *protogen.Message) { if message.Desc.IsMapEntry() { return @@ -501,7 +488,7 @@ func genMessageField(g *protogen.GeneratedFile, f *fileInfo, message *protogen.M } ss := []string{fmt.Sprintf(" Types that are assignable to %s:\n", oneof.GoName)} for _, field := range oneof.Fields { - ss = append(ss, "\t*"+fieldOneofType(field).GoName+"\n") + ss = append(ss, "\t*"+field.GoIdent.GoName+"\n") } leadingComments += protogen.Comments(strings.Join(ss, "")) g.P(leadingComments, @@ -691,7 +678,7 @@ func genMessageGetterMethods(gen *protogen.Plugin, g *protogen.GeneratedFile, f g.P("}") case field.Oneof != nil: g.P(leadingComments, "func (x *", message.GoIdent, ") Get", field.GoName, "() ", goType, " {") - g.P("if x, ok := x.Get", field.Oneof.GoName, "().(*", fieldOneofType(field), "); ok {") + g.P("if x, ok := x.Get", field.Oneof.GoName, "().(*", field.GoIdent, "); ok {") g.P("return x.", field.GoName) g.P("}") g.P("return ", defaultValue) @@ -793,7 +780,14 @@ func fieldGoType(g *protogen.GeneratedFile, f *fileInfo, field *protogen.Field) func fieldProtobufTagValue(field *protogen.Field) string { var enumName string if field.Desc.Kind() == protoreflect.EnumKind { - enumName = enumLegacyName(field.Enum) + // For historical reasons, the name used in the tag is neither + // the protobuf full name nor the fully qualified Go identifier, + // but an odd mix of both. + enumName = field.Enum.GoIdent.GoName + protoPkg := string(field.Enum.Desc.ParentFile().Package()) + if protoPkg != "" { + enumName = protoPkg + "." + enumName + } } return tag.Marshal(field.Desc, enumName) } @@ -891,7 +885,7 @@ func genExtensions(gen *protogen.Plugin, g *protogen.GeneratedFile, f *fileInfo) leadingComments = appendDeprecationSuffix(leadingComments, extension.Desc.Options().(*descriptorpb.FieldOptions).GetDeprecated()) g.P(leadingComments, - extensionVar(f.File, extension), " = &", extensionTypesVarName(f), "[", allExtensionsByPtr[extension], "]", + "E_", extension.GoIdent, " = &", extensionTypesVarName(f), "[", allExtensionsByPtr[extension], "]", trailingComment(extension.Comments.Trailing)) } g.P(")") @@ -899,16 +893,6 @@ func genExtensions(gen *protogen.Plugin, g *protogen.GeneratedFile, f *fileInfo) } } -// extensionVar returns the var holding the ExtensionDesc for an extension. -func extensionVar(f *protogen.File, extension *protogen.Extension) protogen.GoIdent { - name := "E_" - if extension.Parent != nil { - name += extension.Parent.GoIdent.GoName + "_" - } - name += extension.GoName - return f.GoImportPath.Ident(name) -} - // genOneofWrapperTypes generates the oneof wrapper types and // associates the types with the parent message type. func genOneofWrapperTypes(gen *protogen.Plugin, g *protogen.GeneratedFile, f *fileInfo, message *protogen.Message) { @@ -919,10 +903,9 @@ func genOneofWrapperTypes(gen *protogen.Plugin, g *protogen.GeneratedFile, f *fi g.P("}") g.P() for _, field := range oneof.Fields { - name := fieldOneofType(field) - g.Annotate(name.GoName, field.Location) - g.Annotate(name.GoName+"."+field.GoName, field.Location) - g.P("type ", name, " struct {") + g.Annotate(field.GoIdent.GoName, field.Location) + g.Annotate(field.GoIdent.GoName+"."+field.GoName, field.Location) + g.P("type ", field.GoIdent, " struct {") goType, _ := fieldGoType(g, f, field) tags := structTags{ {"protobuf", fieldProtobufTagValue(field)}, @@ -936,7 +919,7 @@ func genOneofWrapperTypes(gen *protogen.Plugin, g *protogen.GeneratedFile, f *fi g.P() } for _, field := range oneof.Fields { - g.P("func (*", fieldOneofType(field), ") ", ifName, "() {}") + g.P("func (*", field.GoIdent, ") ", ifName, "() {}") g.P() } } @@ -945,39 +928,7 @@ func genOneofWrapperTypes(gen *protogen.Plugin, g *protogen.GeneratedFile, f *fi // oneofInterfaceName returns the name of the interface type implemented by // the oneof field value types. func oneofInterfaceName(oneof *protogen.Oneof) string { - return fmt.Sprintf("is%s_%s", oneof.Parent.GoIdent.GoName, oneof.GoName) -} - -// fieldOneofType returns the wrapper type used to represent a field in a oneof. -func fieldOneofType(field *protogen.Field) protogen.GoIdent { - ident := protogen.GoIdent{ - GoImportPath: field.Parent.GoIdent.GoImportPath, - GoName: field.Parent.GoIdent.GoName + "_" + field.GoName, - } - // Check for collisions with nested messages or enums. - // - // This conflict resolution is incomplete: Among other things, it - // does not consider collisions with other oneof field types. - // - // TODO: Consider dropping this entirely. Detecting conflicts and - // producing an error is almost certainly better than permuting - // field and type names in mostly unpredictable ways. -Loop: - for { - for _, message := range field.Parent.Messages { - if message.GoIdent == ident { - ident.GoName += "_" - continue Loop - } - } - for _, enum := range field.Parent.Enums { - if enum.GoIdent == ident { - ident.GoName += "_" - continue Loop - } - } - return ident - } + return "is" + oneof.GoIdent.GoName } var jsonIgnoreTags = structTags{{"json", "-"}} diff --git a/cmd/protoc-gen-go/internal_gengo/reflect.go b/cmd/protoc-gen-go/internal_gengo/reflect.go index 63bbbd08..c0f2dfd0 100644 --- a/cmd/protoc-gen-go/internal_gengo/reflect.go +++ b/cmd/protoc-gen-go/internal_gengo/reflect.go @@ -192,7 +192,7 @@ func genReflectFileDescriptor(gen *protogen.Plugin, g *protogen.GeneratedFile, f g.P(typesVar, "[", idx, "].OneofWrappers = []interface{} {") for _, oneof := range message.Oneofs { for _, field := range oneof.Fields { - g.P("(*", fieldOneofType(field), ")(nil),") + g.P("(*", field.GoIdent, ")(nil),") } } g.P("}") diff --git a/compiler/protogen/protogen.go b/compiler/protogen/protogen.go index 0620d3ab..ec037049 100644 --- a/compiler/protogen/protogen.go +++ b/compiler/protogen/protogen.go @@ -556,7 +556,7 @@ func newEnumValue(gen *Plugin, f *File, message *Message, enum *Enum, desc proto // A top-level enum value's name is: EnumName_ValueName // An enum value contained in a message is: MessageName_ValueName // - // Enum value names are not camelcased. + // For historical reasons, enum value names are not camel-cased. parentIdent := enum.GoIdent if message != nil { parentIdent = message.GoIdent @@ -661,15 +661,39 @@ func newMessage(gen *Plugin, f *File, parent *Message, desc protoreflect.Message usedNames["Get"+name] = hasGetter return name } - seenOneofs := make(map[int]bool) for _, field := range message.Fields { field.GoName = makeNameUnique(field.GoName, true) + field.GoIdent.GoName = message.GoIdent.GoName + "_" + field.GoName + if field.Oneof != nil && field.Oneof.Fields[0] == field { + // Make the name for a oneof unique as well. For historical reasons, + // this assumes that a getter method is not generated for oneofs. + // This is incorrect, but fixing it breaks existing code. + field.Oneof.GoName = makeNameUnique(field.Oneof.GoName, false) + field.Oneof.GoIdent.GoName = message.GoIdent.GoName + "_" + field.Oneof.GoName + } + } + + // Oneof field name conflict resolution. + // + // This conflict resolution is incomplete as it does not consider collisions + // with other oneof field types, but fixing it breaks existing code. + for _, field := range message.Fields { if field.Oneof != nil { - if !seenOneofs[field.Oneof.Desc.Index()] { - // If this is a field in a oneof that we haven't seen before, - // make the name for that oneof unique as well. - field.Oneof.GoName = makeNameUnique(field.Oneof.GoName, false) - seenOneofs[field.Oneof.Desc.Index()] = true + Loop: + for { + for _, nestedMessage := range message.Messages { + if nestedMessage.GoIdent == field.GoIdent { + field.GoIdent.GoName += "_" + continue Loop + } + } + for _, nestedEnum := range message.Enums { + if nestedEnum.GoIdent == field.GoIdent { + field.GoIdent.GoName += "_" + continue Loop + } + } + break Loop } } } @@ -703,7 +727,13 @@ type Field struct { // GoName is the base name of this field's Go field and methods. // For code generated by protoc-gen-go, this means a field named // '{{GoName}}' and a getter method named 'Get{{GoName}}'. - GoName string + GoName string // e.g., "FieldName" + + // GoIdent is the base name of a top-level declaration for this field. + // For code generated by protoc-gen-go, this means a wrapper type named + // '{{GoIdent}}' for members fields of a oneof, and a variable named + // 'E_{{GoIdent}}' for extension fields. + GoIdent GoIdent // e.g., "MessageName_FieldName" Parent *Message // message in which this field is declared; nil if top-level extension Oneof *Oneof // containing oneof; nil if not part of a oneof @@ -726,9 +756,18 @@ func newField(gen *Plugin, f *File, message *Message, desc protoreflect.FieldDes default: loc = message.Location.appendPath(fieldnum.DescriptorProto_Field, int32(desc.Index())) } + camelCased := camelCase(string(desc.Name())) + var parentPrefix string + if message != nil { + parentPrefix = message.GoIdent.GoName + "_" + } field := &Field{ - Desc: desc, - GoName: camelCase(string(desc.Name())), + Desc: desc, + GoName: camelCased, + GoIdent: GoIdent{ + GoImportPath: f.GoImportPath, + GoName: parentPrefix + camelCased, + }, Parent: message, Location: loc, Comments: f.comments[newPathKey(loc.Path)], @@ -769,7 +808,13 @@ func (field *Field) resolveDependencies(gen *Plugin) error { type Oneof struct { Desc protoreflect.OneofDescriptor - GoName string // Go field name of this oneof + // GoName is the base name of this oneof's Go field and methods. + // For code generated by protoc-gen-go, this means a field named + // '{{GoName}}' and a getter method named 'Get{{GoName}}'. + GoName string // e.g., "OneofName" + + // GoIdent is the base name of a top-level declaration for this oneof. + GoIdent GoIdent // e.g., "MessageName_OneofName" Parent *Message // message in which this oneof is declared @@ -781,10 +826,16 @@ type Oneof struct { func newOneof(gen *Plugin, f *File, message *Message, desc protoreflect.OneofDescriptor) *Oneof { loc := message.Location.appendPath(fieldnum.DescriptorProto_OneofDecl, int32(desc.Index())) + camelCased := camelCase(string(desc.Name())) + parentPrefix := message.GoIdent.GoName + "_" return &Oneof{ - Desc: desc, - Parent: message, - GoName: camelCase(string(desc.Name())), + Desc: desc, + Parent: message, + GoName: camelCased, + GoIdent: GoIdent{ + GoImportPath: f.GoImportPath, + GoName: parentPrefix + camelCased, + }, Location: loc, Comments: f.comments[newPathKey(loc.Path)], }