Update linker tests to use protodesc to create descriptors from compi…

…lation results (bufbuild#302)

This verifies that the results of compilation are processable by
`protodesc.NewFile`. There's an opt-out for cases where we know the
protobuf-go runtime can't correctly handle it.

(cherry picked from commit 2e42f6f)

options/message_sets.go → internal/messageset/messageset.go

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package options
+package messageset
 
 import (
 	"math"
@@ -28,7 +28,9 @@ var (
 	messageSetSupportInit sync.Once
 )
 
-func canSerializeMessageSets() bool {
+// CanSupportMessageSets returns true if the protobuf-go runtime supports
+// serializing messages with the message set wire format.
+func CanSupportMessageSets() bool {
 	messageSetSupportInit.Do(func() {
 		// We check using the protodesc package, instead of just relying
 		// on protolegacy build tag, in case someone links in a fork of

options/message_sets_protolegacy_test.go → internal/messageset/messageset_protolegacy_test.go

@@ -15,7 +15,7 @@
 //go:build protolegacy
 // +build protolegacy
 
-package options
+package messageset
 
 import (
 	"testing"
@@ -25,5 +25,5 @@ import (
 
 func TestCanSerializeMessageSets(t *testing.T) {
 	t.Parallel()
-	assert.True(t, canSerializeMessageSets())
+	assert.True(t, CanSupportMessageSets())
 }

options/message_sets_test.go → internal/messageset/messageset_test.go

@@ -15,7 +15,7 @@
 //go:build !protolegacy
 // +build !protolegacy
 
-package options
+package messageset
 
 import (
 	"testing"
@@ -25,5 +25,5 @@ import (
 
 func TestCanSerializeMessageSets(t *testing.T) {
 	t.Parallel()
-	assert.False(t, canSerializeMessageSets())
+	assert.False(t, CanSupportMessageSets())
 }

linker/linker_test.go

@@ -32,14 +32,18 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"google.golang.org/protobuf/proto"
+	"google.golang.org/protobuf/reflect/protodesc"
+	"google.golang.org/protobuf/reflect/protoreflect"
 	"google.golang.org/protobuf/reflect/protoregistry"
 	"google.golang.org/protobuf/types/descriptorpb"
 
 	"github.com/kralicky/protocompile"
 	"github.com/kralicky/protocompile/editions"
+	"github.com/kralicky/protocompile/internal/messageset"
 	"github.com/kralicky/protocompile/internal/protoc"
 	"github.com/kralicky/protocompile/linker"
 	"github.com/kralicky/protocompile/protointernal/prototest"
+	"github.com/kralicky/protocompile/protoutil"
 	"github.com/kralicky/protocompile/reporter"
 )
 
@@ -127,6 +131,7 @@ func TestLinkerValidation(t *testing.T) {
 		// Expected error message - leave empty if input is expected to succeed
 		expectedErr            string
 		expectedDiffWithProtoc bool
+		expectProtodescFail    bool
 	}{
 		"success_multi_namespace": {
 			input: map[string]string{
@@ -461,6 +466,7 @@ func TestLinkerValidation(t *testing.T) {
 			input: map[string]string{
 				"foo.proto": "message Foo { option message_set_wire_format = true; extensions 1 to 100; } extend Foo { optional Foo bar = 1; }",
 			},
+			expectProtodescFail: !messageset.CanSupportMessageSets(),
 		},
 		"failure_tag_out_of_range": {
 			input: map[string]string{
@@ -472,6 +478,7 @@ func TestLinkerValidation(t *testing.T) {
 			input: map[string]string{
 				"foo.proto": "message Foo { option message_set_wire_format = true; extensions 1 to max; } extend Foo { optional Foo bar = 536870912; }",
 			},
+			expectProtodescFail: !messageset.CanSupportMessageSets(),
 		},
 		"failure_message_set_wire_format_repeated": {
 			input: map[string]string{
@@ -1444,6 +1451,10 @@ func TestLinkerValidation(t *testing.T) {
 					  string FOO_BAR = 2;
 					}`,
 			},
+			// protodesc.NewFile is applying overly strict checks on name
+			// collisions in proto3 files.
+			// https://github.com/golang/protobuf/issues/1616
+			expectProtodescFail: true,
 		},
 		"failure_json_name_conflict_leading_underscores": {
 			input: map[string]string{
@@ -3220,7 +3231,7 @@ func TestLinkerValidation(t *testing.T) {
 			for filename, data := range tc.input {
 				tc.input[filename] = removePrefixIndent(data)
 			}
-			_, errs := compile(t, tc.input)
+			files, errs := compile(t, tc.input)
 
 			actualErrs := make([]string, len(errs))
 			for i := range errs {
@@ -3286,25 +3297,36 @@ func TestLinkerValidation(t *testing.T) {
 				}
 			}
 
-			// // parse with protoc
-			// passProtoc := testByProtoc(t, tc.input, tc.inputOrder)
-			// if tc.expectedErr == "" {
-			// 	if tc.expectedDiffWithProtoc {
-			// 		// We can explicitly check different result is produced by protoc. When the bug is fixed,
-			// 		// we can change the tc.expectedDiffWithProtoc field to false and delete the comment.
-			// 		require.False(t, passProtoc)
-			// 	} else {
-			// 		// if the test case passes protocompile, it should also pass protoc.
-			// 		require.True(t, passProtoc, "protoc should allow the case")
-			// 	}
-			// } else {
-			// 	if tc.expectedDiffWithProtoc {
-			// 		require.True(t, passProtoc, "expected protoc to allow the case, but it disallows it")
-			// 	} else {
-			// 		// if the test case fails protocompile, it should also fail protoc.
-			// 		require.False(t, passProtoc, "protoc should disallow the case")
-			// 	}
-			// }
+			// Make sure protobuf-go can handle resulting files
+			if len(errs) == 0 && len(files) > 0 {
+				err := convertToProtoreflectDescriptors(files)
+				if tc.expectProtodescFail {
+					// This is a known case where it cannot handle the file.
+					require.Error(t, err)
+				} else {
+					require.NoError(t, err)
+				}
+			}
+
+			// parse with protoc
+			passProtoc := testByProtoc(t, tc.input, tc.inputOrder)
+			if tc.expectedErr == "" {
+				if tc.expectedDiffWithProtoc {
+					// We can explicitly check different result is produced by protoc. When the bug is fixed,
+					// we can change the tc.expectedDiffWithProtoc field to false and delete the comment.
+					require.False(t, passProtoc, "expected protoc to disallow the case, but it allows it")
+				} else {
+					// if the test case passes protocompile, it should also pass protoc.
+					require.True(t, passProtoc, "protoc should allow the case")
+				}
+			} else {
+				if tc.expectedDiffWithProtoc {
+					require.True(t, passProtoc, "expected protoc to allow the case, but it disallows it")
+				} else {
+					// if the test case fails protocompile, it should also fail protoc.
+					require.False(t, passProtoc, "protoc should disallow the case")
+				}
+			}
 		})
 	}
 }
@@ -3871,3 +3893,28 @@ func testByProtoc(t *testing.T, files map[string]string, fileNames []string) boo
 	require.NoError(t, err)
 	return true
 }
+
+func convertToProtoreflectDescriptors(files linker.Files) error {
+	allFiles := make(map[string]*descriptorpb.FileDescriptorProto, len(files))
+	addFileDescriptorsToMap(files, allFiles)
+	fileSlice := make([]*descriptorpb.FileDescriptorProto, 0, len(allFiles))
+	for _, fileProto := range allFiles {
+		fileSlice = append(fileSlice, fileProto)
+	}
+	_, err := protodesc.NewFiles(&descriptorpb.FileDescriptorSet{File: fileSlice})
+	return err
+}
+
+func addFileDescriptorsToMap[F protoreflect.FileDescriptor](files []F, allFiles map[string]*descriptorpb.FileDescriptorProto) {
+	for _, file := range files {
+		if _, exists := allFiles[file.Path()]; exists {
+			continue // already added this one
+		}
+		allFiles[file.Path()] = protoutil.ProtoFromFileDescriptor(file)
+		deps := make([]protoreflect.FileDescriptor, file.Imports().Len())
+		for i := 0; i < file.Imports().Len(); i++ {
+			deps[i] = file.Imports().Get(i).FileDescriptor
+		}
+		addFileDescriptorsToMap(deps, allFiles)
+	}
+}

options/options.go

@@ -40,6 +40,7 @@ import (
 	"google.golang.org/protobuf/types/dynamicpb"
 
 	"github.com/kralicky/protocompile/ast"
+	"github.com/kralicky/protocompile/internal/messageset"
 	"github.com/kralicky/protocompile/linker"
 	"github.com/kralicky/protocompile/parser"
 	"github.com/kralicky/protocompile/protointernal"
@@ -679,7 +680,7 @@ func (interp *interpreter) checkFieldUsage(
 	node ast.Node,
 ) error {
 	msgOpts, _ := fld.ContainingMessage().Options().(*descriptorpb.MessageOptions)
-	if msgOpts.GetMessageSetWireFormat() && !canSerializeMessageSets() {
+	if msgOpts.GetMessageSetWireFormat() && !messageset.CanSupportMessageSets() {
 		err := interp.HandleOptionForbiddenErrorf(mc, node, "field %q may not be used in an option: it uses 'message set wire format' legacy proto1 feature which is not supported", fld.FullName())
 		if err != nil {
 			return err