Skip to content

Commit

Permalink
internal/impl: fix WhichOneof() to work with synthetic oneofs
Browse files Browse the repository at this point in the history 
This is the equivalent of CL 638495, but for the Opaque API.

While the behavior is the same for non-synthetic oneofs between
the Open Struct API and Opaque API, the behavior for synthetic oneofs
is not the same: Because the Opaque API uses a bitfield to store
presence, we need to use the Opaque presence check in WhichOneof().

This CL extends the testproto generator to use the protoc
command-line flag for the test3 testproto (which specifies
syntax = "proto3";), as the in-file api_level is only available
in .proto files using editions (but editions does not use synthetic
oneofs for optional fields, only proto3 does).

For golang/protobuf#1659

Change-Id: I0a1fd6e5fc6f96eeab043f966728ce2a14dbd446
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/638515
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Chressie Himpel <chressie@google.com>
  • Loading branch information
@stapelberg
stapelberg committed Dec 23, 2024
1 parent c0c814f commit 8878926
Show file tree
Hide file tree
Showing 13 changed files with 9,321 additions and 7 deletions.
34 changes: 32 additions & 2 deletions internal/cmd/generate-protos/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,13 @@ func generateOpaqueDotProto(repoRoot, tmpDir, relPath string) {
if matches := goPackageRe.FindStringSubmatch(line); matches != nil {
goPkg := matches[1]
hybridGoPkg := strings.TrimSuffix(goPkg, "/") + "/" + filepath.Base(goPkg) + "_hybrid"
return `option go_package = "` + hybridGoPkg + `";` + "\n" +
goPackage := `option go_package = "` + hybridGoPkg + `";` + "\n"
if strings.HasPrefix(relPath, "internal/testprotos/test3/") {
// The test3 testproto must remain on syntax = "proto3";
// and therefore cannot use the editions-only api_level.
return goPackage
}
return goPackage +
`import "google/protobuf/go_features.proto";` + "\n" +
`option features.(pb.go).api_level = API_HYBRID;`
}
Expand Down Expand Up @@ -211,7 +217,13 @@ func generateOpaqueDotProto(repoRoot, tmpDir, relPath string) {
if matches := goPackageRe.FindStringSubmatch(line); matches != nil {
goPkg := matches[1]
opaqueGoPkg := strings.TrimSuffix(goPkg, "/") + "/" + filepath.Base(goPkg) + "_opaque"
return `option go_package = "` + opaqueGoPkg + `";` + "\n" +
goPackage := `option go_package = "` + opaqueGoPkg + `";` + "\n"
if strings.HasPrefix(relPath, "internal/testprotos/test3/") {
// The test3 testproto must remain on syntax = "proto3";
// and therefore cannot use the editions-only api_level.
return goPackage
}
return goPackage +
`import "google/protobuf/go_features.proto";` + "\n" +
`option features.(pb.go).api_level = API_OPAQUE;`
}
Expand Down Expand Up @@ -248,6 +260,12 @@ func generateOpaqueTestprotos() {
}{
{path: "internal/testprotos/required"},
{path: "internal/testprotos/testeditions"},
{
path: "internal/testprotos/test3",
exclude: map[string]bool{
"internal/testprotos/test3/test_extension.proto": true,
},
},
{path: "internal/testprotos/enums"},
{path: "internal/testprotos/textpbeditions"},
{path: "internal/testprotos/messageset"},
Expand Down Expand Up @@ -359,6 +377,18 @@ func generateLocalProtos() {
if d.annotate[filepath.ToSlash(relPath)] {
opts += ",annotate_code"
}
if strings.HasPrefix(relPath, "internal/testprotos/test3/") {
variant := strings.TrimPrefix(relPath, "internal/testprotos/test3/")
if idx := strings.IndexByte(variant, '/'); idx > -1 {
variant = variant[:idx]
}
switch variant {
case "test3_hybrid":
opts += fmt.Sprintf(",apilevelM%v=%v", relPath, "API_HYBRID")
case "test3_opaque":
opts += fmt.Sprintf(",apilevelM%v=%v", relPath, "API_OPAQUE")
}
}
protoc("-I"+filepath.Join(repoRoot, "src"), "-I"+filepath.Join(protoRoot, "src"), "-I"+repoRoot, "--go_out="+opts+":"+tmpDir, filepath.Join(repoRoot, relPath))
return nil
})
Expand Down
24 changes: 21 additions & 3 deletions internal/impl/message_opaque.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,7 @@ func opaqueInitHook(mi *MessageInfo) bool {
mi.oneofs = map[protoreflect.Name]*oneofInfo{}
for i := 0; i < mi.Desc.Oneofs().Len(); i++ {
od := mi.Desc.Oneofs().Get(i)
if !od.IsSynthetic() {
mi.oneofs[od.Name()] = makeOneofInfo(od, si.structInfo, mi.Exporter)
}
mi.oneofs[od.Name()] = makeOneofInfoOpaque(mi, od, si.structInfo, mi.Exporter)
}

mi.denseFields = make([]*fieldInfo, fds.Len()*2)
Expand Down Expand Up @@ -119,6 +117,26 @@ func opaqueInitHook(mi *MessageInfo) bool {
return true
}

func makeOneofInfoOpaque(mi *MessageInfo, od protoreflect.OneofDescriptor, si structInfo, x exporter) *oneofInfo {
oi := &oneofInfo{oneofDesc: od}
if od.IsSynthetic() {
fd := od.Fields().Get(0)
index, _ := presenceIndex(mi.Desc, fd)
oi.which = func(p pointer) protoreflect.FieldNumber {
if p.IsNil() {
return 0
}
if !mi.present(p, index) {
return 0
}
return od.Fields().Get(0).Number()
}
return oi
}
// Dispatch to non-opaque oneof implementation for non-synthetic oneofs.
return makeOneofInfo(od, si, x)
}

func (mi *MessageInfo) fieldInfoForMapOpaque(si opaqueStructInfo, fd protoreflect.FieldDescriptor, fs reflect.StructField) fieldInfo {
ft := fs.Type
if ft.Kind() != reflect.Map {
Expand Down
2,762 changes: 2,762 additions & 0 deletions internal/testprotos/test3/test3_hybrid/test.hybrid.pb.go
View file

Large diffs are not rendered by default.

134 changes: 134 additions & 0 deletions internal/testprotos/test3/test3_hybrid/test.hybrid.proto
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
// Copyright 2018 The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.

syntax = "proto3";

package hybrid.goproto.proto.test3;

import "internal/testprotos/test3/test3_hybrid/test_import.hybrid.proto";

option go_package = "google.golang.org/protobuf/internal/testprotos/test3/test3_hybrid";


message TestAllTypes {
message NestedMessage {
int32 a = 1;
TestAllTypes corecursive = 2;
}

enum NestedEnum {
FOO = 0;
BAR = 1;
BAZ = 2;
NEG = -1; // Intentionally negative.
}

int32 singular_int32 = 81;
int64 singular_int64 = 82;
uint32 singular_uint32 = 83;
uint64 singular_uint64 = 84;
sint32 singular_sint32 = 85;
sint64 singular_sint64 = 86;
fixed32 singular_fixed32 = 87;
fixed64 singular_fixed64 = 88;
sfixed32 singular_sfixed32 = 89;
sfixed64 singular_sfixed64 = 90;
float singular_float = 91;
double singular_double = 92;
bool singular_bool = 93;
string singular_string = 94;
bytes singular_bytes = 95;
NestedMessage singular_nested_message = 98;
ForeignMessage singular_foreign_message = 99;
ImportMessage singular_import_message = 100;
NestedEnum singular_nested_enum = 101;
ForeignEnum singular_foreign_enum = 102;
ImportEnum singular_import_enum = 103;

optional int32 optional_int32 = 1;
optional int64 optional_int64 = 2;
optional uint32 optional_uint32 = 3;
optional uint64 optional_uint64 = 4;
optional sint32 optional_sint32 = 5;
optional sint64 optional_sint64 = 6;
optional fixed32 optional_fixed32 = 7;
optional fixed64 optional_fixed64 = 8;
optional sfixed32 optional_sfixed32 = 9;
optional sfixed64 optional_sfixed64 = 10;
optional float optional_float = 11;
optional double optional_double = 12;
optional bool optional_bool = 13;
optional string optional_string = 14;
optional bytes optional_bytes = 15;
optional NestedMessage optional_nested_message = 18;
optional ForeignMessage optional_foreign_message = 19;
optional ImportMessage optional_import_message = 20;
optional NestedEnum optional_nested_enum = 21;
optional ForeignEnum optional_foreign_enum = 22;
optional ImportEnum optional_import_enum = 23;

repeated int32 repeated_int32 = 31;
repeated int64 repeated_int64 = 32;
repeated uint32 repeated_uint32 = 33;
repeated uint64 repeated_uint64 = 34;
repeated sint32 repeated_sint32 = 35;
repeated sint64 repeated_sint64 = 36;
repeated fixed32 repeated_fixed32 = 37;
repeated fixed64 repeated_fixed64 = 38;
repeated sfixed32 repeated_sfixed32 = 39;
repeated sfixed64 repeated_sfixed64 = 40;
repeated float repeated_float = 41;
repeated double repeated_double = 42;
repeated bool repeated_bool = 43;
repeated string repeated_string = 44;
repeated bytes repeated_bytes = 45;
repeated NestedMessage repeated_nested_message = 48;
repeated ForeignMessage repeated_foreign_message = 49;
repeated ImportMessage repeated_importmessage = 50;
repeated NestedEnum repeated_nested_enum = 51;
repeated ForeignEnum repeated_foreign_enum = 52;
repeated ImportEnum repeated_importenum = 53;

map<int32, int32> map_int32_int32 = 56;
map<int64, int64> map_int64_int64 = 57;
map<uint32, uint32> map_uint32_uint32 = 58;
map<uint64, uint64> map_uint64_uint64 = 59;
map<sint32, sint32> map_sint32_sint32 = 60;
map<sint64, sint64> map_sint64_sint64 = 61;
map<fixed32, fixed32> map_fixed32_fixed32 = 62;
map<fixed64, fixed64> map_fixed64_fixed64 = 63;
map<sfixed32, sfixed32> map_sfixed32_sfixed32 = 64;
map<sfixed64, sfixed64> map_sfixed64_sfixed64 = 65;
map<int32, float> map_int32_float = 66;
map<int32, double> map_int32_double = 67;
map<bool, bool> map_bool_bool = 68;
map<string, string> map_string_string = 69;
map<string, bytes> map_string_bytes = 70;
map<string, NestedMessage> map_string_nested_message = 71;
map<string, NestedEnum> map_string_nested_enum = 73;

oneof oneof_field {
uint32 oneof_uint32 = 111;
NestedMessage oneof_nested_message = 112;
string oneof_string = 113;
bytes oneof_bytes = 114;
bool oneof_bool = 115;
uint64 oneof_uint64 = 116;
float oneof_float = 117;
double oneof_double = 118;
NestedEnum oneof_enum = 119;
}
}

message ForeignMessage {
int32 c = 1;
int32 d = 2;
}

enum ForeignEnum {
FOREIGN_ZERO = 0;
FOREIGN_FOO = 4;
FOREIGN_BAR = 5;
FOREIGN_BAZ = 6;
}
Loading

0 comments on commit 8878926

Please sign in to comment.