From 575aebf635cba30f5545193609d80c647dce68ea Mon Sep 17 00:00:00 2001 From: Michael Stapelberg Date: Mon, 23 Dec 2024 13:18:00 +0100 Subject: [PATCH] internal/impl: revert IsSynthetic() check to fix panic This reverts change https://go.dev/cl/632735, in which I misunderstood what the Protobuf documentation wanted to convey: The quoted docs in CL 632735 refer to reflection for proto3 optional fields, not to reflection for proto3 synthetic oneofs. Synthetic oneofs should remain visible through reflection. For the Open API, this change restores the old behavior. For the Opaque API, another fix is needed and will be sent in a separate, follow-up CL (follow golang/protobuf#1659). For golang/protobuf#1659 Thanks to Joshua Humphries for the report and reproducer! Change-Id: I3a924eed6d2425581adc65651395a68fc1576f4d Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/638495 Reviewed-by: Chressie Himpel LUCI-TryBot-Result: Go LUCI --- internal/impl/message_reflect.go | 4 +--- proto/oneof_which_test.go | 22 ++++++++++++++++++++++ testing/prototest/message.go | 9 ++------- 3 files changed, 25 insertions(+), 10 deletions(-) diff --git a/internal/impl/message_reflect.go b/internal/impl/message_reflect.go index 1b9b16a4..31c19b54 100644 --- a/internal/impl/message_reflect.go +++ b/internal/impl/message_reflect.go @@ -85,9 +85,7 @@ func (mi *MessageInfo) makeKnownFieldsFunc(si structInfo) { mi.oneofs = map[protoreflect.Name]*oneofInfo{} for i := 0; i < md.Oneofs().Len(); i++ { od := md.Oneofs().Get(i) - if !od.IsSynthetic() { - mi.oneofs[od.Name()] = makeOneofInfo(od, si, mi.Exporter) - } + mi.oneofs[od.Name()] = makeOneofInfo(od, si, mi.Exporter) } mi.denseFields = make([]*fieldInfo, fds.Len()*2) diff --git a/proto/oneof_which_test.go b/proto/oneof_which_test.go index bdf10bdb..a53ae30d 100644 --- a/proto/oneof_which_test.go +++ b/proto/oneof_which_test.go @@ -7,6 +7,7 @@ package proto_test import ( "testing" + test3openpb "google.golang.org/protobuf/internal/testprotos/test3" testhybridpb "google.golang.org/protobuf/internal/testprotos/testeditions/testeditions_hybrid" testopaquepb "google.golang.org/protobuf/internal/testprotos/testeditions/testeditions_opaque" "google.golang.org/protobuf/proto" @@ -187,3 +188,24 @@ func TestOpaqueWhich(t *testing.T) { } } } + +func TestSyntheticOneof(t *testing.T) { + msg := test3openpb.TestAllTypes{} + md := msg.ProtoReflect().Descriptor() + ood := md.Oneofs().ByName("_optional_int32") + if ood == nil { + t.Fatal("failed to find oneof _optional_int32") + } + if !ood.IsSynthetic() { + t.Fatal("oneof _optional_int32 should be synthetic") + } + if msg.ProtoReflect().WhichOneof(ood) != nil { + t.Error("oneof _optional_int32 should not have a field set yet") + } + msg.OptionalInt32 = proto.Int32(123) + if msg.ProtoReflect().WhichOneof(ood) == nil { + t.Error("oneof _optional_int32 should have a field set") + } +} + +// TODO(stapelberg): add test variants for the Hybrid API and Opaque API. diff --git a/testing/prototest/message.go b/testing/prototest/message.go index def37bff..86682f2e 100644 --- a/testing/prototest/message.go +++ b/testing/prototest/message.go @@ -583,13 +583,8 @@ func testOneof(t testing.TB, m protoreflect.Message, od protoreflect.OneofDescri // Set fields explicitly. m.Set(fda, newValue(m, fda, 1, nil)) } - if !od.IsSynthetic() { - // Synthetic oneofs are used to represent optional fields in - // proto3. While they show up in protoreflect, WhichOneof does - // not work on these (only on non-synthetic, explicit oneofs). - if got, want := m.WhichOneof(od), fda; got != want { - t.Errorf("after setting oneof field %q:\nWhichOneof(%q) = %v, want %v", fda.FullName(), fda.Name(), got, want) - } + if got, want := m.WhichOneof(od), fda; got != want { + t.Errorf("after setting oneof field %q:\nWhichOneof(%q) = %v, want %v", fda.FullName(), fda.Name(), got, want) } for j := 0; j < od.Fields().Len(); j++ { fdb := od.Fields().Get(j)