Skip to content

Commit

Permalink
update oneof checks to ignore synthetic oneofs; these are now handled…
Browse files Browse the repository at this point in the history 
… by cardinality check
  • Loading branch information
@jhump
jhump committed Apr 29, 2024
1 parent 1b1731a commit 3aadaf2
Show file tree
Hide file tree
Showing 8 changed files with 196 additions and 157 deletions.
24 changes: 12 additions & 12 deletions private/bufpkg/bufcheck/bufbreaking/bufbreaking_test.go
View file
Expand Up @@ -285,17 +285,17 @@ func TestRunBreakingFieldSameOneof(t *testing.T) {
t,
"breaking_field_same_oneof",
bufanalysistesting.NewFileAnnotation(t, "1.proto", 6, 3, 6, 17, "FIELD_SAME_ONEOF"),
bufanalysistesting.NewFileAnnotation(t, "1.proto", 8, 3, 8, 17, "FIELD_SAME_ONEOF"),
bufanalysistesting.NewFileAnnotation(t, "1.proto", 11, 3, 11, 19, "FIELD_SAME_ONEOF"),
bufanalysistesting.NewFileAnnotation(t, "1.proto", 18, 3, 18, 17, "FIELD_SAME_ONEOF"),
bufanalysistesting.NewFileAnnotation(t, "1.proto", 20, 3, 20, 17, "FIELD_SAME_ONEOF"),
bufanalysistesting.NewFileAnnotation(t, "1.proto", 23, 3, 23, 19, "FIELD_SAME_ONEOF"),
bufanalysistesting.NewFileAnnotation(t, "1.proto", 37, 3, 37, 17, "FIELD_SAME_ONEOF"),
bufanalysistesting.NewFileAnnotation(t, "1.proto", 39, 3, 39, 17, "FIELD_SAME_ONEOF"),
bufanalysistesting.NewFileAnnotation(t, "1.proto", 42, 3, 42, 19, "FIELD_SAME_ONEOF"),
bufanalysistesting.NewFileAnnotation(t, "2.proto", 94, 3, 94, 17, "FIELD_SAME_ONEOF"),
bufanalysistesting.NewFileAnnotation(t, "2.proto", 96, 3, 96, 17, "FIELD_SAME_ONEOF"),
bufanalysistesting.NewFileAnnotation(t, "2.proto", 99, 3, 99, 19, "FIELD_SAME_ONEOF"),
bufanalysistesting.NewFileAnnotation(t, "1.proto", 8, 5, 8, 19, "FIELD_SAME_ONEOF"),
bufanalysistesting.NewFileAnnotation(t, "1.proto", 11, 5, 11, 21, "FIELD_SAME_ONEOF"),
bufanalysistesting.NewFileAnnotation(t, "1.proto", 18, 7, 18, 21, "FIELD_SAME_ONEOF"),
bufanalysistesting.NewFileAnnotation(t, "1.proto", 20, 9, 20, 23, "FIELD_SAME_ONEOF"),
bufanalysistesting.NewFileAnnotation(t, "1.proto", 23, 9, 23, 25, "FIELD_SAME_ONEOF"),
bufanalysistesting.NewFileAnnotation(t, "1.proto", 37, 5, 37, 19, "FIELD_SAME_ONEOF"),
bufanalysistesting.NewFileAnnotation(t, "1.proto", 39, 7, 39, 21, "FIELD_SAME_ONEOF"),
bufanalysistesting.NewFileAnnotation(t, "1.proto", 42, 7, 42, 23, "FIELD_SAME_ONEOF"),
bufanalysistesting.NewFileAnnotation(t, "2.proto", 95, 3, 95, 17, "FIELD_SAME_ONEOF"),
bufanalysistesting.NewFileAnnotation(t, "2.proto", 97, 5, 97, 19, "FIELD_SAME_ONEOF"),
bufanalysistesting.NewFileAnnotation(t, "2.proto", 100, 5, 100, 21, "FIELD_SAME_ONEOF"),
)
}

Expand Down Expand Up @@ -597,7 +597,7 @@ func TestRunBreakingOneofNoDelete(t *testing.T) {
bufanalysistesting.NewFileAnnotation(t, "1.proto", 5, 1, 9, 2, "ONEOF_NO_DELETE"),
bufanalysistesting.NewFileAnnotation(t, "1.proto", 13, 5, 17, 6, "ONEOF_NO_DELETE"),
bufanalysistesting.NewFileAnnotation(t, "1.proto", 26, 3, 30, 4, "ONEOF_NO_DELETE"),
bufanalysistesting.NewFileAnnotation(t, "2.proto", 75, 1, 79, 2, "ONEOF_NO_DELETE"),
bufanalysistesting.NewFileAnnotation(t, "2.proto", 76, 1, 80, 2, "ONEOF_NO_DELETE"),
)
}

Expand Down
37 changes: 36 additions & 1 deletion private/bufpkg/bufcheck/bufbreaking/internal/bufbreakingcheck/bufbreakingcheck.go
View file
Expand Up @@ -302,7 +302,31 @@ var CheckFieldSameOneof = newFieldPairCheckFunc(checkFieldSameOneof)

