Split out common types into descriptor and option packages (#2)

Author: bufdev

.golangci.yml

@@ -55,7 +55,7 @@ issues:
       test: "CheckServiceHandlerOption"
     - linters:
         - exhaustive
-      path: check/options.go
+      path: option/options.go
       text: "reflect.Pointer|reflect.Ptr"
     - linters:
         - gocritic
@@ -101,3 +101,10 @@ issues:
     - linters:
         - varnamelen
       path: internal/pkg/xslices/xslices.go
+    - linters:
+        - revive
+      path: internal/pkg/compare/compare.go
+    - linters:
+        - gosec
+      path: check/checktest/checktest.go
+      text: "G115:"

Makefile

@@ -12,9 +12,9 @@ export GOBIN := $(abspath $(BIN))
 COPYRIGHT_YEARS := 2024
 LICENSE_IGNORE := --ignore testdata/
 
-BUF_VERSION := v1.39.0
-GO_MOD_GOTOOLCHAIN := go1.23.0
-GOLANGCI_LINT_VERSION := v1.60.1
+BUF_VERSION := v1.42.0
+GO_MOD_GOTOOLCHAIN := go1.23.1
+GOLANGCI_LINT_VERSION := v1.60.3
 # https://github.com/golangci/golangci-lint/issues/4837
 GOLANGCI_LINT_GOTOOLCHAIN := $(GO_MOD_GOTOOLCHAIN)
 # Remove when we want to upgrade past Go 1.21

check/annotation.go

@@ -19,13 +19,15 @@ import (
 	"sort"
 
 	checkv1 "buf.build/gen/go/bufbuild/bufplugin/protocolbuffers/go/buf/plugin/check/v1"
+	descriptorv1 "buf.build/gen/go/bufbuild/bufplugin/protocolbuffers/go/buf/plugin/descriptor/v1"
+	"buf.build/go/bufplugin/descriptor"
 )
 
 // Annotation represents a rule Failure.
 //
 // An annotation always contains the ID of the Rule that failed. It also optionally
 // contains a user-readable message, a location of the failure, and a location of the
-// failure in the against Files.
+// failure in the against FileDescriptors.
 //
 // Annotations are created on the server-side via ResponseWriters, and returned
 // from Clients on Responses.
@@ -36,12 +38,12 @@ type Annotation interface {
 	RuleID() string
 	// Message is a user-readable message describing the failure.
 	Message() string
-	// Location is the location of the failure.
-	Location() Location
-	// AgainstLocation is the Location of the failure in the against Files.
+	// FileLocation is the location of the failure.
+	FileLocation() descriptor.FileLocation
+	// AgainstFileLocation is the FileLocation of the failure in the against FileDescriptors.
 	//
 	// Will only potentially be produced for breaking change rules.
-	AgainstLocation() Location
+	AgainstFileLocation() descriptor.FileLocation
 
 	toProto() *checkv1.Annotation
 
@@ -51,26 +53,26 @@ type Annotation interface {
 // *** PRIVATE ***
 
 type annotation struct {
-	ruleID          string
-	message         string
-	location        Location
-	againstLocation Location
+	ruleID              string
+	message             string
+	fileLocation        descriptor.FileLocation
+	againstFileLocation descriptor.FileLocation
 }
 
 func newAnnotation(
 	ruleID string,
 	message string,
-	location Location,
-	againstLocation Location,
+	fileLocation descriptor.FileLocation,
+	againstFileLocation descriptor.FileLocation,
 ) (*annotation, error) {
 	if ruleID == "" {
 		return nil, errors.New("check.Annotation: RuleID is empty")
 	}
 	return &annotation{
-		ruleID:          ruleID,
-		message:         message,
-		location:        location,
-		againstLocation: againstLocation,
+		ruleID:              ruleID,
+		message:             message,
+		fileLocation:        fileLocation,
+		againstFileLocation: againstFileLocation,
 	}, nil
 }
 
@@ -82,31 +84,31 @@ func (a *annotation) Message() string {
 	return a.message
 }
 
-func (a *annotation) Location() Location {
-	return a.location
+func (a *annotation) FileLocation() descriptor.FileLocation {
+	return a.fileLocation
 }
 
-func (a *annotation) AgainstLocation() Location {
-	return a.againstLocation
+func (a *annotation) AgainstFileLocation() descriptor.FileLocation {
+	return a.againstFileLocation
 }
 
 func (a *annotation) toProto() *checkv1.Annotation {
 	if a == nil {
 		return nil
 	}
-	var protoLocation *checkv1.Location
-	if a.location != nil {
-		protoLocation = a.location.toProto()
+	var protoFileLocation *descriptorv1.FileLocation
+	if a.fileLocation != nil {
+		protoFileLocation = a.fileLocation.ToProto()
 	}
-	var protoAgainstLocation *checkv1.Location
-	if a.againstLocation != nil {
-		protoAgainstLocation = a.againstLocation.toProto()
+	var protoAgainstFileLocation *descriptorv1.FileLocation
+	if a.againstFileLocation != nil {
+		protoAgainstFileLocation = a.againstFileLocation.ToProto()
 	}
 	return &checkv1.Annotation{
-		RuleId:          a.RuleID(),
-		Message:         a.Message(),
-		Location:        protoLocation,
-		AgainstLocation: protoAgainstLocation,
+		RuleId:              a.RuleID(),
+		Message:             a.Message(),
+		FileLocation:        protoFileLocation,
+		AgainstFileLocation: protoAgainstFileLocation,
 	}
 }
 

check/check_service_handler_test.go

@@ -19,6 +19,7 @@ import (
 	"testing"
 
 	checkv1 "buf.build/gen/go/bufbuild/bufplugin/protocolbuffers/go/buf/plugin/check/v1"
+	descriptorv1 "buf.build/gen/go/bufbuild/bufplugin/protocolbuffers/go/buf/plugin/descriptor/v1"
 	"github.com/stretchr/testify/require"
 	"google.golang.org/protobuf/proto"
 	"google.golang.org/protobuf/types/descriptorpb"
@@ -40,15 +41,15 @@ func TestCheckServiceHandlerUniqueFiles(t *testing.T) {
 	_, err = checkServiceHandler.Check(
 		context.Background(),
 		&checkv1.CheckRequest{
-			Files: []*checkv1.File{
+			FileDescriptors: []*descriptorv1.FileDescriptor{
 				{
 					FileDescriptorProto: &descriptorpb.FileDescriptorProto{
 						Name:           proto.String("foo.proto"),
 						SourceCodeInfo: &descriptorpb.SourceCodeInfo{},
 					},
 				},
 			},
-			AgainstFiles: []*checkv1.File{
+			AgainstFileDescriptors: []*descriptorv1.FileDescriptor{
 				{
 					FileDescriptorProto: &descriptorpb.FileDescriptorProto{
 						Name:           proto.String("foo.proto"),
@@ -63,7 +64,7 @@ func TestCheckServiceHandlerUniqueFiles(t *testing.T) {
 	_, err = checkServiceHandler.Check(
 		context.Background(),
 		&checkv1.CheckRequest{
-			Files: []*checkv1.File{
+			FileDescriptors: []*descriptorv1.FileDescriptor{
 				{
 					FileDescriptorProto: &descriptorpb.FileDescriptorProto{
 						Name:           proto.String("foo.proto"),
@@ -86,15 +87,15 @@ func TestCheckServiceHandlerUniqueFiles(t *testing.T) {
 	_, err = checkServiceHandler.Check(
 		context.Background(),
 		&checkv1.CheckRequest{
-			Files: []*checkv1.File{
+			FileDescriptors: []*descriptorv1.FileDescriptor{
 				{
 					FileDescriptorProto: &descriptorpb.FileDescriptorProto{
 						Name:           proto.String("foo.proto"),
 						SourceCodeInfo: &descriptorpb.SourceCodeInfo{},
 					},
 				},
 			},
-			AgainstFiles: []*checkv1.File{
+			AgainstFileDescriptors: []*descriptorv1.FileDescriptor{
 				{
 					FileDescriptorProto: &descriptorpb.FileDescriptorProto{
 						Name:           proto.String("bar.proto"),

check/checktest/checktest.go

@@ -25,9 +25,11 @@ import (
 	"strconv"
 	"testing"
 
-	checkv1 "buf.build/gen/go/bufbuild/bufplugin/protocolbuffers/go/buf/plugin/check/v1"
+	descriptorv1 "buf.build/gen/go/bufbuild/bufplugin/protocolbuffers/go/buf/plugin/descriptor/v1"
 	"buf.build/go/bufplugin/check"
+	"buf.build/go/bufplugin/descriptor"
 	"buf.build/go/bufplugin/internal/pkg/xslices"
+	"buf.build/go/bufplugin/option"
 	"github.com/bufbuild/protocompile"
 	"github.com/bufbuild/protocompile/linker"
 	"github.com/bufbuild/protocompile/parser"
@@ -116,25 +118,25 @@ func (r *RequestSpec) ToRequest(ctx context.Context) (check.Request, error) {
 		return nil, errors.New("RequestSpec.Files not set")
 	}
 
-	againstFiles, err := r.AgainstFiles.ToFiles(ctx)
+	againstFileDescriptors, err := r.AgainstFiles.ToFileDescriptors(ctx)
 	if err != nil {
 		return nil, err
 	}
-	options, err := check.NewOptions(r.Options)
+	options, err := option.NewOptions(r.Options)
 	if err != nil {
 		return nil, err
 	}
 	requestOptions := []check.RequestOption{
-		check.WithAgainstFiles(againstFiles),
+		check.WithAgainstFileDescriptors(againstFileDescriptors),
 		check.WithOptions(options),
 		check.WithRuleIDs(r.RuleIDs...),
 	}
 
-	files, err := r.Files.ToFiles(ctx)
+	fileDescriptors, err := r.Files.ToFileDescriptors(ctx)
 	if err != nil {
 		return nil, err
 	}
-	return check.NewRequest(files, requestOptions...)
+	return check.NewRequest(fileDescriptors, requestOptions...)
 }
 
 // ProtoFileSpec specifies files to be compiled for testing.
@@ -160,10 +162,10 @@ type ProtoFileSpec struct {
 	FilePaths []string
 }
 
-// ToFiles compiles the files into check.Files.
+// ToFileDescriptors compiles the files into descriptor.FileDescriptors.
 //
 // If p is nil, this returns an empty slice.
-func (p *ProtoFileSpec) ToFiles(ctx context.Context) ([]check.File, error) {
+func (p *ProtoFileSpec) ToFileDescriptors(ctx context.Context) ([]descriptor.FileDescriptor, error) {
 	if p == nil {
 		return nil, nil
 	}
@@ -185,22 +187,22 @@ type ExpectedAnnotation struct {
 	// against the value in Annotation. That is, it is valid to have an Annotation return
 	// a message but to not set it on ExpectedAnnotation.
 	Message string
-	// Location is the location of the failure.
-	Location *ExpectedLocation
-	// AgainstLocation is the against location of the failure.
-	AgainstLocation *ExpectedLocation
+	// FileLocation is the location of the failure.
+	FileLocation *ExpectedFileLocation
+	// AgainstFileLocation is the against location of the failure.
+	AgainstFileLocation *ExpectedFileLocation
 }
 
 // String implements fmt.Stringer.
 func (ea ExpectedAnnotation) String() string {
 	return "ruleID=\"" + ea.RuleID + "\"" +
 		" message=\"" + ea.Message + "\"" +
-		" location=\"" + ea.Location.String() + "\"" +
-		" againstLocation=\"" + ea.AgainstLocation.String() + "\""
+		" location=\"" + ea.FileLocation.String() + "\"" +
+		" againstLocation=\"" + ea.AgainstFileLocation.String() + "\""
 }
 
-// ExpectedLocation contains the values expected from a Location.
-type ExpectedLocation struct {
+// ExpectedFileLocation contains the values expected from a Location.
+type ExpectedFileLocation struct {
 	// FileName is the name of the file.
 	FileName string
 	// StartLine is the zero-indexed start line.
@@ -214,7 +216,7 @@ type ExpectedLocation struct {
 }
 
 // String implements fmt.Stringer.
-func (el *ExpectedLocation) String() string {
+func (el *ExpectedFileLocation) String() string {
 	if el == nil {
 		return "nil"
 	}
@@ -292,28 +294,28 @@ func expectedAnnotationForAnnotation(annotation check.Annotation) ExpectedAnnota
 		RuleID:  annotation.RuleID(),
 		Message: annotation.Message(),
 	}
-	if location := annotation.Location(); location != nil {
-		expectedAnnotation.Location = &ExpectedLocation{
-			FileName:    location.File().FileDescriptor().Path(),
-			StartLine:   location.StartLine(),
-			StartColumn: location.StartColumn(),
-			EndLine:     location.EndLine(),
-			EndColumn:   location.EndColumn(),
+	if fileLocation := annotation.FileLocation(); fileLocation != nil {
+		expectedAnnotation.FileLocation = &ExpectedFileLocation{
+			FileName:    fileLocation.FileDescriptor().ProtoreflectFileDescriptor().Path(),
+			StartLine:   fileLocation.StartLine(),
+			StartColumn: fileLocation.StartColumn(),
+			EndLine:     fileLocation.EndLine(),
+			EndColumn:   fileLocation.EndColumn(),
 		}
 	}
-	if againstLocation := annotation.AgainstLocation(); againstLocation != nil {
-		expectedAnnotation.AgainstLocation = &ExpectedLocation{
-			FileName:    againstLocation.File().FileDescriptor().Path(),
-			StartLine:   againstLocation.StartLine(),
-			StartColumn: againstLocation.StartColumn(),
-			EndLine:     againstLocation.EndLine(),
-			EndColumn:   againstLocation.EndColumn(),
+	if againstFileLocation := annotation.AgainstFileLocation(); againstFileLocation != nil {
+		expectedAnnotation.AgainstFileLocation = &ExpectedFileLocation{
+			FileName:    againstFileLocation.FileDescriptor().ProtoreflectFileDescriptor().Path(),
+			StartLine:   againstFileLocation.StartLine(),
+			StartColumn: againstFileLocation.StartColumn(),
+			EndLine:     againstFileLocation.EndLine(),
+			EndColumn:   againstFileLocation.EndColumn(),
 		}
 	}
 	return expectedAnnotation
 }
 
-func compile(ctx context.Context, dirPaths []string, filePaths []string) ([]check.File, error) {
+func compile(ctx context.Context, dirPaths []string, filePaths []string) ([]descriptor.FileDescriptor, error) {
 	dirPaths = fromSlashPaths(dirPaths)
 	filePaths = fromSlashPaths(filePaths)
 	toSlashFilePathMap := make(map[string]struct{}, len(filePaths))
@@ -351,22 +353,22 @@ func compile(ctx context.Context, dirPaths []string, filePaths []string) ([]chec
 	}
 	fileDescriptorSet := fileDescriptorSetForFileDescriptors(files)
 
-	protoFiles := make([]*checkv1.File, len(fileDescriptorSet.GetFile()))
+	protoFileDescriptors := make([]*descriptorv1.FileDescriptor, len(fileDescriptorSet.GetFile()))
 	for i, fileDescriptorProto := range fileDescriptorSet.GetFile() {
 		_, isNotImport := toSlashFilePathMap[fileDescriptorProto.GetName()]
 		_, isSyntaxUnspecified := syntaxUnspecifiedFilePaths[fileDescriptorProto.GetName()]
 		unusedDependencyIndexes := unusedDependencyIndexesForFilePathToUnusedDependencyFilePaths(
 			fileDescriptorProto,
 			filePathToUnusedDependencyFilePaths[fileDescriptorProto.GetName()],
 		)
-		protoFiles[i] = &checkv1.File{
+		protoFileDescriptors[i] = &descriptorv1.FileDescriptor{
 			FileDescriptorProto: fileDescriptorProto,
 			IsImport:            !isNotImport,
 			IsSyntaxUnspecified: isSyntaxUnspecified,
 			UnusedDependency:    unusedDependencyIndexes,
 		}
 	}
-	return check.FilesForProtoFiles(protoFiles)
+	return descriptor.FileDescriptorsForProtoFileDescriptors(protoFileDescriptors)
 }
 
 func unusedDependencyIndexesForFilePathToUnusedDependencyFilePaths(