reflect/protodesc: robustify dependency resolution implementation

Overview of changes:
* Add an option that specifies whether to replace unresolvable references
with a placeholder instead of producing an error. Since the prior behavior
produced placeholders (not always), we default to that behavior for now,
but will enable strict resolving in a future CL.
* The option is not yet exported because there is concern about what the
public API should look like. This will be exposed in a future CL.
* Unlike before, we now permit placeholders for unresolvable enum values.
* We implement relative name resolution logic.
* We handle the case where the type is unknown, but type_name is specified.
In such a case, we populate both FieldDescriptor.{Enum,Message} and leave
the FieldDescriptor.Kind with the zero value. If the type_name happened
to resolve, we use that to determine the type.
* If a placeholder is used to represent a relative name,
the FullName reports an invalid full name with a "*." prefix.

Change-Id: Ifa8c750423c488fb9324eec4d033a2f251505fda
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/184317
Reviewed-by: Damien Neil <dneil@google.com>

internal/encoding/defval/default.go

@@ -112,7 +112,7 @@ func Unmarshal(s string, k pref.Kind, evs pref.EnumValueDescriptors, f Format) (
 			return pref.ValueOf(b), nil, nil
 		}
 	}
-	return pref.Value{}, nil, errors.New("invalid default value for %v: %q", k, s)
+	return pref.Value{}, nil, errors.New("could not parse value for %v: %q", k, s)
 }
 
 // Marshal serializes v as the default string according to the given kind k.
@@ -168,7 +168,7 @@ func Marshal(v pref.Value, ev pref.EnumValueDescriptor, k pref.Kind, f Format) (
 			return s, nil
 		}
 	}
-	return "", errors.New("invalid default value for %v: %v", k, v)
+	return "", errors.New("could not format value for %v: %v", k, v)
 }
 
 // unmarshalBytes deserializes bytes by applying C unescaping.

reflect/protodesc/desc.go

