Skip to content

Commit 5a1e7ce

Browse files
stapelbergcopybara-github
stapelberg
authored and
copybara-github
committed
No public description
PiperOrigin-RevId: 799008780 Change-Id: I275c7c9605c0b42d586a4f2a8516849a536ea90a
1 parent 87af3f7 commit 5a1e7ce

File tree

4 files changed

+48
-41
lines changed

4 files changed

+48
-41
lines changed

‎internal/fix/build.go‎

Lines changed: 20 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -197,29 +197,8 @@ func updateBuilderElements(c *cursor, lit *dst.CompositeLit) (ok bool) {
197197
continue
198198
}
199199

200-
// Don't rewrite the oneof in
201-
//
202-
// &pb.M2{OneofField: &pb.M2_MsgOneof{}}
203-
// &pb.M2{OneofField: &pb.M2_EnumOneof{}}
204-
//
205-
// It's a tricky case and should be very rare because "oneof
206-
// with a type but without a value" is not a valid protocol buffers
207-
// concept.
208-
//
209-
// It happens due to a mismatch between what's allowed in protocol buffers
210-
// and the Go structs representing protocol buffers in the open API.
211-
// This information will not be marshalled to the wire and the field
212-
// is effectively discarded during marshalling when it does not have
213-
// a value.
214-
//
215-
// The opaque API does not allow this. While the struct can technically
216-
// represent this, you cannot use the API to bring the struct into
217-
// this state.
218-
//
219-
// For enums: this should be rare and we don't want to guess the default
220-
// value.
221-
if fieldValue == nil && !isBasic(fieldType) {
222-
c.Logf("returning: RHS is nil of Type %T (looking for types.Basic)", fieldType)
200+
if fieldValue == nil && !isBasic(fieldType) && !isPtr(fieldType) && !isEnum(fieldType) {
201+
c.Logf("returning: RHS is nil of Type %T (looking for types.Basic or types.Pointer)", fieldType)
223202
return false
224203
}
225204

@@ -235,11 +214,27 @@ func updateBuilderElements(c *cursor, lit *dst.CompositeLit) (ok bool) {
235214
unsafeRewrite = true
236215
}
237216

238-
// Handle `M{Oneof: OneofBasicField{}}`
217+
// Handle `M{Oneof: OneofBasicField{}}`, `M{Oneof: OneofMsgField{}}` or
218+
// `M{Oneof: OneofEnumField{}}`
239219
if fieldValue == nil {
240220
updates = append(updates, func() {
241221
kv.Key.(*dst.Ident).Name = fieldName // Rename the key to field name from the oneof wrapper.
242-
kv.Value = c.newProtoHelperCall(nil, types.Unalias(fieldType).(*types.Basic))
222+
if isBasic(fieldType) {
223+
kv.Value = c.newProtoHelperCall(nil, types.Unalias(fieldType).(*types.Basic))
224+
} else if isPtr(fieldType) {
225+
lit := &dst.CompositeLit{
226+
Type: c.selectorForProtoMessageType(fieldType),
227+
}
228+
c.setType(lit, fieldType)
229+
kv.Value = &dst.UnaryExpr{
230+
Op: token.AND,
231+
X: lit,
232+
}
233+
c.setType(kv.Value, fieldType)
234+
} else { /* must be isEnum(fieldType) */
235+
kv.Value = &dst.Ident{Name: "0"}
236+
c.setType(kv.Value, types.Typ[types.Invalid])
237+
}
243238
})
244239
c.Logf("generated RHS for field %v", fieldName)
245240
continue

‎internal/fix/build_test.go‎

Lines changed: 23 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -451,6 +451,7 @@ _ = &pb2.M2{
451451
_ = pb2.M2_builder{
452452
// comment before
453453
BytesOneof:/*comment1*/ m2.GetBytesOneof(), // eol comment
454+
EmptyOneof: m2.GetEmptyOneof(),
454455
EnumOneof: proto.ValueOrNil(m2.HasEnumOneof(), m2.GetEnumOneof),
455456
IntOneof: proto.ValueOrNil(m2.HasIntOneof(), m2.GetIntOneof),
456457
MsgOneof: m2.GetMsgOneof(),
@@ -461,6 +462,7 @@ _ = pb2.M2_builder{
461462
_ = pb2.M2_builder{
462463
// comment before
463464
BytesOneof:/*comment1*/ m2.GetBytesOneof(), // eol comment
465+
EmptyOneof: m2.GetEmptyOneof(),
464466
EnumOneof: proto.ValueOrNil(m2.HasEnumOneof(), m2.GetEnumOneof),
465467
IntOneof: proto.ValueOrNil(m2.HasIntOneof(), m2.GetIntOneof),
466468
MsgOneof: m2.GetMsgOneof(),
@@ -522,6 +524,7 @@ _ = &pb2.M2{OneofField: ifaceOneof}
522524
_ = pb2.M2_builder{
523525
S: proto.String("42"),
524526
BytesOneof: m2.GetBytesOneof(),
527+
EmptyOneof: m2.GetEmptyOneof(),
525528
EnumOneof: proto.ValueOrNil(m2.HasEnumOneof(), m2.GetEnumOneof),
526529
IntOneof: proto.ValueOrNil(m2.HasIntOneof(), m2.GetIntOneof),
527530
MsgOneof: m2.GetMsgOneof(),
@@ -543,6 +546,7 @@ _ = pb2.M2_builder{OneofField: ifaceOneof}.Build()
543546
_ = pb2.M2_builder{
544547
S: proto.String("42"),
545548
BytesOneof: m2.GetBytesOneof(),
549+
EmptyOneof: m2.GetEmptyOneof(),
546550
EnumOneof: proto.ValueOrNil(m2.HasEnumOneof(), m2.GetEnumOneof),
547551
IntOneof: proto.ValueOrNil(m2.HasIntOneof(), m2.GetIntOneof),
548552
MsgOneof: m2.GetMsgOneof(),
@@ -570,26 +574,29 @@ _ = pb2.M2_builder{MsgOneof: proto.ValueOrDefault(msg)}.Build()
570574
desc: "oneofs: msg zero value",
571575
in: `
572576
_ = &pb2.M2{OneofField: &pb2.M2_MsgOneof{}}
577+
_ = &pb2.M2{OneofField: &pb2.M2_EmptyOneof{}}
573578
`,
574579
want: map[Level]string{
575580
Green: `
576581
_ = &pb2.M2{OneofField: &pb2.M2_MsgOneof{}}
582+
_ = &pb2.M2{OneofField: &pb2.M2_EmptyOneof{}}
577583
`,
578584
Red: `
579-
_ = pb2.M2_builder{OneofField: &pb2.M2_MsgOneof{}}.Build()
585+
_ = pb2.M2_builder{MsgOneof: &pb2.M2{}}.Build()
586+
_ = pb2.M2_builder{EmptyOneof: &xpb.Empty{}}.Build()
580587
`,
581588
},
582589
}, {
583-
desc: "oneofs: enum zero value", // no rewrite (see comments in rules.go)
590+
desc: "oneofs: enum zero value",
584591
in: `
585592
_ = &pb2.M2{OneofField: &pb2.M2_EnumOneof{}}
586593
`,
587594
want: map[Level]string{
588595
Green: `
589-
_ = &pb2.M2{OneofField: &pb2.M2_EnumOneof{}}
596+
_ = pb2.M2_builder{EnumOneof: 0}.Build()
590597
`,
591598
Red: `
592-
_ = pb2.M2_builder{OneofField: &pb2.M2_EnumOneof{}}.Build()
599+
_ = pb2.M2_builder{EnumOneof: 0}.Build()
593600
`,
594601
},
595602
}, {
@@ -617,10 +624,10 @@ _ = &pb2.M2{
617624
want: map[Level]string{
618625
Green: `
619626
msg := &pb2.M2{}
620-
_ = &pb2.M2{
621-
OneofField: &pb2.M2_StringOneof{"hello"},
622-
OneofField2: &pb2.M2_EnumOneof2{},
623-
}
627+
_ = pb2.M2_builder{
628+
StringOneof: proto.String("hello"),
629+
EnumOneof2: 0,
630+
}.Build()
624631
_ = &pb2.M2{
625632
OneofField: &pb2.M2_StringOneof{"hello"},
626633
OneofField2: &pb2.M2_MsgOneof2{msg},
@@ -636,18 +643,18 @@ _ = &pb2.M2{
636643
`,
637644
Yellow: `
638645
msg := &pb2.M2{}
639-
_ = &pb2.M2{
640-
OneofField: &pb2.M2_StringOneof{"hello"},
641-
OneofField2: &pb2.M2_EnumOneof2{},
642-
}
646+
_ = pb2.M2_builder{
647+
StringOneof: proto.String("hello"),
648+
EnumOneof2: 0,
649+
}.Build()
643650
_ = pb2.M2_builder{
644651
StringOneof: proto.String("hello"),
645652
MsgOneof2: proto.ValueOrDefault(msg),
646653
}.Build()
647-
_ = &pb2.M2{
648-
OneofField: &pb2.M2_StringOneof{"hello"},
649-
OneofField2: &pb2.M2_MsgOneof2{},
650-
}
654+
_ = pb2.M2_builder{
655+
StringOneof: proto.String("hello"),
656+
MsgOneof2: &pb2.M2{},
657+
}.Build()
651658
_ = &pb2.M2{
652659
OneofField: &pb2.M2_StringOneof{"hello"},
653660
OneofField2: F(),

‎internal/fix/fix.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -411,6 +411,9 @@ func (cpkg *ConfiguredPackage) Fix() (Result, error) {
411411
for _, imp := range c.imports.importsToAdd {
412412
cur.InsertAfter(imp)
413413
c.setType(imp.Path, types.Typ[types.Invalid])
414+
if imp.Name != nil {
415+
c.setType(imp.Name, types.Typ[types.Invalid])
416+
}
414417
}
415418
return false // import added, abort traversal
416419
})

‎internal/fix/testdata/proto2test_go_proto/proto2test.proto‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ edition = "2023";
77
package net.proto2.go.open2opaque.o2o.test;
88

99
import "google/protobuf/go_features.proto";
10+
import "google/protobuf/empty.proto";
1011

1112
message DoNotMigrateMe {
1213
option features.(pb.go).api_level = API_OPEN;
@@ -40,6 +41,7 @@ message M2 {
4041
M2 msg_oneof = 16;
4142
Enum enum_oneof = 17;
4243
bytes bytes_oneof = 18;
44+
google.protobuf.Empty empty_oneof = 99;
4345
}
4446
oneof oneof_field2 {
4547
string string_oneof2 = 19;

0 commit comments

Comments
 (0)