From db1d4249d1e0a3f9622c8f09d241ee21b097cd18 Mon Sep 17 00:00:00 2001 From: Joshua Humphries <2035234+jhump@users.noreply.github.com> Date: Mon, 29 Apr 2024 10:40:12 -0400 Subject: [PATCH] Make "buf lint" work correctly with top-level extensions (#2920) This fixes a potential panic introduced in a previous PR and fleshes out support (including tests) for running lint checks over top-level extensions (which in past versions were inadvertently excluded from lint checks). --- .../bufpkg/bufcheck/buflint/buflint_test.go | 62 ++++++++++++------- .../internal/buflintcheck/buflintcheck.go | 27 ++++---- .../testdata/field_lower_snake_case/b.proto | 18 ++++++ .../testdata/field_no_descriptor/a.proto | 24 +++++++ 4 files changed, 98 insertions(+), 33 deletions(-) create mode 100644 private/bufpkg/bufcheck/buflint/testdata/field_lower_snake_case/b.proto diff --git a/private/bufpkg/bufcheck/buflint/buflint_test.go b/private/bufpkg/bufcheck/buflint/buflint_test.go index 50b7b81c24..8e42049481 100644 --- a/private/bufpkg/bufcheck/buflint/buflint_test.go +++ b/private/bufpkg/bufcheck/buflint/buflint_test.go @@ -329,6 +329,8 @@ func TestRunFieldLowerSnakeCase(t *testing.T) { bufanalysistesting.NewFileAnnotation(t, "a.proto", 30, 11, 30, 20, "FIELD_LOWER_SNAKE_CASE"), bufanalysistesting.NewFileAnnotation(t, "a.proto", 31, 11, 31, 21, "FIELD_LOWER_SNAKE_CASE"), bufanalysistesting.NewFileAnnotation(t, "a.proto", 32, 11, 32, 21, "FIELD_LOWER_SNAKE_CASE"), + bufanalysistesting.NewFileAnnotation(t, "b.proto", 10, 23, 10, 24, "FIELD_LOWER_SNAKE_CASE"), + bufanalysistesting.NewFileAnnotation(t, "b.proto", 17, 19, 17, 20, "FIELD_LOWER_SNAKE_CASE"), ) } @@ -345,30 +347,46 @@ func TestRunFieldNoDescriptor(t *testing.T) { bufanalysistesting.NewFileAnnotation(t, "a.proto", 11, 19, 11, 30, "FIELD_NO_DESCRIPTOR"), bufanalysistesting.NewFileAnnotation(t, "a.proto", 12, 19, 12, 31, "FIELD_NO_DESCRIPTOR"), bufanalysistesting.NewFileAnnotation(t, "a.proto", 13, 19, 13, 33, "FIELD_NO_DESCRIPTOR"), - bufanalysistesting.NewFileAnnotation(t, "a.proto", 19, 23, 19, 33, "FIELD_NO_DESCRIPTOR"), bufanalysistesting.NewFileAnnotation(t, "a.proto", 20, 23, 20, 33, "FIELD_NO_DESCRIPTOR"), bufanalysistesting.NewFileAnnotation(t, "a.proto", 21, 23, 21, 33, "FIELD_NO_DESCRIPTOR"), - bufanalysistesting.NewFileAnnotation(t, "a.proto", 22, 23, 22, 34, "FIELD_NO_DESCRIPTOR"), - bufanalysistesting.NewFileAnnotation(t, "a.proto", 23, 23, 23, 35, "FIELD_NO_DESCRIPTOR"), - bufanalysistesting.NewFileAnnotation(t, "a.proto", 24, 23, 24, 34, "FIELD_NO_DESCRIPTOR"), - bufanalysistesting.NewFileAnnotation(t, "a.proto", 25, 23, 25, 35, "FIELD_NO_DESCRIPTOR"), - bufanalysistesting.NewFileAnnotation(t, "a.proto", 26, 23, 26, 37, "FIELD_NO_DESCRIPTOR"), - bufanalysistesting.NewFileAnnotation(t, "a.proto", 28, 21, 28, 31, "FIELD_NO_DESCRIPTOR"), - bufanalysistesting.NewFileAnnotation(t, "a.proto", 29, 21, 29, 31, "FIELD_NO_DESCRIPTOR"), - bufanalysistesting.NewFileAnnotation(t, "a.proto", 30, 21, 30, 31, "FIELD_NO_DESCRIPTOR"), - bufanalysistesting.NewFileAnnotation(t, "a.proto", 31, 21, 31, 32, "FIELD_NO_DESCRIPTOR"), - bufanalysistesting.NewFileAnnotation(t, "a.proto", 32, 21, 32, 33, "FIELD_NO_DESCRIPTOR"), - bufanalysistesting.NewFileAnnotation(t, "a.proto", 33, 21, 33, 32, "FIELD_NO_DESCRIPTOR"), - bufanalysistesting.NewFileAnnotation(t, "a.proto", 34, 21, 34, 33, "FIELD_NO_DESCRIPTOR"), - bufanalysistesting.NewFileAnnotation(t, "a.proto", 35, 21, 35, 35, "FIELD_NO_DESCRIPTOR"), - bufanalysistesting.NewFileAnnotation(t, "a.proto", 37, 19, 37, 29, "FIELD_NO_DESCRIPTOR"), - bufanalysistesting.NewFileAnnotation(t, "a.proto", 38, 19, 38, 29, "FIELD_NO_DESCRIPTOR"), - bufanalysistesting.NewFileAnnotation(t, "a.proto", 39, 19, 39, 29, "FIELD_NO_DESCRIPTOR"), - bufanalysistesting.NewFileAnnotation(t, "a.proto", 40, 19, 40, 30, "FIELD_NO_DESCRIPTOR"), - bufanalysistesting.NewFileAnnotation(t, "a.proto", 41, 19, 41, 31, "FIELD_NO_DESCRIPTOR"), - bufanalysistesting.NewFileAnnotation(t, "a.proto", 42, 19, 42, 30, "FIELD_NO_DESCRIPTOR"), - bufanalysistesting.NewFileAnnotation(t, "a.proto", 43, 19, 43, 31, "FIELD_NO_DESCRIPTOR"), - bufanalysistesting.NewFileAnnotation(t, "a.proto", 44, 19, 44, 33, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 22, 23, 22, 33, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 23, 23, 23, 34, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 24, 23, 24, 35, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 25, 23, 25, 34, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 26, 23, 26, 35, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 27, 23, 27, 37, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 30, 27, 30, 38, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 31, 27, 31, 39, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 32, 27, 32, 38, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 33, 27, 33, 39, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 34, 27, 34, 41, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 35, 27, 35, 39, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 36, 27, 36, 38, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 37, 27, 37, 39, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 41, 21, 41, 31, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 42, 21, 42, 31, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 43, 21, 43, 31, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 44, 21, 44, 32, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 45, 21, 45, 33, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 46, 21, 46, 32, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 47, 21, 47, 33, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 48, 21, 48, 35, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 50, 19, 50, 29, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 51, 19, 51, 29, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 52, 19, 52, 29, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 53, 19, 53, 30, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 54, 19, 54, 31, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 55, 19, 55, 30, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 56, 19, 56, 31, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 57, 19, 57, 33, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 61, 19, 61, 29, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 62, 19, 62, 29, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 63, 19, 63, 29, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 64, 19, 64, 30, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 65, 19, 65, 31, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 66, 19, 66, 30, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 67, 19, 67, 31, "FIELD_NO_DESCRIPTOR"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 68, 19, 68, 33, "FIELD_NO_DESCRIPTOR"), ) } diff --git a/private/bufpkg/bufcheck/buflint/internal/buflintcheck/buflintcheck.go b/private/bufpkg/bufcheck/buflint/internal/buflintcheck/buflintcheck.go index a60fb2307b..0b745d94e3 100644 --- a/private/bufpkg/bufcheck/buflint/internal/buflintcheck/buflintcheck.go +++ b/private/bufpkg/bufcheck/buflint/internal/buflintcheck/buflintcheck.go @@ -268,21 +268,24 @@ func checkEnumZeroValueSuffix(add addFunc, enumValue bufprotosource.EnumValue, s var CheckFieldLowerSnakeCase = newFieldCheckFunc(checkFieldLowerSnakeCase) func checkFieldLowerSnakeCase(add addFunc, field bufprotosource.Field) error { - if message := field.ParentMessage(); message != nil && message.IsMapEntry() { + message := field.ParentMessage() + if message != nil && message.IsMapEntry() { // this check should always pass anyways but just in case return nil } name := field.Name() expectedName := fieldToLowerSnakeCase(name) if name != expectedName { + var otherLocs []bufprotosource.Location + if message != nil { + // also check the message for this comment ignore + // this allows users to set this "globally" for a message + otherLocs = []bufprotosource.Location{message.Location()} + } add( field, field.NameLocation(), - // also check the message for this comment ignore - // this allows users to set this "globally" for a message - []bufprotosource.Location{ - field.ParentMessage().Location(), - }, + otherLocs, "Field name %q should be lower_snake_case, such as %q.", name, expectedName, @@ -297,14 +300,16 @@ var CheckFieldNoDescriptor = newFieldCheckFunc(checkFieldNoDescriptor) func checkFieldNoDescriptor(add addFunc, field bufprotosource.Field) error { name := field.Name() if strings.ToLower(strings.Trim(name, "_")) == "descriptor" { + var otherLocs []bufprotosource.Location + if message := field.ParentMessage(); message != nil { + // also check the message for this comment ignore + // this allows users to set this "globally" for a message + otherLocs = []bufprotosource.Location{message.Location()} + } add( field, field.NameLocation(), - // also check the message for this comment ignore - // this allows users to set this "globally" for a message - []bufprotosource.Location{ - field.ParentMessage().Location(), - }, + otherLocs, `Field name %q cannot be any capitalization of "descriptor" with any number of prefix or suffix underscores.`, name, ) diff --git a/private/bufpkg/bufcheck/buflint/testdata/field_lower_snake_case/b.proto b/private/bufpkg/bufcheck/buflint/testdata/field_lower_snake_case/b.proto new file mode 100644 index 0000000000..1932c18ebc --- /dev/null +++ b/private/bufpkg/bufcheck/buflint/testdata/field_lower_snake_case/b.proto @@ -0,0 +1,18 @@ +syntax = "proto2"; + +package b; + +message Foo { + extensions 1 to 100; + message Bar { + extend Foo { + optional string a = 1; + optional string B = 2; + } + } +} + +extend Foo { + optional string c = 3; + optional string D = 4; +} diff --git a/private/bufpkg/bufcheck/buflint/testdata/field_no_descriptor/a.proto b/private/bufpkg/bufcheck/buflint/testdata/field_no_descriptor/a.proto index 38233bd69d..a0e52957dc 100644 --- a/private/bufpkg/bufcheck/buflint/testdata/field_no_descriptor/a.proto +++ b/private/bufpkg/bufcheck/buflint/testdata/field_no_descriptor/a.proto @@ -11,6 +11,7 @@ message One { optional string descriptor_ = 6; optional string descriptor__ = 7; optional string __descriptor__ = 8; + extensions 10 to 100; } message Two { @@ -24,6 +25,18 @@ message Two { optional string descriptor_ = 6; optional string descriptor__ = 7; optional string __descriptor__ = 8; + message Five { + extend One { + optional string descriptor = 10; + optional string Descriptor = 11; + optional string descRiptor = 12; + optional string _descriptor = 13; + optional string __descriptor = 14; + optional string descriptor_ = 15; + optional string descriptor__ = 16; + optional string __descriptor__ = 17; + } + } } optional string descriptor = 1; optional string Descriptor = 2; @@ -43,3 +56,14 @@ message Two { optional string descriptor__ = 7; optional string __descriptor__ = 8; } + +extend One { + optional string descriptor = 20; + optional string Descriptor = 21; + optional string descRiptor = 22; + optional string _descriptor = 23; + optional string __descriptor = 24; + optional string descriptor_ = 25; + optional string descriptor__ = 26; + optional string __descriptor__ = 27; +}