From 65e3536943be6595c7fae138209bf59bbdbedca8 Mon Sep 17 00:00:00 2001 From: Anuraag Agrawal Date: Wed, 16 Sep 2026 17:46:42 +0900 Subject: [PATCH 1/4] Resolve ignore comments for group fields --- CHANGELOG.md | 1 + private/bufpkg/bufcheck/client.go | 118 ++++++++++++- private/bufpkg/bufcheck/client_test.go | 158 ++++++++++++++++++ private/bufpkg/bufcheck/lint_test.go | 10 ++ .../lint/comment_ignores_group/a.proto | 22 +++ .../lint/comment_ignores_group/buf.yaml | 4 + 6 files changed, 308 insertions(+), 5 deletions(-) create mode 100644 private/bufpkg/bufcheck/client_test.go create mode 100644 private/bufpkg/bufcheck/testdata/lint/comment_ignores_group/a.proto create mode 100644 private/bufpkg/bufcheck/testdata/lint/comment_ignores_group/buf.yaml diff --git a/CHANGELOG.md b/CHANGELOG.md index 76c945721b..ade74bd173 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ - Add `--stdin-filepath` flag to `buf format`, which reads a single `.proto` file from stdin and writes the formatted result to stdout. The path is not read from disk, and is only used to report parse errors and diffs. +- Fix lint comment ignores on proto2 `group` fields being ignored. ## [v1.73.0] - 2026-09-11 diff --git a/private/bufpkg/bufcheck/client.go b/private/bufpkg/bufcheck/client.go index 05d1c5ff26..6c5e00f7a9 100644 --- a/private/bufpkg/bufcheck/client.go +++ b/private/bufpkg/bufcheck/client.go @@ -43,6 +43,7 @@ import ( "github.com/bufbuild/buf/private/pkg/protoversion" "github.com/bufbuild/buf/private/pkg/syserror" "github.com/google/uuid" + "google.golang.org/protobuf/reflect/protoreflect" "pluginrpc.com/pluginrpc" ) @@ -824,11 +825,17 @@ func ignoreFileLocation( sourceLocations := protoreflectFileDescriptor.SourceLocations() for _, associatedSourcePath := range associatedSourcePaths { sourceLocation := sourceLocations.ByPath(associatedSourcePath) - if leadingComments := sourceLocation.LeadingComments; leadingComments != "" { - for _, line := range xstrings.SplitTrimLinesNoEmpty(leadingComments) { - if checkCommentLineForCheckIgnore(line, config.CommentIgnorePrefix, ruleID) { - return true, nil - } + if leadingCommentsHaveCheckIgnore(sourceLocation.LeadingComments, config.CommentIgnorePrefix, ruleID) { + return true, nil + } + // A group field has both a field and synthetic message in the descriptor, with comments + // that appear to be on the field actually assigned to the synthetic message. + // So we need to see if the path is a group, resolve its synthetic message, and check the comments + // there if so. + if syntheticMessage := groupFieldSyntheticMessage(protoreflectFileDescriptor, associatedSourcePath); syntheticMessage != nil { + syntheticMessageSourceLocation := sourceLocations.ByDescriptor(syntheticMessage) + if leadingCommentsHaveCheckIgnore(syntheticMessageSourceLocation.LeadingComments, config.CommentIgnorePrefix, ruleID) { + return true, nil } } } @@ -836,6 +843,107 @@ func ignoreFileLocation( return false, nil } +// groupFieldSyntheticMessage returns the synthetic message declaration for the group field +// at the given source path, or nil if the source path does not point to a group field. +func groupFieldSyntheticMessage( + fileDescriptor protoreflect.FileDescriptor, + sourcePath protoreflect.SourcePath, +) protoreflect.MessageDescriptor { + fieldDescriptor := fieldDescriptorForSourcePath(fileDescriptor, sourcePath) + if fieldDescriptor == nil || fieldDescriptor.Kind() != protoreflect.GroupKind { + return nil + } + return fieldDescriptor.Message() +} + +// Source path tags for the descriptor fields traversed when resolving a source path to a +// field declaration. +const ( + // FileDescriptorProto.message_type. + fileMessagesTag = int32(4) + // FileDescriptorProto.extension. + fileExtensionsTag = int32(7) + // DescriptorProto.field. + messageFieldsTag = int32(2) + // DescriptorProto.nested_type. + messageNestedMessagesTag = int32(3) + // DescriptorProto.extension. + messageExtensionsTag = int32(6) +) + +// fieldDescriptorForSourcePath returns the field declaration at the given source path, or +// nil if the source path does not point to a field declaration. +// +// A source path for a field declaration alternates a tag and an index, descending through +// message declarations before terminating at a field or an extension field, for example +// [4, 0, 3, 1, 2, 0] for .message_type(0).nested_type(1).field(0). +func fieldDescriptorForSourcePath( + fileDescriptor protoreflect.FileDescriptor, + sourcePath protoreflect.SourcePath, +) protoreflect.FieldDescriptor { + if len(sourcePath) < 2 || len(sourcePath)%2 != 0 { + return nil + } + // The message that the declaration at the end of the source path belongs to, or nil if + // the declaration is at the top level of the file. + var parentMessageDescriptor protoreflect.MessageDescriptor + for i := 0; i < len(sourcePath)-2; i += 2 { + tag, index := sourcePath[i], int(sourcePath[i+1]) + var messageDescriptors protoreflect.MessageDescriptors + switch { + case parentMessageDescriptor == nil && tag == fileMessagesTag: + messageDescriptors = fileDescriptor.Messages() + case parentMessageDescriptor != nil && tag == messageNestedMessagesTag: + messageDescriptors = parentMessageDescriptor.Messages() + default: + return nil + } + if index < 0 || index >= messageDescriptors.Len() { + return nil + } + parentMessageDescriptor = messageDescriptors.Get(index) + } + tag, index := sourcePath[len(sourcePath)-2], int(sourcePath[len(sourcePath)-1]) + if index < 0 { + return nil + } + if parentMessageDescriptor == nil { + if tag == fileExtensionsTag && index < fileDescriptor.Extensions().Len() { + return fileDescriptor.Extensions().Get(index) + } + return nil + } + switch tag { + case messageFieldsTag: + if index < parentMessageDescriptor.Fields().Len() { + return parentMessageDescriptor.Fields().Get(index) + } + case messageExtensionsTag: + if index < parentMessageDescriptor.Extensions().Len() { + return parentMessageDescriptor.Extensions().Get(index) + } + } + return nil +} + +// leadingCommentsHaveCheckIgnore checks if any line of the given leading comments is a +// comment ignore for the given rule. +func leadingCommentsHaveCheckIgnore( + leadingComments string, + commentIgnorePrefix string, + ruleID string, +) bool { + if leadingComments == "" { + return false + } + for _, line := range xstrings.SplitTrimLinesNoEmpty(leadingComments) { + if checkCommentLineForCheckIgnore(line, commentIgnorePrefix, ruleID) { + return true + } + } + return false +} + // checkCommentLineForCheckIgnore checks that the comment line starts with the configured // comment ignore prefix, a space and the ruleID of the check. // diff --git a/private/bufpkg/bufcheck/client_test.go b/private/bufpkg/bufcheck/client_test.go new file mode 100644 index 0000000000..123228664b --- /dev/null +++ b/private/bufpkg/bufcheck/client_test.go @@ -0,0 +1,158 @@ +// Copyright 2020-2026 Buf Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package bufcheck + +import ( + "context" + "testing" + + "github.com/bufbuild/protocompile" + "github.com/stretchr/testify/require" + "google.golang.org/protobuf/reflect/protoreflect" +) + +const groupsProtoFileContent = `syntax = "proto2"; + +package a; + +message Foo { + optional group Group = 1 { + optional string id = 1; + } + optional string scalar = 2; + message Nested { + optional group NestedGroup = 1 { + optional string id = 1; + } + } + extend Bar { + optional group MessageExtensionGroup = 101 { + optional string id = 1; + } + } +} + +message Bar { + extensions 100 to 200; +} + +extend Bar { + optional group FileExtensionGroup = 100 { + optional string id = 1; + } +} +` + +func TestGroupFieldSyntheticMessage(t *testing.T) { + t.Parallel() + fileDescriptor := testCompileFileDescriptor(t, groupsProtoFileContent) + sourceLocations := fileDescriptor.SourceLocations() + fooMessageDescriptor := fileDescriptor.Messages().ByName("Foo") + require.NotNil(t, fooMessageDescriptor) + nestedMessageDescriptor := fooMessageDescriptor.Messages().ByName("Nested") + require.NotNil(t, nestedMessageDescriptor) + for _, testCase := range []struct { + name string + fieldDescriptor protoreflect.FieldDescriptor + expectedSyntheticMessageName protoreflect.FullName + expectedNoSyntheticMessageFound bool + }{ + { + name: "group field", + fieldDescriptor: fooMessageDescriptor.Fields().ByName("group"), + expectedSyntheticMessageName: "a.Foo.Group", + }, + { + name: "group field in nested message", + fieldDescriptor: nestedMessageDescriptor.Fields().ByName("nestedgroup"), + expectedSyntheticMessageName: "a.Foo.Nested.NestedGroup", + }, + { + name: "group extension field in message", + fieldDescriptor: fooMessageDescriptor.Extensions().ByName("messageextensiongroup"), + expectedSyntheticMessageName: "a.Foo.MessageExtensionGroup", + }, + { + name: "group extension field in file", + fieldDescriptor: fileDescriptor.Extensions().ByName("fileextensiongroup"), + expectedSyntheticMessageName: "a.FileExtensionGroup", + }, + { + name: "non-group field", + fieldDescriptor: fooMessageDescriptor.Fields().ByName("scalar"), + expectedNoSyntheticMessageFound: true, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + require.NotNil(t, testCase.fieldDescriptor) + sourcePath := sourceLocations.ByDescriptor(testCase.fieldDescriptor).Path + require.NotEmpty(t, sourcePath) + syntheticMessageDescriptor := groupFieldSyntheticMessage(fileDescriptor, sourcePath) + if testCase.expectedNoSyntheticMessageFound { + require.Nil(t, syntheticMessageDescriptor) + return + } + require.NotNil(t, syntheticMessageDescriptor) + require.Equal(t, testCase.expectedSyntheticMessageName, syntheticMessageDescriptor.FullName()) + }) + } +} + +func TestGroupFieldSyntheticMessageNonFieldSourcePaths(t *testing.T) { + t.Parallel() + fileDescriptor := testCompileFileDescriptor(t, groupsProtoFileContent) + for _, sourcePath := range []protoreflect.SourcePath{ + nil, + {}, + // .package. + {2}, + // .message_type(0). + {4, 0}, + // .message_type(0).field(0).name. + {4, 0, 2, 0, 1}, + // .message_type(0).nested_type(0). + {4, 0, 3, 0}, + // .message_type(0).enum_type(0). + {4, 0, 4, 0}, + // Out of range indexes. + {4, 100, 2, 0}, + {4, 0, 2, 100}, + {7, 100}, + // Negative indexes. + {4, -1, 2, 0}, + {4, 0, 2, -1}, + } { + require.Nil( + t, + groupFieldSyntheticMessage(fileDescriptor, sourcePath), + "source path %v", sourcePath, + ) + } +} + +func testCompileFileDescriptor(t *testing.T, fileContent string) protoreflect.FileDescriptor { + t.Helper() + compiler := protocompile.Compiler{ + Resolver: &protocompile.SourceResolver{ + Accessor: protocompile.SourceAccessorFromMap(map[string]string{"a.proto": fileContent}), + }, + SourceInfoMode: protocompile.SourceInfoStandard, + } + files, err := compiler.Compile(context.Background(), "a.proto") + require.NoError(t, err) + require.Len(t, files, 1) + return files[0] +} diff --git a/private/bufpkg/bufcheck/lint_test.go b/private/bufpkg/bufcheck/lint_test.go index 2239a7b7da..cdd5860d60 100644 --- a/private/bufpkg/bufcheck/lint_test.go +++ b/private/bufpkg/bufcheck/lint_test.go @@ -1310,6 +1310,16 @@ func TestCommentIgnoresOnlyRule(t *testing.T) { ) } +func TestCommentIgnoresGroup(t *testing.T) { + t.Parallel() + testLint( + t, + "comment_ignores_group", + bufanalysistesting.NewFileAnnotation(t, "a.proto", 10, 18, 10, 28, "FIELD_NOT_REQUIRED"), + bufanalysistesting.NewFileAnnotation(t, "a.proto", 18, 20, 18, 30, "FIELD_NOT_REQUIRED"), + ) +} + func TestCommentIgnoresWithTrailingComment(t *testing.T) { t.Parallel() testLint( diff --git a/private/bufpkg/bufcheck/testdata/lint/comment_ignores_group/a.proto b/private/bufpkg/bufcheck/testdata/lint/comment_ignores_group/a.proto new file mode 100644 index 0000000000..b4bf11a1e3 --- /dev/null +++ b/private/bufpkg/bufcheck/testdata/lint/comment_ignores_group/a.proto @@ -0,0 +1,22 @@ +syntax = "proto2"; + +package a; + +message Foo { + // buf:lint:ignore FIELD_NOT_REQUIRED + required group Ignored = 1 { + optional string id = 1; + } + required group NotIgnored = 2 { + optional string id = 1; + } + message Nested { + // buf:lint:ignore FIELD_NOT_REQUIRED + required group Ignored = 1 { + optional string id = 1; + } + required group NotIgnored = 2 { + optional string id = 1; + } + } +} diff --git a/private/bufpkg/bufcheck/testdata/lint/comment_ignores_group/buf.yaml b/private/bufpkg/bufcheck/testdata/lint/comment_ignores_group/buf.yaml new file mode 100644 index 0000000000..866fbf0995 --- /dev/null +++ b/private/bufpkg/bufcheck/testdata/lint/comment_ignores_group/buf.yaml @@ -0,0 +1,4 @@ +version: v2 +lint: + use: + - FIELD_NOT_REQUIRED From fd76f739c04273873c6dac67ebaa2cfd485aa755 Mon Sep 17 00:00:00 2001 From: Anuraag Agrawal Date: Wed, 16 Sep 2026 17:59:19 +0900 Subject: [PATCH 2/4] lint --- private/bufpkg/bufcheck/client_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/private/bufpkg/bufcheck/client_test.go b/private/bufpkg/bufcheck/client_test.go index 123228664b..b165ced211 100644 --- a/private/bufpkg/bufcheck/client_test.go +++ b/private/bufpkg/bufcheck/client_test.go @@ -15,7 +15,6 @@ package bufcheck import ( - "context" "testing" "github.com/bufbuild/protocompile" @@ -151,7 +150,7 @@ func testCompileFileDescriptor(t *testing.T, fileContent string) protoreflect.Fi }, SourceInfoMode: protocompile.SourceInfoStandard, } - files, err := compiler.Compile(context.Background(), "a.proto") + files, err := compiler.Compile(t.Context(), "a.proto") require.NoError(t, err) require.Len(t, files, 1) return files[0] From cc19f65cfcdafe92ad01f937846728d94ecd623d Mon Sep 17 00:00:00 2001 From: "Anuraag (Rag) Agrawal" Date: Thu, 17 Sep 2026 10:17:03 +0900 Subject: [PATCH 3/4] Update private/bufpkg/bufcheck/client.go Co-authored-by: Edward McFarlane <3036610+emcfarlane@users.noreply.github.com> --- private/bufpkg/bufcheck/client.go | 104 ++++++++++++++++-------------- 1 file changed, 56 insertions(+), 48 deletions(-) diff --git a/private/bufpkg/bufcheck/client.go b/private/bufpkg/bufcheck/client.go index 6c5e00f7a9..182d4f8563 100644 --- a/private/bufpkg/bufcheck/client.go +++ b/private/bufpkg/bufcheck/client.go @@ -843,21 +843,18 @@ func ignoreFileLocation( return false, nil } -// groupFieldSyntheticMessage returns the synthetic message declaration for the group field -// at the given source path, or nil if the source path does not point to a group field. func groupFieldSyntheticMessage( fileDescriptor protoreflect.FileDescriptor, sourcePath protoreflect.SourcePath, ) protoreflect.MessageDescriptor { - fieldDescriptor := fieldDescriptorForSourcePath(fileDescriptor, sourcePath) - if fieldDescriptor == nil || fieldDescriptor.Kind() != protoreflect.GroupKind { + fieldDescriptor, ok := descriptorForSourcePath(fileDescriptor, sourcePath).(protoreflect.FieldDescriptor) + if !ok || fieldDescriptor.Kind() != protoreflect.GroupKind { return nil } return fieldDescriptor.Message() } -// Source path tags for the descriptor fields traversed when resolving a source path to a -// field declaration. +// Source path tags for the declarations traversed when resolving a source path. const ( // FileDescriptorProto.message_type. fileMessagesTag = int32(4) @@ -871,59 +868,70 @@ const ( messageExtensionsTag = int32(6) ) -// fieldDescriptorForSourcePath returns the field declaration at the given source path, or -// nil if the source path does not point to a field declaration. +// descriptorList is the shape shared by protoreflect's descriptor list types, such as +// protoreflect.MessageDescriptors and protoreflect.FieldDescriptors. +type descriptorList[D protoreflect.Descriptor] interface { + Len() int + Get(i int) D +} + +// descriptorAtIndex returns the descriptor at the given index, or nil if the index is out +// of range. +func descriptorAtIndex[D protoreflect.Descriptor, L descriptorList[D]]( + descriptors L, + index int, +) protoreflect.Descriptor { + if index < 0 || index >= descriptors.Len() { + return nil + } + return descriptors.Get(index) +} + +// descriptorForSourcePath returns the declaration at the given source path, or nil if the +// source path does not point to a declaration. // -// A source path for a field declaration alternates a tag and an index, descending through -// message declarations before terminating at a field or an extension field, for example -// [4, 0, 3, 1, 2, 0] for .message_type(0).nested_type(1).field(0). -func fieldDescriptorForSourcePath( +// A source path alternates a tag and an index, descending through the declarations of a +// file, for example [4, 0, 3, 1, 2, 0] for .message_type(0).nested_type(1).field(0). Only +// the tags needed to reach a field or extension declaration are resolved. +func descriptorForSourcePath( fileDescriptor protoreflect.FileDescriptor, sourcePath protoreflect.SourcePath, -) protoreflect.FieldDescriptor { - if len(sourcePath) < 2 || len(sourcePath)%2 != 0 { +) protoreflect.Descriptor { + if len(sourcePath) == 0 || len(sourcePath)%2 != 0 { return nil } - // The message that the declaration at the end of the source path belongs to, or nil if - // the declaration is at the top level of the file. - var parentMessageDescriptor protoreflect.MessageDescriptor - for i := 0; i < len(sourcePath)-2; i += 2 { - tag, index := sourcePath[i], int(sourcePath[i+1]) - var messageDescriptors protoreflect.MessageDescriptors - switch { - case parentMessageDescriptor == nil && tag == fileMessagesTag: - messageDescriptors = fileDescriptor.Messages() - case parentMessageDescriptor != nil && tag == messageNestedMessagesTag: - messageDescriptors = parentMessageDescriptor.Messages() + descriptor := protoreflect.Descriptor(fileDescriptor) + for ; len(sourcePath) > 0; sourcePath = sourcePath[2:] { + tag, index := sourcePath[0], int(sourcePath[1]) + switch typedDescriptor := descriptor.(type) { + case protoreflect.FileDescriptor: + switch tag { + case fileMessagesTag: + descriptor = descriptorAtIndex(typedDescriptor.Messages(), index) + case fileExtensionsTag: + descriptor = descriptorAtIndex(typedDescriptor.Extensions(), index) + default: + return nil + } + case protoreflect.MessageDescriptor: + switch tag { + case messageNestedMessagesTag: + descriptor = descriptorAtIndex(typedDescriptor.Messages(), index) + case messageFieldsTag: + descriptor = descriptorAtIndex(typedDescriptor.Fields(), index) + case messageExtensionsTag: + descriptor = descriptorAtIndex(typedDescriptor.Extensions(), index) + default: + return nil + } default: return nil } - if index < 0 || index >= messageDescriptors.Len() { + if descriptor == nil { return nil } - parentMessageDescriptor = messageDescriptors.Get(index) - } - tag, index := sourcePath[len(sourcePath)-2], int(sourcePath[len(sourcePath)-1]) - if index < 0 { - return nil - } - if parentMessageDescriptor == nil { - if tag == fileExtensionsTag && index < fileDescriptor.Extensions().Len() { - return fileDescriptor.Extensions().Get(index) - } - return nil - } - switch tag { - case messageFieldsTag: - if index < parentMessageDescriptor.Fields().Len() { - return parentMessageDescriptor.Fields().Get(index) - } - case messageExtensionsTag: - if index < parentMessageDescriptor.Extensions().Len() { - return parentMessageDescriptor.Extensions().Get(index) - } } - return nil + return descriptor } // leadingCommentsHaveCheckIgnore checks if any line of the given leading comments is a From fb297484c5811b4892441bfc1e4063ff8736ae16 Mon Sep 17 00:00:00 2001 From: Anuraag Agrawal Date: Thu, 17 Sep 2026 10:35:29 +0900 Subject: [PATCH 4/4] protosourcepath --- private/bufpkg/bufcheck/client.go | 87 +----------- private/bufpkg/bufcheck/client_test.go | 24 ++-- private/pkg/protosourcepath/README.md | 18 ++- private/pkg/protosourcepath/descriptor.go | 117 +++++++++++++++++ .../pkg/protosourcepath/descriptor_test.go | 124 ++++++++++++++++++ .../protosourcepath/protosourcepath_test.go | 29 ++-- .../testdata/descriptor/test.proto | 35 +++++ 7 files changed, 321 insertions(+), 113 deletions(-) create mode 100644 private/pkg/protosourcepath/descriptor.go create mode 100644 private/pkg/protosourcepath/descriptor_test.go create mode 100644 private/pkg/protosourcepath/testdata/descriptor/test.proto diff --git a/private/bufpkg/bufcheck/client.go b/private/bufpkg/bufcheck/client.go index 182d4f8563..dfd6c9fdf4 100644 --- a/private/bufpkg/bufcheck/client.go +++ b/private/bufpkg/bufcheck/client.go @@ -843,97 +843,19 @@ func ignoreFileLocation( return false, nil } +// groupFieldSyntheticMessage returns the synthetic message declaration for the group field +// at the given source path, or nil if the source path does not point to a group field. func groupFieldSyntheticMessage( fileDescriptor protoreflect.FileDescriptor, sourcePath protoreflect.SourcePath, ) protoreflect.MessageDescriptor { - fieldDescriptor, ok := descriptorForSourcePath(fileDescriptor, sourcePath).(protoreflect.FieldDescriptor) + fieldDescriptor, ok := protosourcepath.DescriptorForSourcePath(fileDescriptor, sourcePath).(protoreflect.FieldDescriptor) if !ok || fieldDescriptor.Kind() != protoreflect.GroupKind { return nil } return fieldDescriptor.Message() } -// Source path tags for the declarations traversed when resolving a source path. -const ( - // FileDescriptorProto.message_type. - fileMessagesTag = int32(4) - // FileDescriptorProto.extension. - fileExtensionsTag = int32(7) - // DescriptorProto.field. - messageFieldsTag = int32(2) - // DescriptorProto.nested_type. - messageNestedMessagesTag = int32(3) - // DescriptorProto.extension. - messageExtensionsTag = int32(6) -) - -// descriptorList is the shape shared by protoreflect's descriptor list types, such as -// protoreflect.MessageDescriptors and protoreflect.FieldDescriptors. -type descriptorList[D protoreflect.Descriptor] interface { - Len() int - Get(i int) D -} - -// descriptorAtIndex returns the descriptor at the given index, or nil if the index is out -// of range. -func descriptorAtIndex[D protoreflect.Descriptor, L descriptorList[D]]( - descriptors L, - index int, -) protoreflect.Descriptor { - if index < 0 || index >= descriptors.Len() { - return nil - } - return descriptors.Get(index) -} - -// descriptorForSourcePath returns the declaration at the given source path, or nil if the -// source path does not point to a declaration. -// -// A source path alternates a tag and an index, descending through the declarations of a -// file, for example [4, 0, 3, 1, 2, 0] for .message_type(0).nested_type(1).field(0). Only -// the tags needed to reach a field or extension declaration are resolved. -func descriptorForSourcePath( - fileDescriptor protoreflect.FileDescriptor, - sourcePath protoreflect.SourcePath, -) protoreflect.Descriptor { - if len(sourcePath) == 0 || len(sourcePath)%2 != 0 { - return nil - } - descriptor := protoreflect.Descriptor(fileDescriptor) - for ; len(sourcePath) > 0; sourcePath = sourcePath[2:] { - tag, index := sourcePath[0], int(sourcePath[1]) - switch typedDescriptor := descriptor.(type) { - case protoreflect.FileDescriptor: - switch tag { - case fileMessagesTag: - descriptor = descriptorAtIndex(typedDescriptor.Messages(), index) - case fileExtensionsTag: - descriptor = descriptorAtIndex(typedDescriptor.Extensions(), index) - default: - return nil - } - case protoreflect.MessageDescriptor: - switch tag { - case messageNestedMessagesTag: - descriptor = descriptorAtIndex(typedDescriptor.Messages(), index) - case messageFieldsTag: - descriptor = descriptorAtIndex(typedDescriptor.Fields(), index) - case messageExtensionsTag: - descriptor = descriptorAtIndex(typedDescriptor.Extensions(), index) - default: - return nil - } - default: - return nil - } - if descriptor == nil { - return nil - } - } - return descriptor -} - // leadingCommentsHaveCheckIgnore checks if any line of the given leading comments is a // comment ignore for the given rule. func leadingCommentsHaveCheckIgnore( @@ -941,9 +863,6 @@ func leadingCommentsHaveCheckIgnore( commentIgnorePrefix string, ruleID string, ) bool { - if leadingComments == "" { - return false - } for _, line := range xstrings.SplitTrimLinesNoEmpty(leadingComments) { if checkCommentLineForCheckIgnore(line, commentIgnorePrefix, ruleID) { return true diff --git a/private/bufpkg/bufcheck/client_test.go b/private/bufpkg/bufcheck/client_test.go index b165ced211..c0e19d32ed 100644 --- a/private/bufpkg/bufcheck/client_test.go +++ b/private/bufpkg/bufcheck/client_test.go @@ -110,29 +110,21 @@ func TestGroupFieldSyntheticMessage(t *testing.T) { } } -func TestGroupFieldSyntheticMessageNonFieldSourcePaths(t *testing.T) { +// Source path traversal itself is covered by protosourcepath, this covers the source paths +// that resolve to something other than a group field. +func TestGroupFieldSyntheticMessageNonGroupFieldSourcePaths(t *testing.T) { t.Parallel() fileDescriptor := testCompileFileDescriptor(t, groupsProtoFileContent) for _, sourcePath := range []protoreflect.SourcePath{ + // The file. nil, {}, - // .package. - {2}, - // .message_type(0). + // .message_type(0), a message. {4, 0}, - // .message_type(0).field(0).name. - {4, 0, 2, 0, 1}, - // .message_type(0).nested_type(0). + // .message_type(0).nested_type(0), the synthetic message of a group field. {4, 0, 3, 0}, - // .message_type(0).enum_type(0). - {4, 0, 4, 0}, - // Out of range indexes. - {4, 100, 2, 0}, - {4, 0, 2, 100}, - {7, 100}, - // Negative indexes. - {4, -1, 2, 0}, - {4, 0, 2, -1}, + // .message_type(0).field(0).name, an attribute of a field. + {4, 0, 2, 0, 1}, } { require.Nil( t, diff --git a/private/pkg/protosourcepath/README.md b/private/pkg/protosourcepath/README.md index 718828e7d1..7a173397db 100644 --- a/private/pkg/protosourcepath/README.md +++ b/private/pkg/protosourcepath/README.md @@ -80,8 +80,8 @@ Details examples for associated paths can be found through the tests. ## API -There is a single function, `GetAssociatedSourcePaths`, that takes a `protoreflect.SourcePath` -and returns a list of associated paths. +`GetAssociatedSourcePaths` takes a `protoreflect.SourcePath` and returns a list of associated +paths. ```go func GetAssociatedSourcePaths( @@ -91,6 +91,20 @@ func GetAssociatedSourcePaths( We expect there always to be at least one associated path, the path itself. +`DescriptorForSourcePath` resolves a `protoreflect.SourcePath` against a file to the descriptor +of the Protobuf declaration that the source path points to. + +```go +func DescriptorForSourcePath( + fileDescriptor protoreflect.FileDescriptor, + sourcePath protoreflect.SourcePath, +) protoreflect.Descriptor +``` + +Source paths that point to an attribute of a declaration rather than to the declaration itself, +such as `[4, 0, 1]` for `.message_type(0).name`, return `nil`. `GetAssociatedSourcePaths` can be +used to resolve such a source path to the source paths of the declarations it belongs to. + ## Future We are currently returning all associated source paths, but we have the option to exclude diff --git a/private/pkg/protosourcepath/descriptor.go b/private/pkg/protosourcepath/descriptor.go new file mode 100644 index 0000000000..c011768070 --- /dev/null +++ b/private/pkg/protosourcepath/descriptor.go @@ -0,0 +1,117 @@ +// Copyright 2020-2026 Buf Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package protosourcepath + +import ( + "google.golang.org/protobuf/reflect/protoreflect" +) + +// DescriptorForSourcePath returns the descriptor of the Protobuf declaration that the given +// source path points to, or nil if the source path does not point to a declaration. +// +// A source path to a declaration alternates a type tag and an index, descending from the +// FileDescriptorProto through the declarations that contain it. For example, the source path +// [4, 0, 3, 1, 2, 0] is .message_type(0).nested_type(1).field(0), the first field of the +// second nested message of the first message in the file. The empty source path points to the +// file itself, so the given file descriptor is returned for it. +// +// A source path that points to an attribute of a declaration rather than to the declaration +// itself, such as [4, 0, 1] for .message_type(0).name, returns nil. GetAssociatedSourcePaths +// can be used to resolve such a source path to the source paths of the declarations it +// belongs to. +func DescriptorForSourcePath( + fileDescriptor protoreflect.FileDescriptor, + sourcePath protoreflect.SourcePath, +) protoreflect.Descriptor { + if len(sourcePath)%2 != 0 { + // Each declaration is addressed by a type tag and an index, so a source path with an + // odd number of elements cannot point to a declaration. + return nil + } + descriptor := protoreflect.Descriptor(fileDescriptor) + for ; len(sourcePath) > 0; sourcePath = sourcePath[2:] { + typeTag, index := sourcePath[0], int(sourcePath[1]) + // The same type tag means different things depending on the descriptor it is read + // against, so the descriptor we have descended to so far selects the tags to check. + switch typedDescriptor := descriptor.(type) { + case protoreflect.FileDescriptor: + switch typeTag { + case messagesTypeTag: + descriptor = descriptorAtIndex(typedDescriptor.Messages(), index) + case enumsTypeTag: + descriptor = descriptorAtIndex(typedDescriptor.Enums(), index) + case servicesTypeTag: + descriptor = descriptorAtIndex(typedDescriptor.Services(), index) + case extensionsTypeTag: + descriptor = descriptorAtIndex(typedDescriptor.Extensions(), index) + default: + return nil + } + case protoreflect.MessageDescriptor: + switch typeTag { + case messageFieldsTypeTag: + descriptor = descriptorAtIndex(typedDescriptor.Fields(), index) + case nestedMessagesTypeTag: + descriptor = descriptorAtIndex(typedDescriptor.Messages(), index) + case nestedEnumsTypeTag: + descriptor = descriptorAtIndex(typedDescriptor.Enums(), index) + case messageExtensionsTypeTag: + descriptor = descriptorAtIndex(typedDescriptor.Extensions(), index) + case messageOneOfsTypeTag: + descriptor = descriptorAtIndex(typedDescriptor.Oneofs(), index) + default: + return nil + } + case protoreflect.EnumDescriptor: + if typeTag != enumValuesTypeTag { + return nil + } + descriptor = descriptorAtIndex(typedDescriptor.Values(), index) + case protoreflect.ServiceDescriptor: + if typeTag != serviceMethodsTypeTag { + return nil + } + descriptor = descriptorAtIndex(typedDescriptor.Methods(), index) + default: + // Fields, oneofs, enum values, and methods do not contain declarations. + return nil + } + if descriptor == nil { + return nil + } + } + return descriptor +} + +// *** PRIVATE *** + +// descriptorList is the shape shared by protoreflect's descriptor list types, such as +// protoreflect.MessageDescriptors and protoreflect.FieldDescriptors. +type descriptorList[D protoreflect.Descriptor] interface { + Len() int + Get(i int) D +} + +// descriptorAtIndex returns the descriptor at the given index, or nil if the index is out +// of range. +func descriptorAtIndex[D protoreflect.Descriptor, L descriptorList[D]]( + descriptors L, + index int, +) protoreflect.Descriptor { + if index < 0 || index >= descriptors.Len() { + return nil + } + return descriptors.Get(index) +} diff --git a/private/pkg/protosourcepath/descriptor_test.go b/private/pkg/protosourcepath/descriptor_test.go new file mode 100644 index 0000000000..ec9bac12fe --- /dev/null +++ b/private/pkg/protosourcepath/descriptor_test.go @@ -0,0 +1,124 @@ +// Copyright 2020-2026 Buf Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package protosourcepath + +import ( + "testing" + + "github.com/stretchr/testify/require" + "google.golang.org/protobuf/reflect/protoreflect" +) + +func TestDescriptorForSourcePath(t *testing.T) { + t.Parallel() + fileDescriptor := testBuildFileDescriptor(t, "testdata/descriptor/test.proto") + for _, testCase := range []struct { + sourcePath protoreflect.SourcePath + expectedDescriptorType string + expectedDescriptorName protoreflect.FullName + }{ + // The file itself. + {protoreflect.SourcePath{}, "file", "foo"}, + // .service(0), .service(0).method(0). + {protoreflect.SourcePath{6, 0}, "service", "foo.Service"}, + {protoreflect.SourcePath{6, 0, 2, 0}, "method", "foo.Service.Method"}, + // .enum_type(0), .enum_type(0).value(0). + {protoreflect.SourcePath{5, 0}, "enum", "foo.Enum"}, + {protoreflect.SourcePath{5, 0, 2, 0}, "enum value", "foo.ENUM_UNSPECIFIED"}, + // .message_type(0) and its fields and oneof. + {protoreflect.SourcePath{4, 0}, "message", "foo.Message"}, + {protoreflect.SourcePath{4, 0, 2, 0}, "field", "foo.Message.field"}, + {protoreflect.SourcePath{4, 0, 8, 0}, "oneof", "foo.Message.one_of"}, + {protoreflect.SourcePath{4, 0, 2, 1}, "field", "foo.Message.one_of_field"}, + // The group field and the synthetic message holding the group's fields. + {protoreflect.SourcePath{4, 0, 2, 2}, "field", "foo.Message.group"}, + {protoreflect.SourcePath{4, 0, 3, 0}, "message", "foo.Message.Group"}, + {protoreflect.SourcePath{4, 0, 3, 0, 2, 0}, "field", "foo.Message.Group.id"}, + // .message_type(0).nested_type(1) and .message_type(0).enum_type(0). + {protoreflect.SourcePath{4, 0, 3, 1}, "message", "foo.Message.NestedMessage"}, + {protoreflect.SourcePath{4, 0, 3, 1, 2, 0}, "field", "foo.Message.NestedMessage.field"}, + {protoreflect.SourcePath{4, 0, 4, 0}, "enum", "foo.Message.NestedEnum"}, + {protoreflect.SourcePath{4, 0, 4, 0, 2, 0}, "enum value", "foo.Message.NESTED_ENUM_UNSPECIFIED"}, + // Extensions declared in a message and in the file. + {protoreflect.SourcePath{4, 0, 6, 0}, "field", "foo.Message.message_extension"}, + {protoreflect.SourcePath{7, 0}, "field", "foo.file_extension"}, + } { + descriptor := DescriptorForSourcePath(fileDescriptor, testCase.sourcePath) + require.NotNil(t, descriptor, testCase.sourcePath) + require.Equal(t, testCase.expectedDescriptorType, testDescriptorTypeName(t, descriptor), testCase.sourcePath) + require.Equal(t, testCase.expectedDescriptorName, descriptor.FullName(), testCase.sourcePath) + } +} + +func TestDescriptorForSourcePathNotADeclaration(t *testing.T) { + t.Parallel() + fileDescriptor := testBuildFileDescriptor(t, "testdata/descriptor/test.proto") + for _, sourcePath := range []protoreflect.SourcePath{ + // Attributes of a declaration rather than a declaration. + // .syntax, .package, .message_type(0).name. + {12}, + {2}, + {4, 0, 1}, + // Declarations without a descriptor. + // .dependency(0), .message_type(0).extension_range(0), .message_type(0).reserved_range(0). + {3, 0}, + {4, 0, 5, 0}, + {4, 0, 9, 0}, + // Descending into declarations that do not contain declarations. + // A field, a oneof, an enum value, and a method. + {4, 0, 2, 0, 2, 0}, + {4, 0, 8, 0, 2, 0}, + {5, 0, 2, 0, 2, 0}, + {6, 0, 2, 0, 2, 0}, + // Out of range indexes. + {4, 100}, + {4, 0, 2, 100}, + {4, 0, 3, 100}, + {5, 100}, + {6, 100}, + {7, 100}, + // Negative indexes. + {4, -1}, + {4, 0, 2, -1}, + } { + require.Nil(t, DescriptorForSourcePath(fileDescriptor, sourcePath), sourcePath) + } +} + +// testDescriptorTypeName returns a name for the type of the given descriptor, so that tests +// can assert the kind of declaration that was resolved and not just its name. +func testDescriptorTypeName(t *testing.T, descriptor protoreflect.Descriptor) string { + t.Helper() + switch descriptor.(type) { + case protoreflect.FileDescriptor: + return "file" + case protoreflect.MessageDescriptor: + return "message" + case protoreflect.FieldDescriptor: + return "field" + case protoreflect.OneofDescriptor: + return "oneof" + case protoreflect.EnumDescriptor: + return "enum" + case protoreflect.EnumValueDescriptor: + return "enum value" + case protoreflect.ServiceDescriptor: + return "service" + case protoreflect.MethodDescriptor: + return "method" + } + t.Fatalf("unknown descriptor type %T", descriptor) + return "" +} diff --git a/private/pkg/protosourcepath/protosourcepath_test.go b/private/pkg/protosourcepath/protosourcepath_test.go index 7d2ca903ef..56ab31619c 100644 --- a/private/pkg/protosourcepath/protosourcepath_test.go +++ b/private/pkg/protosourcepath/protosourcepath_test.go @@ -384,6 +384,22 @@ func testGetAssociatedSourcePaths( sourcePathToExpectedAssociatedPaths map[string][]protoreflect.SourcePath, excludeChildAssociatedPaths bool, ) { + fileDescriptor := testBuildFileDescriptor(t, testFilePath) + sourceLocations := fileDescriptor.SourceLocations() + // SourceLocations are indexed starting from 1 + for i := 1; i < sourceLocations.Len(); i++ { + sourceLocation := sourceLocations.Get(i) + associatedSourcePaths, err := getAssociatedSourcePaths(sourceLocation.Path, excludeChildAssociatedPaths) + require.NoError(t, err) + expectedAssociatedPaths, ok := sourcePathToExpectedAssociatedPaths[sourceLocation.Path.String()] + require.True(t, ok, sourceLocation.Path) + require.Equal(t, expectedAssociatedPaths, associatedSourcePaths, i) + } +} + +// testBuildFileDescriptor builds the file descriptor for the given test file path, including +// source code info. +func testBuildFileDescriptor(t *testing.T, testFilePath string) protoreflect.FileDescriptor { var fdpOptions fdp.Options fdpOptions.Apply(fdp.IncludeSourceCodeInfo(true)) results, _, err := incremental.Run(t.Context(), incremental.New(), queries.FDS{ @@ -403,16 +419,7 @@ func testGetAssociatedSourcePaths( require.NoError(t, protoencoding.NewWireUnmarshaler(nil).Unmarshal(fdsBytes, fds)) resolver, err := protoencoding.NewResolver(fds.File...) require.NoError(t, err) - fd, err := resolver.FindFileByPath(testFilePath) + fileDescriptor, err := resolver.FindFileByPath(testFilePath) require.NoError(t, err) - sourceLocations := fd.SourceLocations() - // SourceLocations are indexed starting from 1 - for i := 1; i < sourceLocations.Len(); i++ { - sourceLocation := sourceLocations.Get(i) - associatedSourcePaths, err := getAssociatedSourcePaths(sourceLocation.Path, excludeChildAssociatedPaths) - require.NoError(t, err) - expectedAssociatedPaths, ok := sourcePathToExpectedAssociatedPaths[sourceLocation.Path.String()] - require.True(t, ok, sourceLocation.Path) - require.Equal(t, expectedAssociatedPaths, associatedSourcePaths, i) - } + return fileDescriptor } diff --git a/private/pkg/protosourcepath/testdata/descriptor/test.proto b/private/pkg/protosourcepath/testdata/descriptor/test.proto new file mode 100644 index 0000000000..4c15bb6fd0 --- /dev/null +++ b/private/pkg/protosourcepath/testdata/descriptor/test.proto @@ -0,0 +1,35 @@ +syntax = "proto2"; + +package foo; + +service Service { + rpc Method(Message) returns (Message); +} + +enum Enum { + ENUM_UNSPECIFIED = 0; +} + +message Message { + optional string field = 1; + oneof one_of { + string one_of_field = 2; + } + optional group Group = 3 { + optional string id = 1; + } + message NestedMessage { + optional string field = 1; + } + enum NestedEnum { + NESTED_ENUM_UNSPECIFIED = 0; + } + extensions 100 to 200; + extend Message { + optional string message_extension = 100; + } +} + +extend Message { + optional string file_extension = 101; +}