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 <dneil@google.com>

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 <proto_package>.<go_ident>. 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(field.GoIdent.GoName, field.Location)
-			g.Annotate(name.GoName, field.Location)
+			g.Annotate(field.GoIdent.GoName+"."+field.GoName, field.Location)
-			g.Annotate(name.GoName+"."+field.GoName, field.Location)
+			g.P("type ", field.GoIdent, " struct {")
-			g.P("type ", name, " 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)
+	return "is" + oneof.GoIdent.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
-	}
 }
 
 var jsonIgnoreTags = structTags{{"json", "-"}}

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("}")

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)
-		if field.Oneof != nil {
+		field.GoIdent.GoName = message.GoIdent.GoName + "_" + field.GoName
-			if !seenOneofs[field.Oneof.Desc.Index()] {
+		if field.Oneof != nil && field.Oneof.Fields[0] == field {
-				// If this is a field in a oneof that we haven't seen before,
+			// Make the name for a oneof unique as well. For historical reasons,
-				// make the name for that oneof unique as well.
+			// 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)
-				seenOneofs[field.Oneof.Desc.Index()] = true
+			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 {
+		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())),
+		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())),
+		GoName: camelCased,
+		GoIdent: GoIdent{
+			GoImportPath: f.GoImportPath,
+			GoName:       parentPrefix + camelCased,
+		},
 		Location: loc,
 		Comments: f.comments[newPathKey(loc.Path)],
 	}