Skip to content

Commit

Permalink
Remove canonical bytes feature (#262)
Browse files Browse the repository at this point in the history 
This was never actually used in the buf CLI. And it's rather complicated
and kind of a lot to maintain for an effectively unused feature. It also
had several bugs, so instead of merging fixes for those bugs, we're just
removing the whole feature.
  • Loading branch information
@jhump
jhump authored Mar 27, 2024
1 parent 438e85a commit e1afd44
Show file tree
Hide file tree
Showing 12 changed files with 312 additions and 1,055 deletions.
31 changes: 7 additions & 24 deletions internal/benchmarks/benchmark_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ import (
"github.com/bufbuild/protocompile"
"github.com/bufbuild/protocompile/ast"
"github.com/bufbuild/protocompile/internal/protoc"
"github.com/bufbuild/protocompile/linker"
"github.com/bufbuild/protocompile/parser"
"github.com/bufbuild/protocompile/parser/fastscan"
"github.com/bufbuild/protocompile/protoutil"
Expand Down Expand Up @@ -235,7 +234,7 @@ func downloadAndExpand(url, targetDir string) (e error) {
}

func BenchmarkGoogleapisProtocompile(b *testing.B) {
benchmarkGoogleapisProtocompile(b, false, func() *protocompile.Compiler {
benchmarkGoogleapisProtocompile(b, func() *protocompile.Compiler {
return &protocompile.Compiler{
Resolver: protocompile.WithStandardImports(&protocompile.SourceResolver{
ImportPaths: []string{googleapisDir},
Expand All @@ -246,20 +245,8 @@ func BenchmarkGoogleapisProtocompile(b *testing.B) {
})
}

func BenchmarkGoogleapisProtocompileCanonical(b *testing.B) {
benchmarkGoogleapisProtocompile(b, true, func() *protocompile.Compiler {
return &protocompile.Compiler{
Resolver: protocompile.WithStandardImports(&protocompile.SourceResolver{
ImportPaths: []string{googleapisDir},
}),
SourceInfoMode: protocompile.SourceInfoStandard,
// leave MaxParallelism unset to let it use all cores available
}
})
}

func BenchmarkGoogleapisProtocompileNoSourceInfo(b *testing.B) {
benchmarkGoogleapisProtocompile(b, false, func() *protocompile.Compiler {
benchmarkGoogleapisProtocompile(b, func() *protocompile.Compiler {
return &protocompile.Compiler{
Resolver: protocompile.WithStandardImports(&protocompile.SourceResolver{
ImportPaths: []string{googleapisDir},
Expand All @@ -270,23 +257,19 @@ func BenchmarkGoogleapisProtocompileNoSourceInfo(b *testing.B) {
})
}

func benchmarkGoogleapisProtocompile(b *testing.B, canonicalBytes bool, factory func() *protocompile.Compiler) {
func benchmarkGoogleapisProtocompile(b *testing.B, factory func() *protocompile.Compiler) {
for i := 0; i < b.N; i++ {
benchmarkProtocompile(b, factory(), googleapisSources, canonicalBytes)
benchmarkProtocompile(b, factory(), googleapisSources)
}
}

func benchmarkProtocompile(b *testing.B, c *protocompile.Compiler, sources []string, canonicalBytes bool) {
func benchmarkProtocompile(b *testing.B, c *protocompile.Compiler, sources []string) {
fds, err := c.Compile(context.Background(), sources...)
require.NoError(b, err)
var fdSet descriptorpb.FileDescriptorSet
fdSet.File = make([]*descriptorpb.FileDescriptorProto, len(fds))
for i, fd := range fds {
if canonicalBytes {
fdSet.File[i] = fd.(linker.Result).CanonicalProto()
} else {
fdSet.File[i] = protoutil.ProtoFromFileDescriptor(fd)
}
fdSet.File[i] = protoutil.ProtoFromFileDescriptor(fd)
}
// protoc is writing output to file descriptor set, so we should, too
writeToNull(b, &fdSet)
Expand Down Expand Up @@ -484,7 +467,7 @@ func BenchmarkGoogleapisProtocompileSingleThreaded(b *testing.B) {
// need to run a single-threaded compile
MaxParallelism: 1,
}
benchmarkProtocompile(b, c, googleapisSources, false)
benchmarkProtocompile(b, c, googleapisSources)
}
})
}
Expand Down
27 changes: 5 additions & 22 deletions internal/prototest/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
package prototest

import (
"fmt"
"os"
"testing"

Expand Down Expand Up @@ -73,27 +72,11 @@ func findFileInSet(fps *descriptorpb.FileDescriptorSet, name string) *descriptor
return nil
}

func AssertMessagesEqual(t *testing.T, exp, act proto.Message, msgAndArgs ...interface{}) {
func AssertMessagesEqual(t *testing.T, exp, act proto.Message, description string) bool {
t.Helper()
AssertMessagesEqualWithOptions(t, exp, act, nil, msgAndArgs...)
}

func AssertMessagesEqualWithOptions(t *testing.T, exp, act proto.Message, opts []cmp.Option, msgAndArgs ...interface{}) {
t.Helper()
cmpOpts := []cmp.Option{protocmp.Transform()}
cmpOpts = append(cmpOpts, opts...)
if diff := cmp.Diff(exp, act, cmpOpts...); diff != "" {
var prefix string
if len(msgAndArgs) == 1 {
if msg, ok := msgAndArgs[0].(string); ok {
prefix = msg + ": "
} else {
prefix = fmt.Sprintf("%+v: ", msgAndArgs[0])
}
} else if len(msgAndArgs) > 1 {
prefix = fmt.Sprintf(msgAndArgs[0].(string)+": ", msgAndArgs[1:]...)
}

t.Errorf("%smessage mismatch (-want +got):\n%v", prefix, diff)
if diff := cmp.Diff(exp, act, protocmp.Transform()); diff != "" {
t.Errorf("%s: message mismatch (-want, +got):\n%s", description, diff)
return false
}
return true
}
5 changes: 5 additions & 0 deletions internal/tags.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -243,4 +243,9 @@ const (
// UninterpretedNameNameTag is the tag number of the name element in an
// uninterpreted option name proto.
UninterpretedNameNameTag = 1

// AnyTypeURLTag is the tag number of the type_url field of the Any proto.
AnyTypeURLTag = 1
// AnyValueTag is the tag number of the value field of the Any proto.
AnyValueTag = 2
)
2 changes: 1 addition & 1 deletion internal/testdata/options/options.proto
View file
Original file line number Diff line number Diff line change
Expand Up @@ -227,4 +227,4 @@ option (any) = {
pr_i32: [0,1,2,3]
str: "foo"
}
};
};
6 changes: 6 additions & 0 deletions internal/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,12 @@ func CanPack(k protoreflect.Kind) bool {
}
}

func ClonePath(path protoreflect.SourcePath) protoreflect.SourcePath {
clone := make(protoreflect.SourcePath, len(path))
copy(clone, path)
return clone
}

func reverse(p protoreflect.SourcePath) protoreflect.SourcePath {
for i, j := 0, len(p)-1; i < j; i, j = i+1, j-1 {
p[i], p[j] = p[j], p[i]
Expand Down
140 changes: 0 additions & 140 deletions linker/descriptors.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,11 +151,6 @@ type result struct {
// interpreting options.
usedImports map[string]struct{}

// A map of descriptor options messages to their pre-serialized bytes (using
// a canonical serialization format based on how protoc renders options to
// bytes).
optionBytes map[proto.Message][]byte

// A map of AST nodes that represent identifiers in ast.FieldReferenceNodes
// to their fully-qualified name. The identifiers are for field names in
// message literals (in option values) that are extension fields. These names
Expand Down Expand Up @@ -316,141 +311,6 @@ func asSourceLocations(srcInfoProtos []*descriptorpb.SourceCodeInfo_Location) []
return locs
}

// AddOptionBytes associates the given opts (an options message encoded in the
// binary format) with the given options protobuf message. The protobuf message
// should exist in the hierarchy of this result's FileDescriptorProto. This
// allows the FileDescriptorProto to be marshaled to bytes in a way that
// preserves the way options are defined in source (just as is done by protoc,
// but not possible when only using the generated Go types and standard
// marshaling APIs in the protobuf runtime).
func (r *result) AddOptionBytes(pm proto.Message, opts []byte) {
if r.optionBytes == nil {
r.optionBytes = map[proto.Message][]byte{}
}
r.optionBytes[pm] = append(r.optionBytes[pm], opts...)
}

func (r *result) CanonicalProto() *descriptorpb.FileDescriptorProto {
origFd := r.FileDescriptorProto()
// make a copy that we can mutate
fd := proto.Clone(origFd).(*descriptorpb.FileDescriptorProto) //nolint:errcheck

r.storeOptionBytesInFile(fd, origFd)

return fd
}

func (r *result) storeOptionBytes(opts, origOpts proto.Message) {
optionBytes := r.optionBytes[origOpts]
if len(optionBytes) == 0 {
// If we don't know about this options message, leave it alone.
return
}
proto.Reset(opts)
opts.ProtoReflect().SetUnknown(optionBytes)
}

func (r *result) storeOptionBytesInFile(fd, origFd *descriptorpb.FileDescriptorProto) {
if fd.Options != nil {
r.storeOptionBytes(fd.Options, origFd.Options)
}

for i, md := range fd.MessageType {
origMd := origFd.MessageType[i]
r.storeOptionBytesInMessage(md, origMd)
}

for i, ed := range fd.EnumType {
origEd := origFd.EnumType[i]
r.storeOptionBytesInEnum(ed, origEd)
}

for i, exd := range fd.Extension {
origExd := origFd.Extension[i]
r.storeOptionBytesInField(exd, origExd)
}

for i, sd := range fd.Service {
origSd := origFd.Service[i]
if sd.Options != nil {
r.storeOptionBytes(sd.Options, origSd.Options)
}

for j, mtd := range sd.Method {
origMtd := origSd.Method[j]
if mtd.Options != nil {
r.storeOptionBytes(mtd.Options, origMtd.Options)
}
}
}
}

func (r *result) storeOptionBytesInMessage(md, origMd *descriptorpb.DescriptorProto) {
if md.GetOptions().GetMapEntry() {
// Map entry messages are synthesized. They won't have any option bytes
// since they don't actually appear in the source and thus have any option
// declarations in the source.
return
}

if md.Options != nil {
r.storeOptionBytes(md.Options, origMd.Options)
}

for i, fld := range md.Field {
origFld := origMd.Field[i]
r.storeOptionBytesInField(fld, origFld)
}

for i, ood := range md.OneofDecl {
origOod := origMd.OneofDecl[i]
if ood.Options != nil {
r.storeOptionBytes(ood.Options, origOod.Options)
}
}

for i, exr := range md.ExtensionRange {
origExr := origMd.ExtensionRange[i]
if exr.Options != nil {
r.storeOptionBytes(exr.Options, origExr.Options)
}
}

for i, nmd := range md.NestedType {
origNmd := origMd.NestedType[i]
r.storeOptionBytesInMessage(nmd, origNmd)
}

for i, ed := range md.EnumType {
origEd := origMd.EnumType[i]
r.storeOptionBytesInEnum(ed, origEd)
}

for i, exd := range md.Extension {
origExd := origMd.Extension[i]
r.storeOptionBytesInField(exd, origExd)
}
}

func (r *result) storeOptionBytesInEnum(ed, origEd *descriptorpb.EnumDescriptorProto) {
if ed.Options != nil {
r.storeOptionBytes(ed.Options, origEd.Options)
}

for i, evd := range ed.Value {
origEvd := origEd.Value[i]
if evd.Options != nil {
r.storeOptionBytes(evd.Options, origEvd.Options)
}
}
}

func (r *result) storeOptionBytesInField(fld, origFld *descriptorpb.FieldDescriptorProto) {
if fld.Options != nil {
r.storeOptionBytes(fld.Options, origFld.Options)
}
}

type fileImports struct {
protoreflect.FileImports
files []protoreflect.FileImport
Expand Down
25 changes: 0 additions & 25 deletions linker/linker.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import (
"fmt"

"google.golang.org/protobuf/reflect/protoreflect"
"google.golang.org/protobuf/types/descriptorpb"

"github.com/bufbuild/protocompile/ast"
"github.com/bufbuild/protocompile/parser"
Expand Down Expand Up @@ -128,30 +127,6 @@ type Result interface {
// interpreting options (which is done after linking).
PopulateSourceCodeInfo()

// CanonicalProto returns the file descriptor proto in a form that
// will be serialized in a canonical way. The "canonical" way matches
// the way that "protoc" emits option values, which is a way that
// mostly matches the way options are defined in source, including
// ordering and de-structuring. Unlike the FileDescriptorProto() method,
// this method is more expensive and results in a new descriptor proto
// being constructed with each call.
//
// The returned value will have all options (fields of the various
// descriptorpb.*Options message types) represented via unrecognized
// fields. So the returned value will serialize as desired, but it
// is otherwise not useful since all option values are treated as
// unknown.
//
// Note that CanonicalProto is a no-op if the options in this file
// were not interpreted by this module (e.g. the underlying descriptor
// proto was provided, with options already interpreted, instead of
// parsed from source). If the underlying descriptor proto was provided,
// but with a mix of interpreted and uninterpreted options, this method
// will effectively clear the already-interpreted fields and only the
// options actually interpreted by the compile operation will be
// retained.
CanonicalProto() *descriptorpb.FileDescriptorProto

// RemoveAST drops the AST information from this result.
RemoveAST()
}
Expand Down
2 changes: 1 addition & 1 deletion linker/linker_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2213,7 +2213,7 @@ func TestLinkerValidation(t *testing.T) {
}
`,
},
expectedErr: `test.proto:3:18: feature "enum_type" is allowed on [enum,file], not on field`,
expectedErr: `test.proto:3:27: feature "enum_type" is allowed on [enum,file], not on field`,
},
"failure_editions_feature_on_wrong_target_type_msg_literal": {
input: map[string]string{
Expand Down
Loading

0 comments on commit e1afd44

Please sign in to comment.