protogen: fix oneof name mangling regression
The generator currently uses an unintuitive and stateful algorithm
for name generation where it "fixes" name conflicts by appending "_"
to the end of the new name.
PR#657 refactored the generator code and noticed that the above
algorithm was not properly taking into account that a Get method is
generated for parent oneofs, fixing it in the same PR. While this is
more correct, this breaks users (see #780) since it means that the
generation of names can change.
This PR changes the name mangling logic to be as it was previously.
This does mean that some new proto files may be unbuildable,
but that is arguably better than breaking existing proto files
Change-Id: I2e354f4bb5d9c2b562fa2faa9149e949e2d86a0f
Reviewed-on: https://go-review.googlesource.com/c/156877
Reviewed-by: Damien Neil <dneil@google.com>
diff --git a/cmd/protoc-gen-go/testdata/issue780_oneof_conflict/test.pb.go b/cmd/protoc-gen-go/testdata/issue780_oneof_conflict/test.pb.go
new file mode 100644
index 0000000..89cd93f
--- /dev/null
+++ b/cmd/protoc-gen-go/testdata/issue780_oneof_conflict/test.pb.go
@@ -0,0 +1,162 @@
+// Code generated by protoc-gen-go. DO NOT EDIT.
+// source: issue780_oneof_conflict/test.proto
+
+package oneoftest
+
+import (
+ proto "github.com/golang/protobuf/proto"
+ protoreflect "github.com/golang/protobuf/v2/reflect/protoreflect"
+ prototype "github.com/golang/protobuf/v2/reflect/prototype"
+ protoimpl "github.com/golang/protobuf/v2/runtime/protoimpl"
+)
+
+// This is a compile-time assertion to ensure that this generated file
+// is compatible with the proto package it is being compiled against.
+// A compilation error at this line likely means your copy of the
+// proto package needs to be updated.
+const _ = proto.ProtoPackageIsVersion3 // please upgrade the proto package
+
+type Foo struct {
+ // Types that are valid to be assigned to Bar:
+ // *Foo_GetBar
+ Bar isFoo_Bar `protobuf_oneof:"bar"`
+ XXX_NoUnkeyedLiteral struct{} `json:"-"`
+ XXX_unrecognized []byte `json:"-"`
+ XXX_sizecache int32 `json:"-"`
+}
+
+type xxx_Foo struct{ m *Foo }
+
+func (m *Foo) ProtoReflect() protoreflect.Message {
+ return xxx_Foo{m}
+}
+func (m xxx_Foo) Type() protoreflect.MessageType {
+ return xxx_Test_ProtoFile_MessageTypes[0].Type
+}
+func (m xxx_Foo) KnownFields() protoreflect.KnownFields {
+ return xxx_Test_ProtoFile_MessageTypes[0].KnownFieldsOf(m.m)
+}
+func (m xxx_Foo) UnknownFields() protoreflect.UnknownFields {
+ return xxx_Test_ProtoFile_MessageTypes[0].UnknownFieldsOf(m.m)
+}
+func (m xxx_Foo) Interface() protoreflect.ProtoMessage {
+ return m.m
+}
+
+func (m *Foo) Reset() { *m = Foo{} }
+func (m *Foo) String() string { return proto.CompactTextString(m) }
+func (*Foo) ProtoMessage() {}
+func (*Foo) Descriptor() ([]byte, []int) {
+ return fileDescriptor_48462cafc802a68e, []int{0}
+}
+
+func (m *Foo) XXX_Unmarshal(b []byte) error {
+ return xxx_messageInfo_Foo.Unmarshal(m, b)
+}
+func (m *Foo) XXX_Marshal(b []byte, deterministic bool) ([]byte, error) {
+ return xxx_messageInfo_Foo.Marshal(b, m, deterministic)
+}
+func (m *Foo) XXX_Merge(src proto.Message) {
+ xxx_messageInfo_Foo.Merge(m, src)
+}
+func (m *Foo) XXX_Size() int {
+ return xxx_messageInfo_Foo.Size(m)
+}
+func (m *Foo) XXX_DiscardUnknown() {
+ xxx_messageInfo_Foo.DiscardUnknown(m)
+}
+
+var xxx_messageInfo_Foo proto.InternalMessageInfo
+
+type isFoo_Bar interface {
+ isFoo_Bar()
+}
+
+type Foo_GetBar struct {
+ GetBar string `protobuf:"bytes,1,opt,name=get_bar,json=getBar,oneof"`
+}
+
+func (*Foo_GetBar) isFoo_Bar() {}
+
+func (m *Foo) GetBar() isFoo_Bar {
+ if m != nil {
+ return m.Bar
+ }
+ return nil
+}
+
+func (m *Foo) GetGetBar() string {
+ if x, ok := m.GetBar().(*Foo_GetBar); ok {
+ return x.GetBar
+ }
+ return ""
+}
+
+// XXX_OneofWrappers is for the internal use of the proto package.
+func (*Foo) XXX_OneofWrappers() []interface{} {
+ return []interface{}{
+ (*Foo_GetBar)(nil),
+ }
+}
+
+func init() {
+ proto.RegisterFile("issue780_oneof_conflict/test.proto", fileDescriptor_48462cafc802a68e)
+ proto.RegisterType((*Foo)(nil), "oneoftest.Foo")
+}
+
+var fileDescriptor_48462cafc802a68e = []byte{
+ // 107 bytes of a gzipped FileDescriptorProto
+ 0x1f, 0x8b, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0xff, 0xe2, 0x52, 0xca, 0x2c, 0x2e, 0x2e,
+ 0x4d, 0x35, 0xb7, 0x30, 0x88, 0xcf, 0xcf, 0x4b, 0xcd, 0x4f, 0x8b, 0x4f, 0xce, 0xcf, 0x4b, 0xcb,
+ 0xc9, 0x4c, 0x2e, 0xd1, 0x2f, 0x49, 0x2d, 0x2e, 0xd1, 0x2b, 0x28, 0xca, 0x2f, 0xc9, 0x17, 0xe2,
+ 0x04, 0x4b, 0x81, 0x04, 0x94, 0xd4, 0xb9, 0x98, 0xdd, 0xf2, 0xf3, 0x85, 0x24, 0xb9, 0xd8, 0xd3,
+ 0x53, 0x4b, 0xe2, 0x93, 0x12, 0x8b, 0x24, 0x18, 0x15, 0x18, 0x35, 0x38, 0x3d, 0x18, 0x82, 0xd8,
+ 0xd2, 0x53, 0x4b, 0x9c, 0x12, 0x8b, 0x9c, 0x58, 0xb9, 0x98, 0x93, 0x12, 0x8b, 0x00, 0x01, 0x00,
+ 0x00, 0xff, 0xff, 0x12, 0x66, 0x0c, 0x02, 0x58, 0x00, 0x00, 0x00,
+}
+
+func init() {
+ xxx_Test_ProtoFile_FileDesc.Messages = xxx_Test_ProtoFile_MessageDescs[0:1]
+ var err error
+ Test_ProtoFile, err = prototype.NewFile(&xxx_Test_ProtoFile_FileDesc)
+ if err != nil {
+ panic(err)
+ }
+}
+
+const _ = protoimpl.EnforceVersion(protoimpl.Version - 0)
+
+var Test_ProtoFile protoreflect.FileDescriptor
+
+var xxx_Test_ProtoFile_FileDesc = prototype.File{
+ Syntax: protoreflect.Proto2,
+ Path: "issue780_oneof_conflict/test.proto",
+ Package: "oneoftest",
+}
+var xxx_Test_ProtoFile_MessageTypes = [1]protoimpl.MessageType{
+ {Type: prototype.GoMessage(
+ xxx_Test_ProtoFile_MessageDescs[0].Reference(),
+ func(protoreflect.MessageType) protoreflect.ProtoMessage {
+ return new(Foo)
+ },
+ )},
+}
+var xxx_Test_ProtoFile_MessageDescs = [1]prototype.Message{
+ {
+ Name: "Foo",
+ Fields: []prototype.Field{
+ {
+ Name: "get_bar",
+ Number: 1,
+ Cardinality: protoreflect.Optional,
+ Kind: protoreflect.StringKind,
+ JSONName: "getBar",
+ OneofName: "bar",
+ IsPacked: prototype.False,
+ },
+ },
+ Oneofs: []prototype.Oneof{
+ {Name: "bar"},
+ },
+ },
+}
diff --git a/cmd/protoc-gen-go/testdata/issue780_oneof_conflict/test.proto b/cmd/protoc-gen-go/testdata/issue780_oneof_conflict/test.proto
new file mode 100644
index 0000000..4f93027
--- /dev/null
+++ b/cmd/protoc-gen-go/testdata/issue780_oneof_conflict/test.proto
@@ -0,0 +1,9 @@
+syntax = "proto2";
+
+package oneoftest;
+
+message Foo {
+ oneof bar { // must be generated as Bar field in Foo struct
+ string get_bar = 1;
+ }
+}
diff --git a/protogen/protogen.go b/protogen/protogen.go
index 76964eb..9c279b8 100644
--- a/protogen/protogen.go
+++ b/protogen/protogen.go
@@ -569,22 +569,22 @@
"ExtensionMap": true,
"Descriptor": true,
}
- makeNameUnique := func(name string) string {
- for usedNames[name] || usedNames["Get"+name] {
+ makeNameUnique := func(name string, hasGetter bool) string {
+ for usedNames[name] || (hasGetter && usedNames["Get"+name]) {
name += "_"
}
usedNames[name] = true
- usedNames["Get"+name] = true
+ usedNames["Get"+name] = hasGetter
return name
}
seenOneofs := make(map[int]bool)
for _, field := range message.Fields {
- field.GoName = makeNameUnique(field.GoName)
+ field.GoName = makeNameUnique(field.GoName, true)
if field.OneofType != nil {
if !seenOneofs[field.OneofType.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.OneofType.GoName = makeNameUnique(field.OneofType.GoName)
+ field.OneofType.GoName = makeNameUnique(field.OneofType.GoName, false)
seenOneofs[field.OneofType.Desc.Index()] = true
}
}
