From 2710aa360c877ba6808c5e6fb9682d5a56beab8b Mon Sep 17 00:00:00 2001 From: Josh Humphries <2035234+jhump@users.noreply.github.com> Date: Mon, 29 Apr 2024 10:38:15 -0400 Subject: [PATCH] update oneof checks to ignore synthetic oneofs; these are now handled by cardinality check --- .../bufcheck/bufbreaking/bufbreaking_test.go | 24 +++--- .../bufbreakingcheck/bufbreakingcheck.go | 37 ++++++++- .../breaking_field_same_oneof/1.proto | 64 +++++++-------- .../breaking_field_same_oneof/2.proto | 81 ++++++++++--------- .../testdata/breaking_oneof_no_delete/2.proto | 1 + .../breaking_field_same_oneof/1.proto | 73 ++++++++--------- .../breaking_field_same_oneof/2.proto | 72 ++++++++--------- .../breaking_oneof_no_delete/1.proto | 1 + 8 files changed, 196 insertions(+), 157 deletions(-) diff --git a/private/bufpkg/bufcheck/bufbreaking/bufbreaking_test.go b/private/bufpkg/bufcheck/bufbreaking/bufbreaking_test.go index 6f72b2ddb4..57c1751028 100644 --- a/private/bufpkg/bufcheck/bufbreaking/bufbreaking_test.go +++ b/private/bufpkg/bufcheck/bufbreaking/bufbreaking_test.go @@ -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"), ) } @@ -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"), ) } diff --git a/private/bufpkg/bufcheck/bufbreaking/internal/bufbreakingcheck/bufbreakingcheck.go b/private/bufpkg/bufcheck/bufbreaking/internal/bufbreakingcheck/bufbreakingcheck.go index 9bcc8e44d7..a850942c8f 100644 --- a/private/bufpkg/bufcheck/bufbreaking/internal/bufbreakingcheck/bufbreakingcheck.go +++ b/private/bufpkg/bufcheck/bufbreaking/internal/bufbreakingcheck/bufbreakingcheck.go @@ -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 { @@ -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()) } } diff --git a/private/bufpkg/bufcheck/bufbreaking/testdata/breaking_field_same_oneof/1.proto b/private/bufpkg/bufcheck/bufbreaking/testdata/breaking_field_same_oneof/1.proto index 26e32baa6b..604370feca 100644 --- a/private/bufpkg/bufcheck/bufbreaking/testdata/breaking_field_same_oneof/1.proto +++ b/private/bufpkg/bufcheck/bufbreaking/testdata/breaking_field_same_oneof/1.proto @@ -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; } diff --git a/private/bufpkg/bufcheck/bufbreaking/testdata/breaking_field_same_oneof/2.proto b/private/bufpkg/bufcheck/bufbreaking/testdata/breaking_field_same_oneof/2.proto index 3cd05e5e06..5421311f53 100644 --- a/private/bufpkg/bufcheck/bufbreaking/testdata/breaking_field_same_oneof/2.proto +++ b/private/bufpkg/bufcheck/bufbreaking/testdata/breaking_field_same_oneof/2.proto @@ -4,30 +4,31 @@ 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; } @@ -35,57 +36,57 @@ message Two2 { 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; } @@ -93,9 +94,9 @@ message Nine2 { message Nine { int32 one = 1; oneof baz { - int32 two = 2; + int32 two = 2; } oneof foo { - int32 three = 3; + int32 three = 3; } } diff --git a/private/bufpkg/bufcheck/bufbreaking/testdata/breaking_oneof_no_delete/2.proto b/private/bufpkg/bufcheck/bufbreaking/testdata/breaking_oneof_no_delete/2.proto index d336ecc6b2..211309f141 100644 --- a/private/bufpkg/bufcheck/bufbreaking/testdata/breaking_oneof_no_delete/2.proto +++ b/private/bufpkg/bufcheck/bufbreaking/testdata/breaking_oneof_no_delete/2.proto @@ -8,6 +8,7 @@ message One { int32 two = 2; int32 three = 3; } + string four = 4; } message One2 { diff --git a/private/bufpkg/bufcheck/bufbreaking/testdata_previous/breaking_field_same_oneof/1.proto b/private/bufpkg/bufcheck/bufbreaking/testdata_previous/breaking_field_same_oneof/1.proto index b24e0f25d9..3c1df487c5 100644 --- a/private/bufpkg/bufcheck/bufbreaking/testdata_previous/breaking_field_same_oneof/1.proto +++ b/private/bufpkg/bufcheck/bufbreaking/testdata_previous/breaking_field_same_oneof/1.proto @@ -4,20 +4,21 @@ package a; message One { oneof foo { - int32 one = 1; + int32 one = 1; } oneof bar { - int32 two = 2; + int32 two = 2; } int32 three = 3; + optional string four = 4; } message Two { oneof foo { - int32 one = 1; + int32 one = 1; } oneof bar { - int32 two = 2; + int32 two = 2; } int32 three = 3; } @@ -25,57 +26,57 @@ message Two { message Three { message Four { message Five { - 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 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 { - 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 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; } message Nine { oneof foo { - int32 one = 1; + int32 one = 1; } oneof bar { - int32 two = 2; + int32 two = 2; } int32 three = 3; } diff --git a/private/bufpkg/bufcheck/bufbreaking/testdata_previous/breaking_field_same_oneof/2.proto b/private/bufpkg/bufcheck/bufbreaking/testdata_previous/breaking_field_same_oneof/2.proto index bd2964f3b2..c982e892f4 100644 --- a/private/bufpkg/bufcheck/bufbreaking/testdata_previous/breaking_field_same_oneof/2.proto +++ b/private/bufpkg/bufcheck/bufbreaking/testdata_previous/breaking_field_same_oneof/2.proto @@ -4,20 +4,20 @@ package a; 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; } @@ -25,57 +25,57 @@ message Two2 { 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; } diff --git a/private/bufpkg/bufcheck/bufbreaking/testdata_previous/breaking_oneof_no_delete/1.proto b/private/bufpkg/bufcheck/bufbreaking/testdata_previous/breaking_oneof_no_delete/1.proto index fdee1035a6..f6558b5b71 100644 --- a/private/bufpkg/bufcheck/bufbreaking/testdata_previous/breaking_oneof_no_delete/1.proto +++ b/private/bufpkg/bufcheck/bufbreaking/testdata_previous/breaking_oneof_no_delete/1.proto @@ -8,6 +8,7 @@ message One { int32 two = 2; int32 three = 3; } + optional string four = 4; } message Two {