Update desc/sourceinfo to not wrap descriptors (#622)

The `desc/sourceinfo` package previously wrapped descriptors, by
returning an implementation of the various `protoreflect.Descriptor`
interfaces that _embedded_ the original descriptor values. However,
these various interfaces are all defined as "[do not
implement](https://github.com/protocolbuffers/protobuf-go/blob/c33baa8f3a0d35fd5a39e43c22a50a050f707d34/reflect/protoreflect/type.go#L111)".
So, to avoid future issues where the protobuf runtime may type-assert
implementations to their own internal types (which would panic/fail with
the concrete wrapper types in this package), we no longer wrap this way.

This also happens to resolve #618 (since there's no more wrapping, an
incorrect typed-nil wrapper can't escape).

Author: jhump

desc/descriptor_test.go

@@ -472,8 +472,22 @@ func TestFileDescriptorObjectGraph(t *testing.T) {
 					},
 				},
 			}},
-			"enums":    {(*FileDescriptor).GetEnumTypes, nil},
-			"services": {(*FileDescriptor).GetServices, nil},
+			"enums": {(*FileDescriptor).GetEnumTypes, nil},
+			"services": {(*FileDescriptor).GetServices, []descCase{
+				{
+					name: "testprotos.SomeService",
+					references: map[string]childCases{
+						"methods": {(*ServiceDescriptor).GetMethods, []descCase{
+							{
+								name: "testprotos.SomeService.SomeRPC",
+							},
+							{
+								name: "testprotos.SomeService.SomeOtherRPC",
+							},
+						}},
+					},
+				},
+			}},
 			"extensions": {(*FileDescriptor).GetExtensions, []descCase{
 				{
 					name:   "testprotos.xtm",

desc/imports.go

@@ -34,10 +34,6 @@ var (
 // package or when the alternate path is only used from one file (so you don't
 // want the alternate path used when loading every other file), use an
 // ImportResolver instead.
-//
-// Deprecated: the new protobuf runtime (v1.4+) verifies that import paths are
-// correct and that descriptors can be linked during package initialization. So
-// registering alternate paths is no longer useful or necessary.
 func RegisterImportPath(registerPath, importPath string) {
 	if len(importPath) == 0 {
 		panic("import path cannot be empty")
@@ -60,10 +56,6 @@ func RegisterImportPath(registerPath, importPath string) {
 // ResolveImport resolves the given import path. If it has been registered as an
 // alternate via RegisterImportPath, the registered path is returned. Otherwise,
 // the given import path is returned unchanged.
-//
-// Deprecated: the new protobuf runtime (v1.4+) verifies that import paths are
-// correct and that descriptors can be linked during package initialization. So
-// registering alternate paths is no longer useful or necessary.
 func ResolveImport(importPath string) string {
 	importPath = clean(importPath)
 	globalImportPathMu.RLock()
@@ -244,76 +236,48 @@ func (r *ImportResolver) registerImportPathFrom(registerPath, importPath, source
 // LoadFileDescriptor is the same as the package function of the same name, but
 // any alternate paths configured in this resolver are used when linking the
 // given descriptor proto.
-//
-// Deprecated: the new protobuf runtime (v1.4+) verifies that import paths are
-// correct and that descriptors can be linked during package initialization. So
-// registering alternate paths is no longer useful or necessary.
 func (r *ImportResolver) LoadFileDescriptor(filePath string) (*FileDescriptor, error) {
 	return LoadFileDescriptor(filePath)
 }
 
 // LoadMessageDescriptor is the same as the package function of the same name,
 // but any alternate paths configured in this resolver are used when linking
 // files for the returned descriptor.
-//
-// Deprecated: the new protobuf runtime (v1.4+) verifies that import paths are
-// correct and that descriptors can be linked during package initialization. So
-// registering alternate paths is no longer useful or necessary.
 func (r *ImportResolver) LoadMessageDescriptor(msgName string) (*MessageDescriptor, error) {
 	return LoadMessageDescriptor(msgName)
 }
 
 // LoadMessageDescriptorForMessage is the same as the package function of the
 // same name, but any alternate paths configured in this resolver are used when
 // linking files for the returned descriptor.
-//
-// Deprecated: the new protobuf runtime (v1.4+) verifies that import paths are
-// correct and that descriptors can be linked during package initialization. So
-// registering alternate paths is no longer useful or necessary.
 func (r *ImportResolver) LoadMessageDescriptorForMessage(msg proto.Message) (*MessageDescriptor, error) {
 	return LoadMessageDescriptorForMessage(msg)
 }
 
 // LoadMessageDescriptorForType is the same as the package function of the same
 // name, but any alternate paths configured in this resolver are used when
 // linking files for the returned descriptor.
-//
-// Deprecated: the new protobuf runtime (v1.4+) verifies that import paths are
-// correct and that descriptors can be linked during package initialization. So
-// registering alternate paths is no longer useful or necessary.
 func (r *ImportResolver) LoadMessageDescriptorForType(msgType reflect.Type) (*MessageDescriptor, error) {
 	return LoadMessageDescriptorForType(msgType)
 }
 
 // LoadEnumDescriptorForEnum is the same as the package function of the same
 // name, but any alternate paths configured in this resolver are used when
 // linking files for the returned descriptor.
-//
-// Deprecated: the new protobuf runtime (v1.4+) verifies that import paths are
-// correct and that descriptors can be linked during package initialization. So
-// registering alternate paths is no longer useful or necessary.
 func (r *ImportResolver) LoadEnumDescriptorForEnum(enum protoEnum) (*EnumDescriptor, error) {
 	return LoadEnumDescriptorForEnum(enum)
 }
 
 // LoadEnumDescriptorForType is the same as the package function of the same
 // name, but any alternate paths configured in this resolver are used when
 // linking files for the returned descriptor.
-//
-// Deprecated: the new protobuf runtime (v1.4+) verifies that import paths are
-// correct and that descriptors can be linked during package initialization. So
-// registering alternate paths is no longer useful or necessary.
 func (r *ImportResolver) LoadEnumDescriptorForType(enumType reflect.Type) (*EnumDescriptor, error) {
 	return LoadEnumDescriptorForType(enumType)
 }
 
 // LoadFieldDescriptorForExtension is the same as the package function of the
 // same name, but any alternate paths configured in this resolver are used when
 // linking files for the returned descriptor.
-//
-// Deprecated: the new protobuf runtime (v1.4+) verifies that import paths are
-// correct and that descriptors can be linked during package initialization. So
-// registering alternate paths is no longer useful or necessary.
 func (r *ImportResolver) LoadFieldDescriptorForExtension(ext *proto.ExtensionDesc) (*FieldDescriptor, error) {
 	return LoadFieldDescriptorForExtension(ext)
 }

desc/load.go

@@ -1,6 +1,7 @@
 package desc
 
 import (
+	"errors"
 	"fmt"
 	"reflect"
 	"sync"
@@ -33,7 +34,7 @@ var (
 // re-processed if the same file is fetched again later.
 func LoadFileDescriptor(file string) (*FileDescriptor, error) {
 	d, err := sourceinfo.GlobalFiles.FindFileByPath(file)
-	if err == protoregistry.NotFound {
+	if errors.Is(err, protoregistry.NotFound) {
 		// for backwards compatibility, see if this matches a known old
 		// alias for the file (older versions of libraries that registered
 		// the files using incorrect/non-canonical paths)
@@ -42,7 +43,7 @@ func LoadFileDescriptor(file string) (*FileDescriptor, error) {
 		}
 	}
 	if err != nil {
-		if err != protoregistry.NotFound {
+		if !errors.Is(err, protoregistry.NotFound) {
 			return nil, internal.ErrNoSuchFile(file)
 		}
 		return nil, err
@@ -64,7 +65,7 @@ func LoadFileDescriptor(file string) (*FileDescriptor, error) {
 func LoadMessageDescriptor(message string) (*MessageDescriptor, error) {
 	mt, err := sourceinfo.GlobalTypes.FindMessageByName(protoreflect.FullName(message))
 	if err != nil {
-		if err == protoregistry.NotFound {
+		if errors.Is(err, protoregistry.NotFound) {
 			return nil, nil
 		}
 		return nil, err

desc/protoprint/testfiles/desc_test1-compact.proto

@@ -107,3 +107,10 @@ extend AnotherTestMessage {
   // Comment for xui
   optional uint64 xui = 103;
 }
+// Comment for SomeService
+service SomeService {
+  // Comment for SomeRPC
+  rpc SomeRPC ( TestMessage ) returns ( TestMessage );
+  // Comment for SomeOtherRPC
+  rpc SomeOtherRPC ( AnotherTestMessage ) returns ( AnotherTestMessage );
+}

desc/protoprint/testfiles/desc_test1-custom-sort.proto

@@ -18,6 +18,15 @@ extend AnotherTestMessage {
   optional string xs = 101;
 }
 
+// Comment for SomeService
+service SomeService {
+  // Comment for SomeRPC
+  rpc SomeRPC ( TestMessage ) returns ( TestMessage );
+
+  // Comment for SomeOtherRPC
+  rpc SomeOtherRPC ( AnotherTestMessage ) returns ( AnotherTestMessage );
+}
+
 // Comment for TestMessage
 message TestMessage {
   // Comment for NestedEnum

desc/protoprint/testfiles/desc_test1-default.proto

@@ -144,3 +144,12 @@ extend AnotherTestMessage {
   // Comment for xui
   optional uint64 xui = 103;
 }
+
+// Comment for SomeService
+service SomeService {
+  // Comment for SomeRPC
+  rpc SomeRPC ( TestMessage ) returns ( TestMessage );
+
+  // Comment for SomeOtherRPC
+  rpc SomeOtherRPC ( AnotherTestMessage ) returns ( AnotherTestMessage );
+}

desc/protoprint/testfiles/desc_test1-multiline-style-comments.proto

@@ -144,3 +144,12 @@ extend AnotherTestMessage {
 	/* Comment for xui */
 	optional uint64 xui = 103;
 }
+
+/* Comment for SomeService */
+service SomeService {
+	/* Comment for SomeRPC */
+	rpc SomeRPC ( TestMessage ) returns ( TestMessage );
+
+	/* Comment for SomeOtherRPC */
+	rpc SomeOtherRPC ( AnotherTestMessage ) returns ( AnotherTestMessage );
+}

desc/protoprint/testfiles/desc_test1-no-trailing-comments.proto

@@ -144,3 +144,12 @@ extend AnotherTestMessage {
   // Comment for xui
   optional uint64 xui = 103;
 }
+
+// Comment for SomeService
+service SomeService {
+  // Comment for SomeRPC
+  rpc SomeRPC ( TestMessage ) returns ( TestMessage );
+
+  // Comment for SomeOtherRPC
+  rpc SomeOtherRPC ( AnotherTestMessage ) returns ( AnotherTestMessage );
+}

desc/protoprint/testfiles/desc_test1-only-doc-comments.proto

@@ -144,3 +144,12 @@ extend AnotherTestMessage {
   // Comment for xui
   optional uint64 xui = 103;
 }
+
+// Comment for SomeService
+service SomeService {
+  // Comment for SomeRPC
+  rpc SomeRPC ( TestMessage ) returns ( TestMessage );
+
+  // Comment for SomeOtherRPC
+  rpc SomeOtherRPC ( AnotherTestMessage ) returns ( AnotherTestMessage );
+}

desc/protoprint/testfiles/desc_test1-sorted-AND-multiline-style-comments.proto

@@ -127,6 +127,15 @@ message TestMessage {
   }
 }
 
+/* Comment for SomeService */
+service SomeService {
+  /* Comment for SomeOtherRPC */
+  rpc SomeOtherRPC ( AnotherTestMessage ) returns ( AnotherTestMessage );
+
+  /* Comment for SomeRPC */
+  rpc SomeRPC ( TestMessage ) returns ( TestMessage );
+}
+
 /* Comment for AnotherTestMessage extensions (2) */
 extend AnotherTestMessage {
   /* Comment for xtm */

desc/protoprint/testfiles/desc_test1-sorted.proto

@@ -127,6 +127,15 @@ message TestMessage {
    }
 }
 
+// Comment for SomeService
+service SomeService {
+   // Comment for SomeOtherRPC
+   rpc SomeOtherRPC ( AnotherTestMessage ) returns ( AnotherTestMessage );
+
+   // Comment for SomeRPC
+   rpc SomeRPC ( TestMessage ) returns ( TestMessage );
+}
+
 // Comment for AnotherTestMessage extensions (2)
 extend AnotherTestMessage {
    // Comment for xtm

desc/protoprint/testfiles/desc_test1-trailing-on-next-line.proto

@@ -144,3 +144,12 @@ extend AnotherTestMessage {
   // Comment for xui
   optional uint64 xui = 103;
 }
+
+// Comment for SomeService
+service SomeService {
+  // Comment for SomeRPC
+  rpc SomeRPC ( TestMessage ) returns ( TestMessage );
+
+  // Comment for SomeOtherRPC
+  rpc SomeOtherRPC ( AnotherTestMessage ) returns ( AnotherTestMessage );
+}

desc/sourceinfo/export_test.go

@@ -0,0 +1,17 @@
+package sourceinfo
+
+import "google.golang.org/protobuf/reflect/protoreflect"
+
+// exported for tests
+
+func CanUpgrade(d protoreflect.Descriptor) bool {
+	return canUpgrade(d)
+}
+
+func UpdateDescriptor[D protoreflect.Descriptor](d D) (D, error) {
+	return updateDescriptor(d)
+}
+
+func UpdateField(fld protoreflect.FieldDescriptor) (protoreflect.FieldDescriptor, error) {
+	return updateField(fld)
+}