Skip to content

Commit

Permalink
Less slice construction/copying thanks to generics (#2584)
Browse files Browse the repository at this point in the history
Instead of converting a `[]bufimage.ImageFile` or `[]*imagev1.ImageFile`
first to a `[]protodescriptor.FileDescriptor` and then to a
`[]*descriptorpb.FileDescriptorProto`, we can easily elide the first
step using generics.

So instead of "generic" functions accepting a
`[]protodescriptor.FileDescriptor`, they use type arguments, so they
accept any `[]F` where `F` _implements_
`protodescriptor.FileDescriptor`.

This allowed for removal of a few no-longer-needed helper functions.
  • Loading branch information
jhump authored Nov 14, 2023
1 parent d99a5fb commit 82103ef
Show file tree
Hide file tree
Showing 12 changed files with 26 additions and 79 deletions.
6 changes: 1 addition & 5 deletions private/buf/bufwire/image_reader.go
Original file line number Diff line number Diff line change
Expand Up @@ -179,11 +179,7 @@ func (i *imageReader) bootstrapResolver(
}
span.End()
_, newResolverSpan := i.tracer.Start(ctx, "new_resolver")
resolver, err := protoencoding.NewResolver(
bufimage.ProtoImageToFileDescriptors(
firstProtoImage,
)...,
)
resolver, err := protoencoding.NewResolver(firstProtoImage.File...)
if err != nil {
newResolverSpan.RecordError(err)
newResolverSpan.SetStatus(codes.Error, err.Error())
Expand Down
8 changes: 2 additions & 6 deletions private/buf/bufwire/image_writer.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,9 +109,7 @@ func (i *imageWriter) imageMarshal(
case buffetch.ImageEncodingJSON:
// TODO: verify that image is complete
resolver, err := protoencoding.NewResolver(
bufimage.ImageToFileDescriptors(
image,
)...,
bufimage.ImageToFileDescriptorProtos(image)...,
)
if err != nil {
return nil, err
Expand All @@ -120,9 +118,7 @@ func (i *imageWriter) imageMarshal(
case buffetch.ImageEncodingTxtpb:
// TODO: verify that image is complete
resolver, err := protoencoding.NewResolver(
bufimage.ImageToFileDescriptors(
image,
)...,
bufimage.ImageToFileDescriptorProtos(image)...,
)
if err != nil {
return nil, err
Expand Down
4 changes: 1 addition & 3 deletions private/buf/bufwire/proto_encoding_reader.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,7 @@ func (p *protoEncodingReader) GetMessage(
}()
// Currently, this support binpb and JSON format.
resolver, err := protoencoding.NewResolver(
bufimage.ImageToFileDescriptors(
image,
)...,
bufimage.ImageToFileDescriptorProtos(image)...,
)
if err != nil {
return nil, err
Expand Down
4 changes: 1 addition & 3 deletions private/buf/bufwire/proto_encoding_writer.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,7 @@ func (p *protoEncodingWriter) PutMessage(
) (retErr error) {
// Currently, this support binpb and JSON format.
resolver, err := protoencoding.NewResolver(
bufimage.ImageToFileDescriptors(
image,
)...,
bufimage.ImageToFileDescriptorProtos(image)...,
)
if err != nil {
return err
Expand Down
2 changes: 1 addition & 1 deletion private/buf/cmd/buf/command/curl/curl.go
Original file line number Diff line number Diff line change
Expand Up @@ -943,7 +943,7 @@ func run(ctx context.Context, container appflag.Container, f *flags) (err error)
if err != nil {
return err
}
res, err = protoencoding.NewResolver(bufimage.ImageToFileDescriptors(image)...)
res, err = protoencoding.NewResolver(bufimage.ImageToFileDescriptorProtos(image)...)
if err != nil {
return err
}
Expand Down
24 changes: 5 additions & 19 deletions private/bufpkg/bufimage/bufimage.go
Original file line number Diff line number Diff line change
Expand Up @@ -381,12 +381,7 @@ func ImageToProtoImage(image Image) *imagev1.Image {

// ImageToFileDescriptorSet returns a new FileDescriptorSet for the Image.
func ImageToFileDescriptorSet(image Image) *descriptorpb.FileDescriptorSet {
return protodescriptor.FileDescriptorSetForFileDescriptors(ImageToFileDescriptors(image)...)
}

// ImageToFileDescriptors returns the FileDescriptors for the Image.
func ImageToFileDescriptors(image Image) []protodescriptor.FileDescriptor {
return imageFilesToFileDescriptors(image.Files())
return protodescriptor.FileDescriptorSetForFileDescriptors(ImageToFileDescriptorProtos(image)...)
}

// ImageToFileDescriptorProtos returns the FileDescriptorProtos for the Image.
Expand Down Expand Up @@ -478,12 +473,7 @@ func ImagesToCodeGeneratorRequests(
return requests
}

// ProtoImageToFileDescriptors returns the FileDescriptors for the proto Image.
func ProtoImageToFileDescriptors(protoImage *imagev1.Image) []protodescriptor.FileDescriptor {
return protoImageFilesToFileDescriptors(protoImage.File)
}

// ImageDependency is a dependency of an image.
// ImageModuleDependency is a dependency of an image.
//
// This could conceivably be part of ImageFile or bufmoduleref.FileInfo.
// For ImageFile, this would be a field that is ignored when translated to proto,
Expand Down Expand Up @@ -533,9 +523,9 @@ type ImageModuleDependency interface {
isImageModuleDependency()
}

// ImageModuleDependency returns all ImageModuleDependencies for the Image.
// ImageModuleDependencies returns all ImageModuleDependency values for the Image.
//
// Does not return any ImageModuleDependencies for non-imports, that is the
// Does not return any ImageModuleDependency values for non-imports, that is the
// ModuleIdentities and commits represented by non-imports are not represented
// in this list.
func ImageModuleDependencies(image Image) []ImageModuleDependency {
Expand Down Expand Up @@ -583,11 +573,7 @@ type newImageForProtoOptions struct {
func reparseImageProto(protoImage *imagev1.Image, computeUnusedImports bool) error {
// TODO right now, NewResolver sets AllowUnresolvable to true all the time
// we want to make this into a check, and we verify if we need this for the individual command
resolver := protoencoding.NewLazyResolver(
ProtoImageToFileDescriptors(
protoImage,
)...,
)
resolver := protoencoding.NewLazyResolver(protoImage.File...)
if err := protoencoding.ReparseUnrecognized(resolver, protoImage.ProtoReflect()); err != nil {
return fmt.Errorf("could not reparse image: %v", err)
}
Expand Down
4 changes: 2 additions & 2 deletions private/bufpkg/bufimage/bufimageutil/bufimageutil_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@ func runDiffTest(t *testing.T, testdataDir string, typenames []string, expectedF
// So we serialize and then de-serialize, and use only the filtered results to parse extensions. That
// way, the result will omit custom options that aren't present in the filtered set (as they will be
// considered unrecognized fields).
resolver, err := protoencoding.NewResolver(bufimage.ImageToFileDescriptors(filteredImage)...)
resolver, err := protoencoding.NewResolver(bufimage.ImageToFileDescriptorProtos(filteredImage)...)
require.NoError(t, err)
data, err := proto.Marshal(bufimage.ImageToFileDescriptorSet(filteredImage))
require.NoError(t, err)
Expand Down Expand Up @@ -346,7 +346,7 @@ func runSourceCodeInfoTest(t *testing.T, typename string, expectedFile string, o

checkExpectation(t, ctx, actual, bucket, expectedFile)

resolver, err := protoencoding.NewResolver(bufimage.ImageToFileDescriptors(filteredImage)...)
resolver, err := protoencoding.NewResolver(bufimage.ImageToFileDescriptorProtos(filteredImage)...)
require.NoError(t, err)
file, err := resolver.FindFileByPath("test.proto")
require.NoError(t, err)
Expand Down
17 changes: 0 additions & 17 deletions private/bufpkg/bufimage/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import (
"github.com/bufbuild/buf/private/gen/data/datawkt"
imagev1 "github.com/bufbuild/buf/private/gen/proto/go/buf/alpha/image/v1"
"github.com/bufbuild/buf/private/pkg/normalpath"
"github.com/bufbuild/buf/private/pkg/protodescriptor"
"github.com/bufbuild/buf/private/pkg/slicesextended"
"google.golang.org/protobuf/encoding/protowire"
"google.golang.org/protobuf/proto"
Expand Down Expand Up @@ -292,22 +291,6 @@ func checkExcludePathsExistInImage(image Image, excludeFileOrDirPaths []string)
return nil
}

func protoImageFilesToFileDescriptors(protoImageFiles []*imagev1.ImageFile) []protodescriptor.FileDescriptor {
fileDescriptors := make([]protodescriptor.FileDescriptor, len(protoImageFiles))
for i, protoImageFile := range protoImageFiles {
fileDescriptors[i] = protoImageFile
}
return fileDescriptors
}

func imageFilesToFileDescriptors(imageFiles []ImageFile) []protodescriptor.FileDescriptor {
fileDescriptors := make([]protodescriptor.FileDescriptor, len(imageFiles))
for i, imageFile := range imageFiles {
fileDescriptors[i] = imageFile.FileDescriptorProto()
}
return fileDescriptors
}

func imageFilesToFileDescriptorProtos(imageFiles []ImageFile) []*descriptorpb.FileDescriptorProto {
fileDescriptorProtos := make([]*descriptorpb.FileDescriptorProto, len(imageFiles))
for i, imageFile := range imageFiles {
Expand Down
23 changes: 7 additions & 16 deletions private/pkg/protodescriptor/protodescriptor.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,20 +45,6 @@ type FileDescriptor interface {
GetEdition() descriptorpb.Edition
}

// FileDescriptorsForFileDescriptorProtos is a convenience function since Go does not have generics.
func FileDescriptorsForFileDescriptorProtos(fileDescriptorProtos ...*descriptorpb.FileDescriptorProto) []FileDescriptor {
fileDescriptors := make([]FileDescriptor, len(fileDescriptorProtos))
for i, fileDescriptorProto := range fileDescriptorProtos {
fileDescriptors[i] = fileDescriptorProto
}
return fileDescriptors
}

// FileDescriptorsForFileDescriptorSet is a convenience function since Go does not have generics.
func FileDescriptorsForFileDescriptorSet(fileDescriptorSet *descriptorpb.FileDescriptorSet) []FileDescriptor {
return FileDescriptorsForFileDescriptorProtos(fileDescriptorSet.File...)
}

// FileDescriptorProtoForFileDescriptor creates a new *descriptorpb.FileDescriptorProto for the fileDescriptor.
//
// If the FileDescriptor is already a *descriptorpb.FileDescriptorProto, this returns the input value.
Expand Down Expand Up @@ -106,7 +92,12 @@ func FileDescriptorProtoForFileDescriptor(fileDescriptor FileDescriptor) *descri
// object that is a FileDescriptor, and then passed to this function, the return value will not be equal
// if name, package, or syntax are set but empty. Instead, the return value will have these values unset.
// For our/most purposes, this is fine.
func FileDescriptorProtosForFileDescriptors(fileDescriptors ...FileDescriptor) []*descriptorpb.FileDescriptorProto {
func FileDescriptorProtosForFileDescriptors[F FileDescriptor](fileDescriptors ...F) []*descriptorpb.FileDescriptorProto {
fileDescriptorsAny := any(fileDescriptors) // must assign to interface var to do type assertion below
if fileDescriptorProtos, ok := fileDescriptorsAny.([]*descriptorpb.FileDescriptorProto); ok {
return fileDescriptorProtos
}

fileDescriptorProtos := make([]*descriptorpb.FileDescriptorProto, len(fileDescriptors))
for i, fileDescriptor := range fileDescriptors {
fileDescriptorProtos[i] = FileDescriptorProtoForFileDescriptor(fileDescriptor)
Expand All @@ -120,7 +111,7 @@ func FileDescriptorProtosForFileDescriptors(fileDescriptors ...FileDescriptor) [
// object that is a FileDescriptor, and then passed to this function, the return value will not be equal
// if name, package, or syntax are set but empty. Instead, the return value will have these values unset.
// For our/most purposes, this is fine.
func FileDescriptorSetForFileDescriptors(fileDescriptors ...FileDescriptor) *descriptorpb.FileDescriptorSet {
func FileDescriptorSetForFileDescriptors[F FileDescriptor](fileDescriptors ...F) *descriptorpb.FileDescriptorSet {
return &descriptorpb.FileDescriptorSet{
File: FileDescriptorProtosForFileDescriptors(fileDescriptors...),
}
Expand Down
4 changes: 2 additions & 2 deletions private/pkg/protoencoding/protoencoding.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ type Resolver interface {
// If the input slice is empty, this returns nil
// The given FileDescriptors must be self-contained, that is they must contain all imports.
// This can NOT be guaranteed for FileDescriptorSets given over the wire, and can only be guaranteed from builds.
func NewResolver(fileDescriptors ...protodescriptor.FileDescriptor) (Resolver, error) {
func NewResolver[F protodescriptor.FileDescriptor](fileDescriptors ...F) (Resolver, error) {
return newResolver(fileDescriptors...)
}

Expand All @@ -44,7 +44,7 @@ func NewResolver(fileDescriptors ...protodescriptor.FileDescriptor) (Resolver, e
//
// If there is an error when constructing the resolver, it will be returned by all
// method calls of the returned resolver.
func NewLazyResolver(fileDescriptors ...protodescriptor.FileDescriptor) Resolver {
func NewLazyResolver[F protodescriptor.FileDescriptor](fileDescriptors ...F) Resolver {
return &lazyResolver{fn: func() (Resolver, error) {
return newResolver(fileDescriptors...)
}}
Expand Down
2 changes: 1 addition & 1 deletion private/pkg/protoencoding/resolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import (
"google.golang.org/protobuf/types/dynamicpb"
)

func newResolver(fileDescriptors ...protodescriptor.FileDescriptor) (Resolver, error) {
func newResolver[F protodescriptor.FileDescriptor](fileDescriptors ...F) (Resolver, error) {
if len(fileDescriptors) == 0 {
return nil, nil
}
Expand Down
7 changes: 3 additions & 4 deletions private/pkg/prototesting/prototesting.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import (

"github.com/bufbuild/buf/private/pkg/command"
"github.com/bufbuild/buf/private/pkg/diff"
"github.com/bufbuild/buf/private/pkg/protodescriptor"
"github.com/bufbuild/buf/private/pkg/protoencoding"
"github.com/google/go-cmp/cmp"
"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -84,7 +83,7 @@ func GetProtocFileDescriptorSet(
if err := protoencoding.NewWireUnmarshaler(nil).Unmarshal(data, firstFileDescriptorSet); err != nil {
return nil, err
}
resolver, err := protoencoding.NewResolver(protodescriptor.FileDescriptorsForFileDescriptorSet(firstFileDescriptorSet)...)
resolver, err := protoencoding.NewResolver(firstFileDescriptorSet.File...)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -191,15 +190,15 @@ func DiffFileDescriptorSetsJSON(
oneName string,
twoName string,
) (string, error) {
oneResolver, err := protoencoding.NewResolver(protodescriptor.FileDescriptorsForFileDescriptorSet(one)...)
oneResolver, err := protoencoding.NewResolver(one.File...)
if err != nil {
return "", err
}
oneData, err := protoencoding.NewJSONMarshaler(oneResolver, protoencoding.JSONMarshalerWithIndent()).Marshal(one)
if err != nil {
return "", err
}
twoResolver, err := protoencoding.NewResolver(protodescriptor.FileDescriptorsForFileDescriptorSet(two)...)
twoResolver, err := protoencoding.NewResolver(two.File...)
if err != nil {
return "", err
}
Expand Down

0 comments on commit 82103ef

Please sign in to comment.