@@ -9,6 +9,7 @@ package protodesc
 import (
 	"google.golang.org/protobuf/internal/errors"
 	"google.golang.org/protobuf/internal/filedesc"
+	"google.golang.org/protobuf/internal/pragma"
 	"google.golang.org/protobuf/reflect/protoreflect"
 	"google.golang.org/protobuf/reflect/protoregistry"
 
@@ -22,6 +23,36 @@ type Resolver interface {
 	FindDescriptorByName(protoreflect.FullName) (protoreflect.Descriptor, error)
 }
 
+// option is an optional argument that may be provided to NewFile.
+type option struct {
+	pragma.DoNotCompare
+	allowUnresolvable bool
+}
+
+// allowUnresolvable configures NewFile to permissively allow unresolvable
+// file, enum, or message dependencies. Unresolved dependencies are replaced by
+// placeholder equivalents.
+//
+// The following dependencies may be left unresolved:
+//	• Resolving an imported file.
+//	• Resolving the type for a message field or extension field.
+//	If the kind of the field is unknown, then a placeholder is used for both
+//	protoreflect.FieldDescriptor.Enum and protoreflect.FieldDescriptor.Message.
+//	• Resolving the enum value set as the default for an optional enum field.
+//	If unresolvable, the protoreflect.FieldDescriptor.Default is set to the
+//	first enum value in the associated enum (or zero if the also enum dependency
+//	is also unresolvable). The protoreflect.FieldDescriptor.DefaultEnumValue
+//	is set as a placeholder.
+//	• Resolving the extended message type for an extension field.
+//	• Resolving the input or output message type for a service method.
+//
+// If the unresolved dependency uses a relative name, then the placeholder will
+// contain an invalid FullName with a "*." prefix, indicating that the starting
+// prefix of the full name is unknown.
+func allowUnresolvable() option {
+	return option{allowUnresolvable: true}
+}
+
 // NewFile creates a new protoreflect.FileDescriptor from the provided
 // file descriptor message. The file must represent a valid proto file according
 // to protobuf semantics. The returned descriptor is a deep copy of the input.
@@ -31,9 +62,20 @@ type Resolver interface {
 // the path must be unique. The newly created file descriptor is not registered
 // back into the provided file registry.
 func NewFile(fd *descriptorpb.FileDescriptorProto, r Resolver) (protoreflect.FileDescriptor, error) {
+	// TODO: remove setting allowUnresolvable once naughty users are migrated.
+	return newFile(fd, r, allowUnresolvable())
+}
+func newFile(fd *descriptorpb.FileDescriptorProto, r Resolver, opts ...option) (protoreflect.FileDescriptor, error) {
+	// Process the options.
+	var allowUnresolvable bool
+	for _, o := range opts {
+		allowUnresolvable = allowUnresolvable || o.allowUnresolvable
+	}
 	if r == nil {
 		r = (*protoregistry.Files)(nil) // empty resolver
 	}
+
+	// Handle the file descriptor content.
 	f := &filedesc.File{L2: &filedesc.FileL2{}}
 	switch fd.GetSyntax() {
 	case "proto2", "":
@@ -67,9 +109,10 @@ func NewFile(fd *descriptorpb.FileDescriptorProto, r Resolver) (protoreflect.Fil
 	for i, path := range fd.GetDependency() {
 		imp := &f.L2.Imports[i]
 		f, err := r.FindFileByPath(path)
-		if err != nil {
-			// TODO: This should be an error.
+		if err == protoregistry.NotFound && (allowUnresolvable || imp.IsWeak) {
 			f = filedesc.PlaceholderFile(path)
+		} else if err != nil {
+			return nil, errors.New("could not resolve import %q: %v", path, err)
 		}
 		imp.FileDescriptor = f
 
@@ -101,7 +144,7 @@ func NewFile(fd *descriptorpb.FileDescriptorProto, r Resolver) (protoreflect.Fil
 	}
 
 	// Step 2: Resolve every dependency reference not handled by step 1.
-	r2 := &resolver{local: r1, remote: r, imports: imps}
+	r2 := &resolver{local: r1, remote: r, imports: imps, allowUnresolvable: allowUnresolvable}
 	if err := r2.resolveMessageDependencies(f.L1.Messages.List, fd.GetMessageType()); err != nil {
 		return nil, err
 	}
@@ -136,11 +179,3 @@ func (is importSet) importPublic(imps protoreflect.FileImports) {
 		}
 	}
 }
-
-// check returns an error if d does not belong to a currently imported file.
-func (is importSet) check(d protoreflect.Descriptor) error {
-	if !is[d.ParentFile().Path()] {
-		return errors.New("reference to type %v without import of %v", d.FullName(), d.ParentFile().Path())
-	}
-	return nil
-}

reflect/protodesc/desc_resolve.go

@@ -5,8 +5,6 @@
 package protodesc
 
 import (
-	"strings"
-
 	"google.golang.org/protobuf/internal/encoding/defval"
 	"google.golang.org/protobuf/internal/errors"
 	"google.golang.org/protobuf/internal/filedesc"
@@ -16,10 +14,15 @@ import (
 	"google.golang.org/protobuf/types/descriptorpb"
 )
 
+// resolver is a wrapper around a local registry of declarations within the file
+// and the remote resolver. The remote resolver is restricted to only return
+// descriptors that have been imported.
 type resolver struct {
 	local   descsByName
 	remote  Resolver
 	imports importSet
+
+	allowUnresolvable bool
 }
 
 func (r *resolver) resolveMessageDependencies(ms []filedesc.Message, mds []*descriptorpb.DescriptorProto) (err error) {
@@ -32,41 +35,21 @@ func (r *resolver) resolveMessageDependencies(ms []filedesc.Message, mds []*desc
 			}
 			if fd.OneofIndex != nil {
 				k := int(fd.GetOneofIndex())
-				if k >= len(md.GetOneofDecl()) {
-					return errors.New("invalid oneof index: %d", k)
+				if !(0 <= k && k < len(md.GetOneofDecl())) {
+					return errors.New("message field %q has an invalid oneof index: %d", f.FullName(), k)
 				}
 				o := &m.L2.Oneofs.List[k]
 				f.L1.ContainingOneof = o
 				o.L1.Fields.List = append(o.L1.Fields.List, f)
 			}
-			switch f.L1.Kind {
-			case protoreflect.EnumKind:
-				ed, err := findEnumDescriptor(fd.GetTypeName(), f.L1.IsWeak, r)
-				if err != nil {
-					return err
-				}
-				f.L1.Enum = ed
-			case protoreflect.MessageKind, protoreflect.GroupKind:
-				md, err := findMessageDescriptor(fd.GetTypeName(), f.L1.IsWeak, r)
-				if err != nil {
-					return err
-				}
-				f.L1.Message = md
-			default:
-				if fd.GetTypeName() != "" {
-					return errors.New("field of kind %v has type_name set", f.L1.Kind)
-				}
+
+			if f.L1.Kind, f.L1.Enum, f.L1.Message, err = r.findTarget(f.Kind(), f.Parent().FullName(), partialName(fd.GetTypeName()), f.IsWeak()); err != nil {
+				return errors.New("message field %q cannot resolve type: %v", f.FullName(), err)
 			}
 			if fd.DefaultValue != nil {
-				// Handle default value after resolving the enum since the
-				// list of enum values is needed to resolve enums by name.
-				var evs protoreflect.EnumValueDescriptors
-				if f.L1.Kind == protoreflect.EnumKind {
-					evs = f.L1.Enum.Values()
-				}
-				v, ev, err := defval.Unmarshal(fd.GetDefaultValue(), f.L1.Kind, evs, defval.Descriptor)
+				v, ev, err := unmarshalDefault(fd.GetDefaultValue(), f, r.allowUnresolvable)
 				if err != nil {
-					return err
+					return errors.New("message field %q has invalid default: %v", f.FullName(), err)
 				}
 				f.L1.Default = filedesc.DefaultValue(v, ev)
 			}
@@ -82,42 +65,19 @@ func (r *resolver) resolveMessageDependencies(ms []filedesc.Message, mds []*desc
 	return nil
 }
 
-func (r *resolver) resolveExtensionDependencies(xs []filedesc.Extension, xds []*descriptorpb.FieldDescriptorProto) error {
+func (r *resolver) resolveExtensionDependencies(xs []filedesc.Extension, xds []*descriptorpb.FieldDescriptorProto) (err error) {
 	for i, xd := range xds {
 		x := &xs[i]
-		md, err := findMessageDescriptor(xd.GetExtendee(), false, r)
-		if err != nil {
-			return err
+		if x.L1.Extendee, err = r.findMessageDescriptor(x.Parent().FullName(), partialName(xd.GetExtendee()), false); err != nil {
+			return errors.New("extension field %q cannot resolve extendee: %v", x.FullName(), err)
 		}
-		x.L1.Extendee = md
-		switch x.L1.Kind {
-		case protoreflect.EnumKind:
-			ed, err := findEnumDescriptor(xd.GetTypeName(), false, r)
-			if err != nil {
-				return err
-			}
-			x.L2.Enum = ed
-		case protoreflect.MessageKind, protoreflect.GroupKind:
-			md, err := findMessageDescriptor(xd.GetTypeName(), false, r)
-			if err != nil {
-				return err
-			}
-			x.L2.Message = md
-		default:
-			if xd.GetTypeName() != "" {
-				return errors.New("field of kind %v has type_name set", x.L1.Kind)
-			}
+		if x.L1.Kind, x.L2.Enum, x.L2.Message, err = r.findTarget(x.Kind(), x.Parent().FullName(), partialName(xd.GetTypeName()), false); err != nil {
+			return errors.New("extension field %q cannot resolve type: %v", x.FullName(), err)
 		}
 		if xd.DefaultValue != nil {
-			// Handle default value after resolving the enum since the
-			// list of enum values is needed to resolve enums by name.
-			var evs protoreflect.EnumValueDescriptors
-			if x.L1.Kind == protoreflect.EnumKind {
-				evs = x.L2.Enum.Values()
-			}
-			v, ev, err := defval.Unmarshal(xd.GetDefaultValue(), x.L1.Kind, evs, defval.Descriptor)
+			v, ev, err := unmarshalDefault(xd.GetDefaultValue(), x, r.allowUnresolvable)
 			if err != nil {
-				return err
+				return errors.New("extension field %q has invalid default: %v", x.FullName(), err)
 			}
 			x.L2.Default = filedesc.DefaultValue(v, ev)
 		}
@@ -130,84 +90,193 @@ func (r *resolver) resolveServiceDependencies(ss []filedesc.Service, sds []*desc
 		s := &ss[i]
 		for j, md := range sd.GetMethod() {
 			m := &s.L2.Methods.List[j]
-			m.L1.Input, err = findMessageDescriptor(md.GetInputType(), false, r)
+			m.L1.Input, err = r.findMessageDescriptor(m.Parent().FullName(), partialName(md.GetInputType()), false)
 			if err != nil {
-				return err
+				return errors.New("service method %q cannot resolve input: %v", m.FullName(), err)
 			}
-			m.L1.Output, err = findMessageDescriptor(md.GetOutputType(), false, r)
+			m.L1.Output, err = r.findMessageDescriptor(s.FullName(), partialName(md.GetOutputType()), false)
 			if err != nil {
-				return err
+				return errors.New("service method %q cannot resolve output: %v", m.FullName(), err)
 			}
 		}
 	}
 	return nil
 }
 
-// TODO: Should we allow relative names? The protoc compiler has emitted
-// absolute names for some time now. Requiring absolute names as an input
-// simplifies our implementation as we won't need to implement C++'s namespace
-// scoping rules.
-
-func (r resolver) FindFileByPath(s string) (protoreflect.FileDescriptor, error) {
-	return r.remote.FindFileByPath(s)
+// findTarget finds an enum or message descriptor if k is an enum, message,
+// group, or unknown. If unknown, and the name could be resolved, the kind
+// returned kind is set based on the type of the resolved descriptor.
+func (r *resolver) findTarget(k protoreflect.Kind, scope protoreflect.FullName, ref partialName, isWeak bool) (protoreflect.Kind, protoreflect.EnumDescriptor, protoreflect.MessageDescriptor, error) {
+	switch k {
+	case protoreflect.EnumKind:
+		ed, err := r.findEnumDescriptor(scope, ref, isWeak)
+		if err != nil {
+			return 0, nil, nil, err
+		}
+		return k, ed, nil, nil
+	case protoreflect.MessageKind, protoreflect.GroupKind:
+		md, err := r.findMessageDescriptor(scope, ref, isWeak)
+		if err != nil {
+			return 0, nil, nil, err
+		}
+		return k, nil, md, nil
+	case 0:
+		// Handle unspecified kinds (possible with parsers that operate
+		// on a per-file basis without knowledge of dependencies).
+		d, err := r.findDescriptor(scope, ref)
+		if err == protoregistry.NotFound && (r.allowUnresolvable || isWeak) {
+			return k, filedesc.PlaceholderEnum(ref.FullName()), filedesc.PlaceholderMessage(ref.FullName()), nil
+		} else if err == protoregistry.NotFound {
+			return 0, nil, nil, errors.New("%q not found", ref.FullName())
+		} else if err != nil {
+			return 0, nil, nil, err
+		}
+		switch d := d.(type) {
+		case protoreflect.EnumDescriptor:
+			return protoreflect.EnumKind, d, nil, nil
+		case protoreflect.MessageDescriptor:
+			return protoreflect.MessageKind, nil, d, nil
+		default:
+			return 0, nil, nil, errors.New("unknown kind")
+		}
+	default:
+		if ref != "" {
+			return 0, nil, nil, errors.New("target name cannot be specified for %v", k)
+		}
+		if !k.IsValid() {
+			return 0, nil, nil, errors.New("invalid kind: %d", k)
+		}
+		return k, nil, nil, nil
+	}
 }
 
-func (r resolver) FindDescriptorByName(s protoreflect.FullName) (protoreflect.Descriptor, error) {
-	if d, ok := r.local[s]; ok {
-		return d, nil
+// findDescriptor finds the descriptor by name,
+// which may be a relative name within some scope.
+//
+// Suppose the scope was "fizz.buzz" and the reference was "Foo.Bar",
+// then the following full names are searched:
+//	* fizz.buzz.Foo.Bar
+//	* fizz.Foo.Bar
+//	* Foo.Bar
+func (r *resolver) findDescriptor(scope protoreflect.FullName, ref partialName) (protoreflect.Descriptor, error) {
+	if !ref.IsValid() {
+		return nil, errors.New("invalid name reference: %q", ref)
+	}
+	if ref.IsFull() {
+		scope, ref = "", ref[1:]
+	}
+	var foundButNotImported protoreflect.Descriptor
+	for {
+		// Derive the full name to search.
+		s := protoreflect.FullName(ref)
+		if scope != "" {
+			s = scope + "." + s
+		}
+
+		// Check the current file for the descriptor.
+		if d, ok := r.local[s]; ok {
+			return d, nil
+		}
+
+		// Check the remote registry for the descriptor.
+		d, err := r.remote.FindDescriptorByName(s)
+		if err == nil {
+			// Only allow descriptors covered by one of the imports.
+			if r.imports[d.ParentFile().Path()] {
+				return d, nil
+			}
+			foundButNotImported = d
+		} else if err != protoregistry.NotFound {
+			return nil, err
+		}
+
+		// Continue on at a higher level of scoping.
+		if scope == "" {
+			if d := foundButNotImported; d != nil {
+				return nil, errors.New("resolved %q, but %q is not imported", d.FullName(), d.ParentFile().Path())
+			}
+			return nil, protoregistry.NotFound
+		}
+		scope = scope.Parent()
 	}
-	return r.remote.FindDescriptorByName(s)
 }
 
-func findEnumDescriptor(s string, isWeak bool, r *resolver) (protoreflect.EnumDescriptor, error) {
-	d, err := findDescriptor(s, isWeak, r)
-	if err != nil {
+func (r *resolver) findEnumDescriptor(scope protoreflect.FullName, ref partialName, isWeak bool) (protoreflect.EnumDescriptor, error) {
+	d, err := r.findDescriptor(scope, ref)
+	if err == protoregistry.NotFound && (r.allowUnresolvable || isWeak) {
+		return filedesc.PlaceholderEnum(ref.FullName()), nil
+	} else if err == protoregistry.NotFound {
+		return nil, errors.New("%q not found", ref.FullName())
+	} else if err != nil {
 		return nil, err
 	}
-	if ed, ok := d.(protoreflect.EnumDescriptor); ok {
-		if err == protoregistry.NotFound {
-			if isWeak {
-				return filedesc.PlaceholderEnum(protoreflect.FullName(s[1:])), nil
-			}
-			// TODO: This should be an error.
-			return filedesc.PlaceholderEnum(protoreflect.FullName(s[1:])), nil
-			// return nil, errors.New("could not resolve enum: %v", name)
-		}
-		return ed, nil
+	ed, ok := d.(protoreflect.EnumDescriptor)
+	if !ok {
+		return nil, errors.New("resolved %q, but it is not an enum", d.FullName())
 	}
-	return nil, errors.New("invalid descriptor type")
+	return ed, nil
 }
 
-func findMessageDescriptor(s string, isWeak bool, r *resolver) (protoreflect.MessageDescriptor, error) {
-	d, err := findDescriptor(s, isWeak, r)
-	if err != nil {
-		if err == protoregistry.NotFound {
-			if isWeak {
-				return filedesc.PlaceholderMessage(protoreflect.FullName(s[1:])), nil
-			}
-			// TODO: This should be an error.
-			return filedesc.PlaceholderMessage(protoreflect.FullName(s[1:])), nil
-			// return nil, errors.New("could not resolve enum: %v", name)
-		}
+func (r *resolver) findMessageDescriptor(scope protoreflect.FullName, ref partialName, isWeak bool) (protoreflect.MessageDescriptor, error) {
+	d, err := r.findDescriptor(scope, ref)
+	if err == protoregistry.NotFound && (r.allowUnresolvable || isWeak) {
+		return filedesc.PlaceholderMessage(ref.FullName()), nil
+	} else if err == protoregistry.NotFound {
+		return nil, errors.New("%q not found", ref.FullName())
+	} else if err != nil {
 		return nil, err
 	}
-	if md, ok := d.(protoreflect.MessageDescriptor); ok {
-		return md, nil
+	md, ok := d.(protoreflect.MessageDescriptor)
+	if !ok {
+		return nil, errors.New("resolved %q, but it is not an message", d.FullName())
 	}
-	return nil, errors.New("invalid descriptor type")
+	return md, nil
 }
 
-func findDescriptor(s string, isWeak bool, r *resolver) (protoreflect.Descriptor, error) {
-	if !strings.HasPrefix(s, ".") {
-		return nil, errors.New("identifier name must be fully qualified with a leading dot: %v", s)
+// partialName is the partial name. A leading dot means that the name is full,
+// otherwise the name is relative to some current scope.
+// See google.protobuf.FieldDescriptorProto.type_name.
+type partialName string
+
+func (s partialName) IsFull() bool {
+	return len(s) > 0 && s[0] == '.'
+}
+
+func (s partialName) IsValid() bool {
+	if s.IsFull() {
+		return protoreflect.FullName(s[1:]).IsValid()
 	}
-	name := protoreflect.FullName(strings.TrimPrefix(s, "."))
-	d, err := r.FindDescriptorByName(name)
-	if err != nil {
-		return nil, err
+	return protoreflect.FullName(s).IsValid()
+}
+
+const unknownPrefix = "*."
+
+// FullName converts the partial name to a full name on a best-effort basis.
+// If relative, it creates an invalid full name, using a "*." prefix
+// to indicate that the start of the full name is unknown.
+func (s partialName) FullName() protoreflect.FullName {
+	if s.IsFull() {
+		return protoreflect.FullName(s[1:])
 	}
-	if err := r.imports.check(d); err != nil {
-		return nil, err
+	return protoreflect.FullName(unknownPrefix + s)
+}
+
+func unmarshalDefault(s string, fd protoreflect.FieldDescriptor, allowUnresolvable bool) (protoreflect.Value, protoreflect.EnumValueDescriptor, error) {
+	var evs protoreflect.EnumValueDescriptors
+	if fd.Enum() != nil {
+		evs = fd.Enum().Values()
+	}
+	v, ev, err := defval.Unmarshal(s, fd.Kind(), evs, defval.Descriptor)
+	if err != nil && allowUnresolvable && evs != nil && protoreflect.Name(s).IsValid() {
+		v = protoreflect.ValueOf(protoreflect.EnumNumber(0))
+		if evs.Len() > 0 {
+			v = protoreflect.ValueOf(evs.Get(0).Number())
+		}
+		ev = filedesc.PlaceholderEnumValue(fd.Enum().FullName().Parent().Append(protoreflect.Name(s)))
+	} else if err != nil {
+		return v, ev, err
 	}
-	return d, nil
+	// TODO: Validate that the default is not specified under proto3.
+	// TODO: Validate that the default is not specified for composite types.
+	return v, ev, nil
 }

reflect/protodesc/file_test.go

@@ -0,0 +1,249 @@
+// Copyright 2019 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package protodesc
+
+import (
+	"fmt"
+	"strings"
+	"testing"
+
+	"google.golang.org/protobuf/encoding/prototext"
+	"google.golang.org/protobuf/proto"
+	"google.golang.org/protobuf/reflect/protoregistry"
+
+	"google.golang.org/protobuf/types/descriptorpb"
+)
+
+func mustParseFile(s string) *descriptorpb.FileDescriptorProto {
+	pb := new(descriptorpb.FileDescriptorProto)
+	if err := prototext.Unmarshal([]byte(s), pb); err != nil {
+		panic(err)
+	}
+	return pb
+}
+
+func cloneFile(in *descriptorpb.FileDescriptorProto) *descriptorpb.FileDescriptorProto {
+	out := new(descriptorpb.FileDescriptorProto)
+	proto.Merge(out, in)
+	return out
+}
+
+func TestNewFile(t *testing.T) {
+	tests := []struct {
+		label    string
+		inDeps   []*descriptorpb.FileDescriptorProto
+		inDesc   *descriptorpb.FileDescriptorProto
+		inOpts   []option
+		wantDesc *descriptorpb.FileDescriptorProto
+		wantErr  string
+	}{{
+		label: "resolve relative reference",
+		inDesc: mustParseFile(`
+			name: "test.proto"
+			package: "fizz.buzz"
+			message_type: [{
+				name: "A"
+				field: [{name:"F" number:1 label:LABEL_OPTIONAL type:TYPE_MESSAGE type_name:"B.C"}]
+				nested_type: [{name: "B"}]
+			}, {
+				name: "B"
+				nested_type: [{name: "C"}]
+			}]
+		`),
+		wantDesc: mustParseFile(`
+			name: "test.proto"
+			package: "fizz.buzz"
+			message_type: [{
+				name: "A"
+				field: [{name:"F" number:1 label:LABEL_OPTIONAL type:TYPE_MESSAGE type_name:".fizz.buzz.B.C"}]
+				nested_type: [{name: "B"}]
+			}, {
+				name: "B"
+				nested_type: [{name: "C"}]
+			}]
+		`),
+	}, {
+		label: "resolve the wrong type",
+		inDesc: mustParseFile(`
+			name: "test.proto"
+			package: ""
+			message_type: [{
+				name: "M"
+				field: [{name:"F" number:1 label:LABEL_OPTIONAL type:TYPE_MESSAGE type_name:"E"}]
+				enum_type: [{name: "E" value: [{name:"V0" number:0}, {name:"V1" number:1}]}]
+			}]
+		`),
+		wantErr: `message field "M.F" cannot resolve type: resolved "M.E", but it is not an message`,
+	}, {
+		label: "auto-resolve unknown kind",
+		inDesc: mustParseFile(`
+			name: "test.proto"
+			package: ""
+			message_type: [{
+				name: "M"
+				field: [{name:"F" number:1 label:LABEL_OPTIONAL type_name:"E"}]
+				enum_type: [{name: "E" value: [{name:"V0" number:0}, {name:"V1" number:1}]}]
+			}]
+		`),
+		wantDesc: mustParseFile(`
+			name: "test.proto"
+			package: ""
+			message_type: [{
+				name: "M"
+				field: [{name:"F" number:1 label:LABEL_OPTIONAL type:TYPE_ENUM type_name:".M.E"}]
+				enum_type: [{name: "E" value: [{name:"V0" number:0}, {name:"V1" number:1}]}]
+			}]
+		`),
+	}, {
+		label: "unresolved import",
+		inDesc: mustParseFile(`
+			name: "test.proto"
+			package: "fizz.buzz"
+			dependency: "remote.proto"
+		`),
+		wantErr: `could not resolve import "remote.proto": not found`,
+	}, {
+		label: "unresolved message field",
+		inDesc: mustParseFile(`
+			name: "test.proto"
+			package: "fizz.buzz"
+			message_type: [{
+				name: "M"
+				field: [{name:"F1" number:1 label:LABEL_OPTIONAL type:TYPE_ENUM type_name:"some.other.enum" default_value:"UNKNOWN"}]
+			}]
+		`),
+		wantErr: `message field "fizz.buzz.M.F1" cannot resolve type: "*.some.other.enum" not found`,
+	}, {
+		label: "unresolved default enum value",
+		inDesc: mustParseFile(`
+			name: "test.proto"
+			package: "fizz.buzz"
+			message_type: [{
+				name: "M"
+				field: [{name:"F1" number:1 label:LABEL_OPTIONAL type:TYPE_ENUM type_name:"E" default_value:"UNKNOWN"}]
+				enum_type: [{name:"E" value:[{name:"V0" number:0}]}]
+			}]
+		`),
+		wantErr: `message field "fizz.buzz.M.F1" has invalid default: could not parse value for enum: "UNKNOWN"`,
+	}, {
+		label: "allowed unresolved default enum value",
+		inDesc: mustParseFile(`
+			name: "test.proto"
+			package: "fizz.buzz"
+			message_type: [{
+				name: "M"
+				field: [{name:"F1" number:1 label:LABEL_OPTIONAL type:TYPE_ENUM type_name:".fizz.buzz.M.E" default_value:"UNKNOWN"}]
+				enum_type: [{name:"E" value:[{name:"V0" number:0}]}]
+			}]
+		`),
+		inOpts: []option{allowUnresolvable()},
+	}, {
+		label: "unresolved extendee",
+		inDesc: mustParseFile(`
+			name: "test.proto"
+			package: "fizz.buzz"
+			extension: [{name:"X" number:1 label:LABEL_OPTIONAL extendee:"some.extended.message" type:TYPE_MESSAGE type_name:"some.other.message"}]
+		`),
+		wantErr: `extension field "fizz.buzz.X" cannot resolve extendee: "*.some.extended.message" not found`,
+	}, {
+		label: "unresolved method input",
+		inDesc: mustParseFile(`
+			name: "test.proto"
+			package: "fizz.buzz"
+			service: [{
+				name: "S"
+				method: [{name:"M" input_type:"foo.bar.input" output_type:".absolute.foo.bar.output"}]
+			}]
+		`),
+		wantErr: `service method "fizz.buzz.S.M" cannot resolve input: "*.foo.bar.input" not found`,
+	}, {
+		label: "allowed unresolved references",
+		inDesc: mustParseFile(`
+			name: "test.proto"
+			package: "fizz.buzz"
+			dependency: "remote.proto"
+			message_type: [{
+				name: "M"
+				field: [{name:"F1" number:1 label:LABEL_OPTIONAL type_name:"some.other.enum" default_value:"UNKNOWN"}]
+			}]
+			extension: [{name:"X" number:1 label:LABEL_OPTIONAL extendee:"some.extended.message" type:TYPE_MESSAGE type_name:"some.other.message"}]
+			service: [{
+				name: "S"
+				method: [{name:"M" input_type:"foo.bar.input" output_type:".absolute.foo.bar.output"}]
+			}]
+		`),
+		inOpts: []option{allowUnresolvable()},
+	}, {
+		label: "resolved but not imported",
+		inDeps: []*descriptorpb.FileDescriptorProto{mustParseFile(`
+			name: "dep.proto"
+			package: "fizz"
+			message_type: [{name:"M" nested_type:[{name:"M"}]}]
+		`)},
+		inDesc: mustParseFile(`
+			name: "test.proto"
+			package: "fizz.buzz"
+			message_type: [{
+				name: "M"
+				field: [{name:"F" number:1 label:LABEL_OPTIONAL type:TYPE_MESSAGE type_name:"M.M"}]
+			}]
+		`),
+		wantErr: `message field "fizz.buzz.M.F" cannot resolve type: resolved "fizz.M.M", but "dep.proto" is not imported`,
+	}, {
+		label: "resolved from remote import",
+		inDeps: []*descriptorpb.FileDescriptorProto{mustParseFile(`
+			name: "dep.proto"
+			package: "fizz"
+			message_type: [{name:"M" nested_type:[{name:"M"}]}]
+		`)},
+		inDesc: mustParseFile(`
+			name: "test.proto"
+			package: "fizz.buzz"
+			dependency: "dep.proto"
+			message_type: [{
+				name: "M"
+				field: [{name:"F" number:1 label:LABEL_OPTIONAL type:TYPE_MESSAGE type_name:"M.M"}]
+			}]
+		`),
+		wantDesc: mustParseFile(`
+			name: "test.proto"
+			package: "fizz.buzz"
+			dependency: "dep.proto"
+			message_type: [{
+				name: "M"
+				field: [{name:"F" number:1 label:LABEL_OPTIONAL type:TYPE_MESSAGE type_name:".fizz.M.M"}]
+			}]
+		`),
+	}}
+
+	for _, tt := range tests {
+		t.Run(tt.label, func(t *testing.T) {
+			r := new(protoregistry.Files)
+			for i, dep := range tt.inDeps {
+				f, err := newFile(dep, r)
+				if err != nil {
+					t.Fatalf("dependency %d: unexpected NewFile() error: %v", i, err)
+				}
+				if err := r.Register(f); err != nil {
+					t.Fatalf("dependency %d: unexpected Register() error: %v", i, err)
+				}
+			}
+			var gotDesc *descriptorpb.FileDescriptorProto
+			if tt.wantErr == "" && tt.wantDesc == nil {
+				tt.wantDesc = cloneFile(tt.inDesc)
+			}
+			gotFile, err := newFile(tt.inDesc, r, tt.inOpts...)
+			if gotFile != nil {
+				gotDesc = ToFileDescriptorProto(gotFile)
+			}
+			if !proto.Equal(gotDesc, tt.wantDesc) {
+				t.Errorf("NewFile() mismatch:\ngot  %v\nwant %v", gotDesc, tt.wantDesc)
+			}
+			if ((err == nil) != (tt.wantErr == "")) || !strings.Contains(fmt.Sprint(err), tt.wantErr) {
+				t.Errorf("NewFile() error:\ngot:  %v\nwant: %v", err, tt.wantErr)
+			}
+		})
+	}
+}

reflect/protodesc/proto.go

@@ -7,6 +7,7 @@ package protodesc
 import (
 	"fmt"
 	"reflect"
+	"strings"
 
 	"google.golang.org/protobuf/internal/encoding/defval"
 	"google.golang.org/protobuf/internal/scalar"
@@ -102,16 +103,18 @@ func ToFieldDescriptorProto(field protoreflect.FieldDescriptor) *descriptorpb.Fi
 		Name:    scalar.String(string(field.Name())),
 		Number:  scalar.Int32(int32(field.Number())),
 		Label:   descriptorpb.FieldDescriptorProto_Label(field.Cardinality()).Enum(),
-		Type:    descriptorpb.FieldDescriptorProto_Type(field.Kind()).Enum(),
 		Options: clone(field.Options()).(*descriptorpb.FieldOptions),
 	}
 	if field.IsExtension() {
 		p.Extendee = fullNameOf(field.ContainingMessage())
 	}
-	switch field.Kind() {
-	case protoreflect.EnumKind:
+	if field.Kind().IsValid() {
+		p.Type = descriptorpb.FieldDescriptorProto_Type(field.Kind()).Enum()
+	}
+	if field.Enum() != nil {
 		p.TypeName = fullNameOf(field.Enum())
-	case protoreflect.MessageKind, protoreflect.GroupKind:
+	}
+	if field.Message() != nil {
 		p.TypeName = fullNameOf(field.Message())
 	}
 	if field.HasJSONName() {
@@ -119,7 +122,9 @@ func ToFieldDescriptorProto(field protoreflect.FieldDescriptor) *descriptorpb.Fi
 	}
 	if field.HasDefault() {
 		def, err := defval.Marshal(field.Default(), field.DefaultEnumValue(), field.Kind(), defval.Descriptor)
-		if err != nil {
+		if err != nil && field.DefaultEnumValue() != nil {
+			def = string(field.DefaultEnumValue().Name()) // occurs for unresolved enum values
+		} else if err != nil {
 			panic(fmt.Sprintf("%v: %v", field.FullName(), err))
 		}
 		p.DefaultValue = scalar.String(def)
@@ -207,6 +212,9 @@ func fullNameOf(d protoreflect.Descriptor) *string {
 	if d == nil {
 		return nil
 	}
+	if strings.HasPrefix(string(d.FullName()), unknownPrefix) {
+		return scalar.String(string(d.FullName()[len(unknownPrefix):]))
+	}
 	return scalar.String("." + string(d.FullName()))
 }
 

reflect/protodesc/protodesc_test.go

@@ -8,21 +8,12 @@ import (
 	"strings"
 	"testing"
 
-	"google.golang.org/protobuf/encoding/prototext"
 	"google.golang.org/protobuf/internal/scalar"
 	"google.golang.org/protobuf/reflect/protoregistry"
 
 	"google.golang.org/protobuf/types/descriptorpb"
 )
 
-func mustParseFile(s string) *descriptorpb.FileDescriptorProto {
-	pb := new(descriptorpb.FileDescriptorProto)
-	if err := prototext.Unmarshal([]byte(s), pb); err != nil {
-		panic(err)
-	}
-	return pb
-}
-
 // Tests validation logic for malformed descriptors.
 func TestNewFile_ValidationErrors(t *testing.T) {
 	testCases := []struct {
@@ -144,7 +135,7 @@ func TestNewFile_ValidationErrors(t *testing.T) {
 				}},
 			}},
 		},
-		wantErr: "type_name",
+		wantErr: `message field "foo.BadMessage.bad_field" cannot resolve type: target name cannot be specified for int32`,
 	}, {
 		name: "type_name on string extension",
 		deps: []*descriptorpb.FileDescriptorProto{{
@@ -180,7 +171,7 @@ func TestNewFile_ValidationErrors(t *testing.T) {
 				TypeName: scalar.String("AnEnum"),
 			}},
 		},
-		wantErr: "type_name",
+		wantErr: `extension field "bar.my_ext" cannot resolve type: target name cannot be specified for string`,
 	}, {
 		name: "oneof_index on extension",
 		deps: []*descriptorpb.FileDescriptorProto{{
@@ -299,7 +290,7 @@ func TestNewFile_ValidationErrors(t *testing.T) {
 				}},
 			}},
 		},
-		wantErr: "foo.Foo without import of foo.proto",
+		wantErr: `message field "bar.Bar.foo" cannot resolve type: resolved "foo.Foo", but "foo.proto" is not imported`,
 	}, {
 		name: "enum dependency without import",
 		deps: []*descriptorpb.FileDescriptorProto{{
@@ -334,7 +325,7 @@ func TestNewFile_ValidationErrors(t *testing.T) {
 				}},
 			}},
 		},
-		wantErr: "foo.Foo without import of foo.proto",
+		wantErr: `message field "bar.Bar.foo" cannot resolve type: resolved "foo.Foo", but "foo.proto" is not imported`,
 	}, {
 		name: "message dependency on without import on file imported by a public import",
 		deps: []*descriptorpb.FileDescriptorProto{{
@@ -377,7 +368,7 @@ func TestNewFile_ValidationErrors(t *testing.T) {
 				}},
 			}},
 		},
-		wantErr: "foo.Foo without import of foo.proto",
+		wantErr: `message field "bar.Bar.foo" cannot resolve type: resolved "foo.Foo", but "foo.proto" is not imported`,
 	}}
 
 	for _, tc := range testCases {