Skip to content

Commit 8878926

Browse files
stapelbergstapelberg
committed
internal/impl: fix WhichOneof() to work with synthetic oneofs
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 <[email protected]> Reviewed-by: Chressie Himpel <[email protected]>
1 parent c0c814f commit 8878926

13 files changed

+9321
-7
lines changed

‎internal/cmd/generate-protos/main.go

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,13 @@ func generateOpaqueDotProto(repoRoot, tmpDir, relPath string) {
182182
if matches := goPackageRe.FindStringSubmatch(line); matches != nil {
183183
goPkg := matches[1]
184184
hybridGoPkg := strings.TrimSuffix(goPkg, "/") + "/" + filepath.Base(goPkg) + "_hybrid"
185-
return `option go_package = "` + hybridGoPkg + `";` + "\n" +
185+
goPackage := `option go_package = "` + hybridGoPkg + `";` + "\n"
186+
if strings.HasPrefix(relPath, "internal/testprotos/test3/") {
187+
// The test3 testproto must remain on syntax = "proto3";
188+
// and therefore cannot use the editions-only api_level.
189+
return goPackage
190+
}
191+
return goPackage +
186192
`import "google/protobuf/go_features.proto";` + "\n" +
187193
`option features.(pb.go).api_level = API_HYBRID;`
188194
}
@@ -211,7 +217,13 @@ func generateOpaqueDotProto(repoRoot, tmpDir, relPath string) {
211217
if matches := goPackageRe.FindStringSubmatch(line); matches != nil {
212218
goPkg := matches[1]
213219
opaqueGoPkg := strings.TrimSuffix(goPkg, "/") + "/" + filepath.Base(goPkg) + "_opaque"
214-
return `option go_package = "` + opaqueGoPkg + `";` + "\n" +
220+
goPackage := `option go_package = "` + opaqueGoPkg + `";` + "\n"
221+
if strings.HasPrefix(relPath, "internal/testprotos/test3/") {
222+
// The test3 testproto must remain on syntax = "proto3";
223+
// and therefore cannot use the editions-only api_level.
224+
return goPackage
225+
}
226+
return goPackage +
215227
`import "google/protobuf/go_features.proto";` + "\n" +
216228
`option features.(pb.go).api_level = API_OPAQUE;`
217229
}
@@ -248,6 +260,12 @@ func generateOpaqueTestprotos() {
248260
}{
249261
{path: "internal/testprotos/required"},
250262
{path: "internal/testprotos/testeditions"},
263+
{
264+
path: "internal/testprotos/test3",
265+
exclude: map[string]bool{
266+
"internal/testprotos/test3/test_extension.proto": true,
267+
},
268+
},
251269
{path: "internal/testprotos/enums"},
252270
{path: "internal/testprotos/textpbeditions"},
253271
{path: "internal/testprotos/messageset"},
@@ -359,6 +377,18 @@ func generateLocalProtos() {
359377
if d.annotate[filepath.ToSlash(relPath)] {
360378
opts += ",annotate_code"
361379
}
380+
if strings.HasPrefix(relPath, "internal/testprotos/test3/") {
381+
variant := strings.TrimPrefix(relPath, "internal/testprotos/test3/")
382+
if idx := strings.IndexByte(variant, '/'); idx > -1 {
383+
variant = variant[:idx]
384+
}
385+
switch variant {
386+
case "test3_hybrid":
387+
opts += fmt.Sprintf(",apilevelM%v=%v", relPath, "API_HYBRID")
388+
case "test3_opaque":
389+
opts += fmt.Sprintf(",apilevelM%v=%v", relPath, "API_OPAQUE")
390+
}
391+
}
362392
protoc("-I"+filepath.Join(repoRoot, "src"), "-I"+filepath.Join(protoRoot, "src"), "-I"+repoRoot, "--go_out="+opts+":"+tmpDir, filepath.Join(repoRoot, relPath))
363393
return nil
364394
})

‎internal/impl/message_opaque.go

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -88,9 +88,7 @@ func opaqueInitHook(mi *MessageInfo) bool {
8888
mi.oneofs = map[protoreflect.Name]*oneofInfo{}
8989
for i := 0; i < mi.Desc.Oneofs().Len(); i++ {
9090
od := mi.Desc.Oneofs().Get(i)
91-
if !od.IsSynthetic() {
92-
mi.oneofs[od.Name()] = makeOneofInfo(od, si.structInfo, mi.Exporter)
93-
}
91+
mi.oneofs[od.Name()] = makeOneofInfoOpaque(mi, od, si.structInfo, mi.Exporter)
9492
}
9593

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

120+
func makeOneofInfoOpaque(mi *MessageInfo, od protoreflect.OneofDescriptor, si structInfo, x exporter) *oneofInfo {
121+
oi := &oneofInfo{oneofDesc: od}
122+
if od.IsSynthetic() {
123+
fd := od.Fields().Get(0)
124+
index, _ := presenceIndex(mi.Desc, fd)
125+
oi.which = func(p pointer) protoreflect.FieldNumber {
126+
if p.IsNil() {
127+
return 0
128+
}
129+
if !mi.present(p, index) {
130+
return 0
131+
}
132+
return od.Fields().Get(0).Number()
133+
}
134+
return oi
135+
}
136+
// Dispatch to non-opaque oneof implementation for non-synthetic oneofs.
137+
return makeOneofInfo(od, si, x)
138+
}
139+
122140
func (mi *MessageInfo) fieldInfoForMapOpaque(si opaqueStructInfo, fd protoreflect.FieldDescriptor, fs reflect.StructField) fieldInfo {
123141
ft := fs.Type
124142
if ft.Kind() != reflect.Map {

‎internal/testprotos/test3/test3_hybrid/test.hybrid.pb.go

Lines changed: 2762 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎internal/testprotos/test3/test3_hybrid/test.hybrid.proto

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,134 @@
1+
// Copyright 2018 The Go Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style
3+
// license that can be found in the LICENSE file.
4+
5+
syntax = "proto3";
6+
7+
package hybrid.goproto.proto.test3;
8+
9+
import "internal/testprotos/test3/test3_hybrid/test_import.hybrid.proto";
10+
11+
option go_package = "google.golang.org/protobuf/internal/testprotos/test3/test3_hybrid";
12+
13+
14+
message TestAllTypes {
15+
message NestedMessage {
16+
int32 a = 1;
17+
TestAllTypes corecursive = 2;
18+
}
19+
20+
enum NestedEnum {
21+
FOO = 0;
22+
BAR = 1;
23+
BAZ = 2;
24+
NEG = -1; // Intentionally negative.
25+
}
26+
27+
int32 singular_int32 = 81;
28+
int64 singular_int64 = 82;
29+
uint32 singular_uint32 = 83;
30+
uint64 singular_uint64 = 84;
31+
sint32 singular_sint32 = 85;
32+
sint64 singular_sint64 = 86;
33+
fixed32 singular_fixed32 = 87;
34+
fixed64 singular_fixed64 = 88;
35+
sfixed32 singular_sfixed32 = 89;
36+
sfixed64 singular_sfixed64 = 90;
37+
float singular_float = 91;
38+
double singular_double = 92;
39+
bool singular_bool = 93;
40+
string singular_string = 94;
41+
bytes singular_bytes = 95;
42+
NestedMessage singular_nested_message = 98;
43+
ForeignMessage singular_foreign_message = 99;
44+
ImportMessage singular_import_message = 100;
45+
NestedEnum singular_nested_enum = 101;
46+
ForeignEnum singular_foreign_enum = 102;
47+
ImportEnum singular_import_enum = 103;
48+
49+
optional int32 optional_int32 = 1;
50+
optional int64 optional_int64 = 2;
51+
optional uint32 optional_uint32 = 3;
52+
optional uint64 optional_uint64 = 4;
53+
optional sint32 optional_sint32 = 5;
54+
optional sint64 optional_sint64 = 6;
55+
optional fixed32 optional_fixed32 = 7;
56+
optional fixed64 optional_fixed64 = 8;
57+
optional sfixed32 optional_sfixed32 = 9;
58+
optional sfixed64 optional_sfixed64 = 10;
59+
optional float optional_float = 11;
60+
optional double optional_double = 12;
61+
optional bool optional_bool = 13;
62+
optional string optional_string = 14;
63+
optional bytes optional_bytes = 15;
64+
optional NestedMessage optional_nested_message = 18;
65+
optional ForeignMessage optional_foreign_message = 19;
66+
optional ImportMessage optional_import_message = 20;
67+
optional NestedEnum optional_nested_enum = 21;
68+
optional ForeignEnum optional_foreign_enum = 22;
69+
optional ImportEnum optional_import_enum = 23;
70+
71+
repeated int32 repeated_int32 = 31;
72+
repeated int64 repeated_int64 = 32;
73+
repeated uint32 repeated_uint32 = 33;
74+
repeated uint64 repeated_uint64 = 34;
75+
repeated sint32 repeated_sint32 = 35;
76+
repeated sint64 repeated_sint64 = 36;
77+
repeated fixed32 repeated_fixed32 = 37;
78+
repeated fixed64 repeated_fixed64 = 38;
79+
repeated sfixed32 repeated_sfixed32 = 39;
80+
repeated sfixed64 repeated_sfixed64 = 40;
81+
repeated float repeated_float = 41;
82+
repeated double repeated_double = 42;
83+
repeated bool repeated_bool = 43;
84+
repeated string repeated_string = 44;
85+
repeated bytes repeated_bytes = 45;
86+
repeated NestedMessage repeated_nested_message = 48;
87+
repeated ForeignMessage repeated_foreign_message = 49;
88+
repeated ImportMessage repeated_importmessage = 50;
89+
repeated NestedEnum repeated_nested_enum = 51;
90+
repeated ForeignEnum repeated_foreign_enum = 52;
91+
repeated ImportEnum repeated_importenum = 53;
92+
93+
map<int32, int32> map_int32_int32 = 56;
94+
map<int64, int64> map_int64_int64 = 57;
95+
map<uint32, uint32> map_uint32_uint32 = 58;
96+
map<uint64, uint64> map_uint64_uint64 = 59;
97+
map<sint32, sint32> map_sint32_sint32 = 60;
98+
map<sint64, sint64> map_sint64_sint64 = 61;
99+
map<fixed32, fixed32> map_fixed32_fixed32 = 62;
100+
map<fixed64, fixed64> map_fixed64_fixed64 = 63;
101+
map<sfixed32, sfixed32> map_sfixed32_sfixed32 = 64;
102+
map<sfixed64, sfixed64> map_sfixed64_sfixed64 = 65;
103+
map<int32, float> map_int32_float = 66;
104+
map<int32, double> map_int32_double = 67;
105+
map<bool, bool> map_bool_bool = 68;
106+
map<string, string> map_string_string = 69;
107+
map<string, bytes> map_string_bytes = 70;
108+
map<string, NestedMessage> map_string_nested_message = 71;
109+
map<string, NestedEnum> map_string_nested_enum = 73;
110+
111+
oneof oneof_field {
112+
uint32 oneof_uint32 = 111;
113+
NestedMessage oneof_nested_message = 112;
114+
string oneof_string = 113;
115+
bytes oneof_bytes = 114;
116+
bool oneof_bool = 115;
117+
uint64 oneof_uint64 = 116;
118+
float oneof_float = 117;
119+
double oneof_double = 118;
120+
NestedEnum oneof_enum = 119;
121+
}
122+
}
123+
124+
message ForeignMessage {
125+
int32 c = 1;
126+
int32 d = 2;
127+
}
128+
129+
enum ForeignEnum {
130+
FOREIGN_ZERO = 0;
131+
FOREIGN_FOO = 4;
132+
FOREIGN_BAR = 5;
133+
FOREIGN_BAZ = 6;
134+
}

0 commit comments

Comments
 (0)