Skip to content

Commit

Permalink
internal/impl: skip synthetic oneofs in messageInfo
Browse files Browse the repository at this point in the history
Calling WhichOneof should not be possible for synthetic oneofs;
on the reflection level, these fields should work as if they
were regular fields, not as if they were oneofs:

> Reflection for proto3 optional fields should work properly. For example, a
> method like Reflection::HasField() should know to look for the hasbit for a
> proto3 optional field. It should not be fooled by the synthetic oneof into
> thinking that there is a case member for the oneof.

From the Protobuf docs at:
https://github.com/protocolbuffers/protobuf/blob/main/docs/implementing_proto3_presence.md#updating-reflection

This change was tested Google-internally as CL 701866153.

Change-Id: Id9500d4aa492acf4ebc6d2d84be07ed81201d2aa
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/632735
Reviewed-by: Chressie Himpel <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
  • Loading branch information
stapelberg committed Dec 2, 2024
1 parent 30f628e commit bdcc7ad
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 3 deletions.
4 changes: 3 additions & 1 deletion internal/impl/message_reflect.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,9 @@ 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)
mi.oneofs[od.Name()] = makeOneofInfo(od, si, mi.Exporter)
if !od.IsSynthetic() {
mi.oneofs[od.Name()] = makeOneofInfo(od, si, mi.Exporter)
}
}

mi.denseFields = make([]*fieldInfo, fds.Len()*2)
Expand Down
9 changes: 7 additions & 2 deletions testing/prototest/message.go
Original file line number Diff line number Diff line change
Expand Up @@ -579,8 +579,13 @@ func testOneof(t testing.TB, m protoreflect.Message, od protoreflect.OneofDescri
// Set fields explicitly.
m.Set(fda, newValue(m, fda, 1, nil))
}
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 !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)
}
}
for j := 0; j < od.Fields().Len(); j++ {
fdb := od.Fields().Get(j)
Expand Down

0 comments on commit bdcc7ad

Please sign in to comment.