func checkFieldSameOneof(add addFunc, corpus *corpus, previousField bufprotosource.Field, field bufprotosource.Field) error {
previousOneof := previousField.Oneof()
if previousOneof != nil {
previousOneofDescriptor, err := previousOneof.AsDescriptor()
if err != nil {
return err
}
if previousOneofDescriptor.IsSynthetic() {
// Not considering synthetic oneofs since those are really
// just strange byproducts of how "explicit presence" is
// modeled in proto3 syntax. We will separately detect this
// kind of change via field presence check.
previousOneof = nil
}
}
oneof := field.Oneof()
if oneof != nil {
oneofDescriptor, err := oneof.AsDescriptor()
if err != nil {
return err
}
if oneofDescriptor.IsSynthetic() {
// Same remark as above.
oneof = nil
}
}

previousInsideOneof := previousOneof != nil
insideOneof := oneof != nil
if !previousInsideOneof && !insideOneof {
Expand Down Expand Up @@ -904,8 +928,19 @@ func checkOneofNoDelete(add addFunc, corpus *corpus, previousMessage bufprotosou
if err != nil {
return err
}
for previousName := range previousNameToOneof {
for previousName, previousOneof := range previousNameToOneof {
if _, ok := nameToOneof[previousName]; !ok {
previousOneofDescriptor, err := previousOneof.AsDescriptor()
if err != nil {
return err
}
if previousOneofDescriptor.IsSynthetic() {
// Not considering synthetic oneofs since those are really
// just strange byproducts of how "explicit presence" is
// modeled in proto3 syntax. We will separately detect this
// kind of change via field presence check.
continue
}
add(message, nil, message.Location(), `Previously present oneof %q on message %q was deleted.`, previousName, message.Name())
}
}
Expand Down
64 changes: 32 additions & 32 deletions private/bufpkg/bufcheck/bufbreaking/testdata/breaking_field_same_oneof/1.proto
View file
Expand Up @@ -5,57 +5,57 @@ package a;
message Two {
int32 one = 1;
oneof baz {
int32 two = 2;
int32 two = 2;
}
oneof foo {
int32 three = 3;
int32 three = 3;
}
}

message Three {
message Four {
message Five {
int32 one = 1;
oneof baz {
int32 two = 2;
}
oneof foo {
int32 three = 3;
}
int32 one = 1;
oneof baz {
int32 two = 2;
}
oneof foo {
int32 three = 3;
}
}
message Six {
oneof foo {
int32 one = 1;
}
oneof bar {
int32 two = 2;
}
int32 three = 3;
oneof foo {
int32 one = 1;
}
oneof bar {
int32 two = 2;
}
int32 three = 3;
}
}
message Seven {
int32 one = 1;
oneof baz {
int32 two = 2;
}
oneof foo {
int32 three = 3;
}
int32 one = 1;
oneof baz {
int32 two = 2;
}
oneof foo {
int32 three = 3;
}
}
message Eight {
oneof foo {
int32 one = 1;
}
oneof bar {
int32 two = 2;
}
int32 three = 3;
oneof foo {
int32 one = 1;
}
oneof bar {
int32 two = 2;
}
int32 three = 3;
}
oneof foo {
int32 one = 1;
int32 one = 1;
}
oneof bar {
int32 two = 2;
int32 two = 2;
}
int32 three = 3;
}
81 changes: 41 additions & 40 deletions private/bufpkg/bufcheck/bufbreaking/testdata/breaking_field_same_oneof/2.proto
View file
Expand Up @@ -4,98 +4,99 @@ package a;

message One {
oneof foo {
int32 one = 1;
int32 one = 1;
}
oneof bar {
int32 two = 2;
int32 two = 2;
}
int32 three = 3;
string four = 4;
}

message One2 {
oneof foo {
int32 one = 1;
int32 one = 1;
}
oneof bar {
int32 two = 2;
int32 two = 2;
}
int32 three = 3;
}

message Two2 {
oneof foo {
int32 one = 1;
int32 one = 1;
}
oneof bar {
int32 two = 2;
int32 two = 2;
}
int32 three = 3;
}

message Three2 {
message Four2 {
message Five2 {
oneof foo {
int32 one = 1;
}
oneof bar {
int32 two = 2;
}
int32 three = 3;
oneof foo {
int32 one = 1;
}
oneof bar {
int32 two = 2;
}
int32 three = 3;
}
message Six2 {
oneof foo {
int32 one = 1;
}
oneof bar {
int32 two = 2;
}
int32 three = 3;
oneof foo {
int32 one = 1;
}
oneof bar {
int32 two = 2;
}
int32 three = 3;
}
}
message Seven2 {
oneof foo {
int32 one = 1;
}
oneof bar {
int32 two = 2;
}
int32 three = 3;
oneof foo {
int32 one = 1;
}
oneof bar {
int32 two = 2;
}
int32 three = 3;
}
message Eight2 {
oneof foo {
int32 one = 1;
}
oneof bar {
int32 two = 2;
}
int32 three = 3;
oneof foo {
int32 one = 1;
}
oneof bar {
int32 two = 2;
}
int32 three = 3;
}
oneof foo {
int32 one = 1;
int32 one = 1;
}
oneof bar {
int32 two = 2;
int32 two = 2;
}
int32 three = 3;
}

message Nine2 {
oneof foo {
int32 one = 1;
int32 one = 1;
}
oneof bar {
int32 two = 2;
int32 two = 2;
}
int32 three = 3;
}

message Nine {
int32 one = 1;
oneof baz {
int32 two = 2;
int32 two = 2;
}
oneof foo {
int32 three = 3;
int32 three = 3;
}
}
1 change: 1 addition & 0 deletions private/bufpkg/bufcheck/bufbreaking/testdata/breaking_oneof_no_delete/2.proto
View file
Expand Up @@ -8,6 +8,7 @@ message One {
int32 two = 2;
int32 three = 3;
}
string four = 4;
}

message One2 {
Expand Down

0 comments on commit 3aadaf2

Please sign in to comment.