protoprint: make sure output is stable, even when there is no source …

…code info (#550)
jhump · Feb 23, 2023 · 1d63728 · 1d63728
1 parent 220303e
commit 1d63728
Show file tree

Hide file tree

Showing 31 changed files with 222 additions and 120 deletions.
diff --git a/desc/protoprint/print.go b/desc/protoprint/print.go
@@ -1985,10 +1985,13 @@ func uninterpretedToOptions(uninterp []*descriptorpb.UninterpretedOption) []opti
 }
 
 func optionsAsElementAddrs(optionsTag int32, order int, opts map[int32][]option) []elementAddr {
-	var optAddrs []elementAddr
+	optAddrs := make([]elementAddr, 0, len(opts))
 	for tag := range opts {
 		optAddrs = append(optAddrs, elementAddr{elementType: optionsTag, elementIndex: int(tag), order: order})
 	}
+	// We want stable output. So, if the printer can't sort these a better way,
+	// they'll at least be in a deterministic order (by name).
+	sort.Sort(optionsByName{addrs: optAddrs, opts: opts})
 	return optAddrs
 }
 
@@ -2392,6 +2395,25 @@ func (cso customSortOrder) Less(i, j int) bool {
 	return cso.less(ei, ej)
 }
 
+type optionsByName struct {
+	addrs []elementAddr
+	opts  map[int32][]option
+}
+
+func (o optionsByName) Len() int {
+	return len(o.addrs)
+}
+
+func (o optionsByName) Less(i, j int) bool {
+	oi := o.opts[int32(o.addrs[i].elementIndex)]
+	oj := o.opts[int32(o.addrs[j].elementIndex)]
+	return optionLess(oi, oj)
+}
+
+func (o optionsByName) Swap(i, j int) {
+	o.addrs[i], o.addrs[j] = o.addrs[j], o.addrs[i]
+}
+
 func optionLess(i, j []option) bool {
 	ni := i[0].name
 	nj := j[0].name

diff --git a/desc/protoprint/print_test.go b/desc/protoprint/print_test.go
@@ -10,6 +10,7 @@ import (
 	"testing"
 
 	"google.golang.org/protobuf/proto"
+	"google.golang.org/protobuf/reflect/protoregistry"
 	"google.golang.org/protobuf/types/descriptorpb"
 
 	"github.com/jhump/protoreflect/desc"
@@ -87,8 +88,14 @@ func TestPrinter(t *testing.T) {
 		fds[i] = fd
 	}
 	// extra descriptor that has no source info
-	fd, err := desc.LoadFileDescriptor("desc_test2.proto")
+	// NB: We can't use desc.LoadFileDescriptor here because that, under the hood, will get
+	//     source code info from the desc/sourceinfo package! So explicitly load the version
+	//     from the underlying registry, which will NOT have source code info.
+	underlyingFd, err := protoregistry.GlobalFiles.FindFileByPath("desc_test2.proto")
 	testutil.Ok(t, err)
+	fd, err := desc.WrapFile(underlyingFd)
+	testutil.Ok(t, err)
+	testutil.Require(t, fd.AsFileDescriptorProto().SourceCodeInfo == nil)
 	fds[len(files)] = fd
 
 	for _, fd := range fds {

diff --git a/desc/protoprint/testfiles/desc_test2-compact.proto b/desc/protoprint/testfiles/desc_test2-compact.proto
@@ -1,9 +1,15 @@
 syntax = "proto2";
-option go_package = "github.com/jhump/protoreflect/internal/testprotos";
 package testprotos;
 import "desc_test1.proto";
 import "pkg/desc_test_pkg.proto";
 import "nopkg/desc_test_nopkg.proto";
+option cc_enable_arenas = true;
+option csharp_namespace = "jhump.protoreflect.testprotos";
+option go_package = "github.com/jhump/protoreflect/internal/testprotos";
+option java_generate_equals_and_hash = true;
+option java_multiple_files = true;
+option java_package = "com.github.jhump.protoreflect.internal.testprotos";
+option ruby_package = "protoreflect-testprotos";
 message Frobnitz {
   optional TestMessage a = 1;
   optional AnotherTestMessage b = 2;

diff --git a/desc/protoprint/testfiles/desc_test2-custom-sort.proto b/desc/protoprint/testfiles/desc_test2-custom-sort.proto
@@ -44,8 +44,20 @@ message Frobnitz {
   optional TestMessage a = 1;
 }
 
+option ruby_package = "protoreflect-testprotos";
+
+option java_package = "com.github.jhump.protoreflect.internal.testprotos";
+
+option java_multiple_files = true;
+
+option java_generate_equals_and_hash = true;
+
 option go_package = "github.com/jhump/protoreflect/internal/testprotos";
 
+option csharp_namespace = "jhump.protoreflect.testprotos";
+
+option cc_enable_arenas = true;
+
 import "pkg/desc_test_pkg.proto";
 
 import "nopkg/desc_test_nopkg.proto";

diff --git a/desc/protoprint/testfiles/desc_test2-default.proto b/desc/protoprint/testfiles/desc_test2-default.proto
@@ -1,7 +1,5 @@
 syntax = "proto2";
 
-option go_package = "github.com/jhump/protoreflect/internal/testprotos";
-
 package testprotos;
 
 import "desc_test1.proto";
@@ -10,6 +8,20 @@ import "pkg/desc_test_pkg.proto";
 
 import "nopkg/desc_test_nopkg.proto";
 
+option cc_enable_arenas = true;
+
+option csharp_namespace = "jhump.protoreflect.testprotos";
+
+option go_package = "github.com/jhump/protoreflect/internal/testprotos";
+
+option java_generate_equals_and_hash = true;
+
+option java_multiple_files = true;
+
+option java_package = "com.github.jhump.protoreflect.internal.testprotos";
+
+option ruby_package = "protoreflect-testprotos";
+
 message Frobnitz {
   optional TestMessage a = 1;
 

diff --git a/desc/protoprint/testfiles/desc_test2-multiline-style-comments.proto b/desc/protoprint/testfiles/desc_test2-multiline-style-comments.proto
@@ -1,7 +1,5 @@
 syntax = "proto2";
 
-option go_package = "github.com/jhump/protoreflect/internal/testprotos";
-
 package testprotos;
 
 import "desc_test1.proto";
@@ -10,6 +8,20 @@ import "pkg/desc_test_pkg.proto";
 
 import "nopkg/desc_test_nopkg.proto";
 
+option cc_enable_arenas = true;
+
+option csharp_namespace = "jhump.protoreflect.testprotos";
+
+option go_package = "github.com/jhump/protoreflect/internal/testprotos";
+
+option java_generate_equals_and_hash = true;
+
+option java_multiple_files = true;
+
+option java_package = "com.github.jhump.protoreflect.internal.testprotos";
+
+option ruby_package = "protoreflect-testprotos";
+
 message Frobnitz {
 	optional TestMessage a = 1;
 

diff --git a/desc/protoprint/testfiles/desc_test2-no-trailing-comments.proto b/desc/protoprint/testfiles/desc_test2-no-trailing-comments.proto
@@ -1,7 +1,5 @@
 syntax = "proto2";
 
-option go_package = "github.com/jhump/protoreflect/internal/testprotos";
-
 package testprotos;
 
 import "desc_test1.proto";
@@ -10,6 +8,20 @@ import "pkg/desc_test_pkg.proto";
 
 import "nopkg/desc_test_nopkg.proto";
 
+option cc_enable_arenas = true;
+
+option csharp_namespace = "jhump.protoreflect.testprotos";
+
+option go_package = "github.com/jhump/protoreflect/internal/testprotos";
+
+option java_generate_equals_and_hash = true;
+
+option java_multiple_files = true;
+
+option java_package = "com.github.jhump.protoreflect.internal.testprotos";
+
+option ruby_package = "protoreflect-testprotos";
+
 message Frobnitz {
   optional TestMessage a = 1;
 

diff --git a/desc/protoprint/testfiles/desc_test2-only-doc-comments.proto b/desc/protoprint/testfiles/desc_test2-only-doc-comments.proto
@@ -1,7 +1,5 @@
 syntax = "proto2";
 
-option go_package = "github.com/jhump/protoreflect/internal/testprotos";
-
 package testprotos;
 
 import "desc_test1.proto";
@@ -10,6 +8,20 @@ import "pkg/desc_test_pkg.proto";
 
 import "nopkg/desc_test_nopkg.proto";
 
+option cc_enable_arenas = true;
+
+option csharp_namespace = "jhump.protoreflect.testprotos";
+
+option go_package = "github.com/jhump/protoreflect/internal/testprotos";
+
+option java_generate_equals_and_hash = true;
+
+option java_multiple_files = true;
+
+option java_package = "com.github.jhump.protoreflect.internal.testprotos";
+
+option ruby_package = "protoreflect-testprotos";
+
 message Frobnitz {
   optional TestMessage a = 1;
 

diff --git a/desc/protoprint/testfiles/desc_test2-sorted-AND-multiline-style-comments.proto b/desc/protoprint/testfiles/desc_test2-sorted-AND-multiline-style-comments.proto
@@ -8,8 +8,20 @@ import "nopkg/desc_test_nopkg.proto";
 
 import "pkg/desc_test_pkg.proto";
 
+option cc_enable_arenas = true;
+
+option csharp_namespace = "jhump.protoreflect.testprotos";
+
 option go_package = "github.com/jhump/protoreflect/internal/testprotos";
 
+option java_generate_equals_and_hash = true;
+
+option java_multiple_files = true;
+
+option java_package = "com.github.jhump.protoreflect.internal.testprotos";
+
+option ruby_package = "protoreflect-testprotos";
+
 message Frobnitz {
   optional TestMessage a = 1;
 

diff --git a/desc/protoprint/testfiles/desc_test2-sorted.proto b/desc/protoprint/testfiles/desc_test2-sorted.proto
@@ -8,8 +8,20 @@ import "nopkg/desc_test_nopkg.proto";
 
 import "pkg/desc_test_pkg.proto";
 
+option cc_enable_arenas = true;
+
+option csharp_namespace = "jhump.protoreflect.testprotos";
+
 option go_package = "github.com/jhump/protoreflect/internal/testprotos";
 
+option java_generate_equals_and_hash = true;
+
+option java_multiple_files = true;
+
+option java_package = "com.github.jhump.protoreflect.internal.testprotos";
+
+option ruby_package = "protoreflect-testprotos";
+
 message Frobnitz {
    optional TestMessage a = 1;
 

diff --git a/desc/protoprint/testfiles/desc_test2-trailing-on-next-line.proto b/desc/protoprint/testfiles/desc_test2-trailing-on-next-line.proto
@@ -1,7 +1,5 @@
 syntax = "proto2";
 
-option go_package = "github.com/jhump/protoreflect/internal/testprotos";
-
 package testprotos;
 
 import "desc_test1.proto";
@@ -10,6 +8,20 @@ import "pkg/desc_test_pkg.proto";
 
 import "nopkg/desc_test_nopkg.proto";
 
+option cc_enable_arenas = true;
+
+option csharp_namespace = "jhump.protoreflect.testprotos";
+
+option go_package = "github.com/jhump/protoreflect/internal/testprotos";
+
+option java_generate_equals_and_hash = true;
+
+option java_multiple_files = true;
+
+option java_package = "com.github.jhump.protoreflect.internal.testprotos";
+
+option ruby_package = "protoreflect-testprotos";
+
 message Frobnitz {
   optional TestMessage a = 1;
 

diff --git a/grpcreflect/internal/grpc_reflection_v1/reflection.pb.go b/grpcreflect/internal/grpc_reflection_v1/reflection.pb.go
diff --git a/internal/testprotos/desc_test1.pb.go b/internal/testprotos/desc_test1.pb.go
diff --git a/internal/testprotos/desc_test2.pb.go b/internal/testprotos/desc_test2.pb.go