Repository: jhump/protoreflect (go)

Issue:
Regression upgrading from v1.14.1 to v1.15.4: extensions are resolved recursively instead of non-recursively
I noticed a behavioral difference when upgrading from protoreflect v1.14.1 to protoreflect v1.15.4.

When using protoprint to generate a .proto file, a few `import` lines are missing compared to before. The affected imports are other .proto files that declare option extensions.

I tracked this down to `desc/builder/resolver.go:resolveTypesInOptions` thinking the extension is already known and therefore not adding a dependency.

This, in turn, is caused by the following difference:

In v1.14.1, extensions are added non-recursively (`recursive == false`):

* https://github.com/jhump/protoreflect/blob/0facb742c9dcbc99667de77a4b74b842f9aa0bb3/desc/builder/resolver.go#L31
* https://github.com/jhump/protoreflect/blob/0facb742c9dcbc99667de77a4b74b842f9aa0bb3/dynamic/extension_registry.go#L71-L86

But in v1.15.4, extensions are added recursively (`publicImportsOnly == false`, which is equivalent to `recursive == true`):

* https://github.com/jhump/protoreflect/blob/f139a6debe4ec67cddb8e11df35b8a1934203c68/desc/builder/resolver.go#L36
* https://github.com/jhump/protoreflect/blob/f139a6debe4ec67cddb8e11df35b8a1934203c68/desc/internal/registry.go#L9-L25

I hope this is enough information to work with. Let me know if you can’t make progress without a proper standalone reproducer, though extracting one will take me a couple of days I expect…

Pull request description:
Fix issue with imports in desc/builder
When adding extensions to the type resolver that the builder used, the logic was adding too many. For a given import, it was incorrectly adding all extensions visible to that imported file, instead of just the extensions made visible to the _importing_ file.

Fixes #589 

Patch:
diff --git a/desc/builder/resolver.go b/desc/builder/resolver.go
index 5c2ee8c..47382ae 100644
--- a/desc/builder/resolver.go
+++ b/desc/builder/resolver.go
@@ -33,7 +33,7 @@ func (d *dependencies) add(fd *desc.FileDescriptor) {
 		return
 	}
 	d.descs[fd] = struct{}{}
-	internal.RegisterExtensionsForFile(&d.res, fd.UnwrapFile())
+	internal.RegisterExtensionsFromImportedFile(&d.res, fd.UnwrapFile())
 }
 
 // dependencyResolver is the work-horse for converting a tree of builders into a
@@ -150,7 +150,6 @@ func (r *dependencyResolver) resolveFile(fb *FileBuilder, root Builder, seen []B
 	fileNames := map[string]struct{}{}
 	for _, d := range depSlice {
 		addFileNames(d, fileNames)
-		fileNames[d.GetName()] = struct{}{}
 	}
 	unique := makeUnique(fp.GetName(), fileNames)
 	if unique != fp.GetName() {
@@ -481,7 +480,7 @@ func (r *dependencyResolver) resolveTypesInOptions(root Builder, fileExts *dynam
 			continue
 		}
 		// see if configured extension registry knows about it
-		if extd := r.opts.Extensions.FindExtension(string(msgName), int32(tag)); extd != nil {
+		if extd := r.opts.Extensions.FindExtension(msgName, tag); extd != nil {
 			// extension registry recognized it!
 			deps.add(extd.GetFile())
 			continue
diff --git a/desc/internal/registry.go b/desc/internal/registry.go
index 9f160a3..d7259e4 100644
--- a/desc/internal/registry.go
+++ b/desc/internal/registry.go
@@ -6,11 +6,30 @@ import (
 	"google.golang.org/protobuf/types/dynamicpb"
 )
 
-func RegisterExtensionsForFile(reg *protoregistry.Types, fd protoreflect.FileDescriptor) {
+// RegisterExtensionsFromImportedFile registers extensions in the given file as well
+// as those in its public imports. So if another file imports the given fd, this adds
+// all extensions made visible to that importing file.
+//
+// All extensions in the given file are made visible to the importing file, and so are
+// extensions in any public imports in the given file.
+func RegisterExtensionsFromImportedFile(reg *protoregistry.Types, fd protoreflect.FileDescriptor) {
+	registerTypesForFile(reg, fd, true, true)
+}
+
+// RegisterExtensionsVisibleToFile registers all extensions visible to the given file.
+// This includes all extensions defined in fd and as well as extensions defined in the
+// files that it imports (and any public imports thereof, etc).
+//
+// This is effectively the same as registering the extensions in fd and then calling
+// RegisterExtensionsFromImportedFile for each file imported by fd.
+func RegisterExtensionsVisibleToFile(reg *protoregistry.Types, fd protoreflect.FileDescriptor) {
 	registerTypesForFile(reg, fd, true, false)
 }
 
-func RegisterTypesForFile(reg *protoregistry.Types, fd protoreflect.FileDescriptor) {
+// RegisterTypesVisibleToFile registers all types visible to the given file.
+// This is the same as RegisterExtensionsVisibleToFile but it also registers
+// message and enum types, not just extensions.
+func RegisterTypesVisibleToFile(reg *protoregistry.Types, fd protoreflect.FileDescriptor) {
 	registerTypesForFile(reg, fd, false, false)
 }
 
diff --git a/desc/protoprint/print.go b/desc/protoprint/print.go
index c9fc2d9..481746b 100644
--- a/desc/protoprint/print.go
+++ b/desc/protoprint/print.go
@@ -319,7 +319,7 @@ func (p *Printer) printProto(dsc desc.Descriptor, out io.Writer) error {
 	extendOptionLocations(sourceInfo, fdp.GetSourceCodeInfo().GetLocation())
 
 	var reg protoregistry.Types
-	internal.RegisterTypesForFile(&reg, dsc.GetFile().UnwrapFile())
+	internal.RegisterTypesVisibleToFile(&reg, dsc.GetFile().UnwrapFile())
 	reparseUnknown(&reg, fdp.ProtoReflect())
 
 	path := findElement(dsc)


Test patch:
diff --git a/desc/builder/builder_test.go b/desc/builder/builder_test.go
index 1d57817..28c0a59 100644
--- a/desc/builder/builder_test.go
+++ b/desc/builder/builder_test.go
@@ -1155,18 +1155,35 @@ func TestCustomOptionsDiscoveredInDependencies(t *testing.T) {
 	fileDesc, err := file.Build()
 	testutil.Ok(t, err)
 
+	// Another file that imports the options. Since it's not a public import, presence
+	// of this file should not prevent builder from correctly adding options.proto dependency
+	// in the "auto" case below.
+	otherFileDesc, err := NewFile("other.proto").AddImportedDependency(fileDesc).Build()
+	testutil.Ok(t, err)
+
+	regWithOpts := &dynamic.ExtensionRegistry{}
+	regWithOpts.AddExtensionsFromFileRecursively(fileDesc)
+
 	// Now we can test referring to these and making sure they show up correctly
 	// in built descriptors
-	for name, useBuilder := range map[string]bool{"descriptor": false, "builder": true} {
+	for name, useBuilder := range map[string]*bool{"descriptor": proto.Bool(false), "builder": proto.Bool(true), "auto": nil} {
 		newFile := func() *FileBuilder {
-			fb := NewFile("foo.proto")
-			if useBuilder {
-				fb.AddDependency(file)
-			} else {
-				fb.AddImportedDependency(fileDesc)
+			fb := NewFile("foo.proto").AddImportedDependency(otherFileDesc)
+			if useBuilder != nil {
+				if *useBuilder {
+					fb.AddDependency(file)
+				} else {
+					fb.AddImportedDependency(fileDesc)
+				}
 			}
 			return fb
 		}
+		var extReg *dynamic.ExtensionRegistry
+		if useBuilder == nil {
+			// if providing neither builder nor descriptor, we need to provide
+			// a registry for resolving custom options
+			extReg = regWithOpts
+		}
 		t.Run(name, func(t *testing.T) {
 			t.Run("file options", func(t *testing.T) {
 				fb := newFile()
@@ -1175,7 +1192,7 @@ func TestCustomOptionsDiscoveredInDependencies(t *testing.T) {
 				testutil.Ok(t, err)
 				err = dynamic.SetExtension(fb.Options, ext, "fubar")
 				testutil.Ok(t, err)
-				checkBuildWithImportedExtensions(t, fb)
+				checkBuildWithImportedExtensions(t, fb, extReg)
 			})
 
 			t.Run("message options", func(t *testing.T) {
@@ -1188,7 +1205,7 @@ func TestCustomOptionsDiscoveredInDependencies(t *testing.T) {
 
 				fb := newFile()
 				fb.AddMessage(mb)
-				checkBuildWithImportedExtensions(t, mb)
+				checkBuildWithImportedExtensions(t, mb, extReg)
 			})
 
 			t.Run("field options", func(t *testing.T) {
@@ -1203,7 +1220,7 @@ func TestCustomOptionsDiscoveredInDependencies(t *testing.T) {
 
 				fb := newFile()
 				fb.AddMessage(mb)
-				checkBuildWithImportedExtensions(t, flb)
+				checkBuildWithImportedExtensions(t, flb, extReg)
 			})
 
 			t.Run("oneof options", func(t *testing.T) {
@@ -1219,7 +1236,7 @@ func TestCustomOptionsDiscoveredInDependencies(t *testing.T) {
 
 				fb := newFile()
 				fb.AddMessage(mb)
-				checkBuildWithImportedExtensions(t, oob)
+				checkBuildWithImportedExtensions(t, oob, extReg)
 			})
 
 			t.Run("extension range options", func(t *testing.T) {
@@ -1232,7 +1249,7 @@ func TestCustomOptionsDiscoveredInDependencies(t *testing.T) {
 
 				fb := newFile()
 				fb.AddMessage(mb)
-				checkBuildWithImportedExtensions(t, mb)
+				checkBuildWithImportedExtensions(t, mb, extReg)
 			})
 
 			t.Run("enum options", func(t *testing.T) {
@@ -1246,7 +1263,7 @@ func TestCustomOptionsDiscoveredInDependencies(t *testing.T) {
 
 				fb := newFile()
 				fb.AddEnum(eb)
-				checkBuildWithImportedExtensions(t, eb)
+				checkBuildWithImportedExtensions(t, eb, extReg)
 			})
 
 			t.Run("enum val options", func(t *testing.T) {
@@ -1261,7 +1278,7 @@ func TestCustomOptionsDiscoveredInDependencies(t *testing.T) {
 
 				fb := newFile()
 				fb.AddEnum(eb)
-				checkBuildWithImportedExtensions(t, evb)
+				checkBuildWithImportedExtensions(t, evb, extReg)
 			})
 
 			t.Run("service options", func(t *testing.T) {
@@ -1274,7 +1291,7 @@ func TestCustomOptionsDiscoveredInDependencies(t *testing.T) {
 
 				fb := newFile()
 				fb.AddService(sb)
-				checkBuildWithImportedExtensions(t, sb)
+				checkBuildWithImportedExtensions(t, sb, extReg)
 			})
 
 			t.Run("method options", func(t *testing.T) {
@@ -1293,20 +1310,22 @@ func TestCustomOptionsDiscoveredInDependencies(t *testing.T) {
 
 				fb := newFile()
 				fb.AddService(sb).AddMessage(req).AddMessage(resp)
-				checkBuildWithImportedExtensions(t, mtb)
+				checkBuildWithImportedExtensions(t, mtb, extReg)
 			})
 		})
 	}
 }
 
-func checkBuildWithImportedExtensions(t *testing.T, builder Builder) {
+func checkBuildWithImportedExtensions(t *testing.T, builder Builder, extReg *dynamic.ExtensionRegistry) {
 	// requiring options and succeeding (since they are defined in explicit import)
-	var opts BuilderOptions
-	opts.RequireInterpretedOptions = true
+	opts := BuilderOptions{
+		RequireInterpretedOptions: true,
+		Extensions:                extReg,
+	}
 	d, err := opts.Build(builder)
 	testutil.Ok(t, err)
-	// the only import is for the custom options
-	testutil.Eq(t, []string{"options.proto"}, d.GetFile().AsFileDescriptorProto().GetDependency())
+	// the only import is the explicitly added one and one added for the custom options
+	testutil.Eq(t, []string{"options.proto", "other.proto"}, d.GetFile().AsFileDescriptorProto().GetDependency())
 }
 
 func TestUseOfExtensionRegistry(t *testing.T) {
diff --git a/desc/protoparse/source_code_info_test.go b/desc/protoparse/source_code_info_test.go
index e3062c0..8989804 100644
--- a/desc/protoparse/source_code_info_test.go
+++ b/desc/protoparse/source_code_info_test.go
@@ -37,9 +37,9 @@ func TestSourceCodeInfo(t *testing.T) {
 	// (human readable so diffs in source control are comprehensible)
 	var buf bytes.Buffer
 	for _, fd := range fds {
-		printSourceCodeInfo(t, fd, &buf)
+		printSourceCodeInfo(fd, &buf)
 	}
-	printSourceCodeInfo(t, importedFd, &buf)
+	printSourceCodeInfo(importedFd, &buf)
 	actual := buf.String()
 
 	if regenerateMode {
@@ -58,11 +58,11 @@ func TestSourceCodeInfo(t *testing.T) {
 // NB: this function can be used to manually inspect the source code info for a
 // descriptor, in a manner that is much easier to read and check than raw
 // descriptor form.
-func printSourceCodeInfo(t *testing.T, fd *desc.FileDescriptor, out io.Writer) {
+func printSourceCodeInfo(fd *desc.FileDescriptor, out io.Writer) {
 	_, _ = fmt.Fprintf(out, "---- %s ----\n", fd.GetName())
 	msg := fd.AsFileDescriptorProto().ProtoReflect()
 	var reg protoregistry.Types
-	internal.RegisterExtensionsForFile(&reg, fd.UnwrapFile())
+	internal.RegisterExtensionsVisibleToFile(&reg, fd.UnwrapFile())
 
 	for _, loc := range fd.AsFileDescriptorProto().GetSourceCodeInfo().GetLocation() {
 		var buf bytes.Buffer