Explorar el Código

reflect/protoreflect, reflect/protoregistry: move descriptor lookup to registry

Drop the protoreflect.FileDescriptor.DescriptorByName method.
Descriptor lookup will always happen through a protoregistry.Files, which
is more generally useful (it's rare that you want to find a descriptor in a
specific file, as opposed to a package which may be composed of multiple files).

Split protoregistry.Files descriptor lookup into individual per-type functions
(enum, message, extension, service), matching the preg.Types API.

Drop the ability to look up enum values, message fields, and service methods
for now. This can be easily added later if needed, and is trivial to implement
in user code. (e.g., look up the service and then consult sd.Methods.ByName().)

Change-Id: I2b3d8ef888921a8464ba1434eddab20c7d3a458e
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/172118
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
Damien Neil hace 6 años
padre
commit
2300c18725

+ 10 - 12
internal/fileinit/desc.go

@@ -246,7 +246,6 @@ type (
 	fileLazy struct {
 		syntax  pref.Syntax
 		imports fileImports
-		byName  map[pref.FullName]pref.Descriptor
 		options []byte
 	}
 )
@@ -260,17 +259,16 @@ func (fd *fileDesc) IsPlaceholder() bool             { return false }
 func (fd *fileDesc) Options() pref.ProtoMessage {
 	return unmarshalOptions(descopts.File, fd.lazyInit().options)
 }
-func (fd *fileDesc) Path() string                                     { return fd.path }
-func (fd *fileDesc) Package() pref.FullName                           { return fd.protoPackage }
-func (fd *fileDesc) Imports() pref.FileImports                        { return &fd.lazyInit().imports }
-func (fd *fileDesc) Enums() pref.EnumDescriptors                      { return &fd.enums }
-func (fd *fileDesc) Messages() pref.MessageDescriptors                { return &fd.messages }
-func (fd *fileDesc) Extensions() pref.ExtensionDescriptors            { return &fd.extensions }
-func (fd *fileDesc) Services() pref.ServiceDescriptors                { return &fd.services }
-func (fd *fileDesc) DescriptorByName(s pref.FullName) pref.Descriptor { return fd.lazyInit().byName[s] }
-func (fd *fileDesc) Format(s fmt.State, r rune)                       { pfmt.FormatDesc(s, r, fd) }
-func (fd *fileDesc) ProtoType(pref.FileDescriptor)                    {}
-func (fd *fileDesc) ProtoInternal(pragma.DoNotImplement)              {}
+func (fd *fileDesc) Path() string                          { return fd.path }
+func (fd *fileDesc) Package() pref.FullName                { return fd.protoPackage }
+func (fd *fileDesc) Imports() pref.FileImports             { return &fd.lazyInit().imports }
+func (fd *fileDesc) Enums() pref.EnumDescriptors           { return &fd.enums }
+func (fd *fileDesc) Messages() pref.MessageDescriptors     { return &fd.messages }
+func (fd *fileDesc) Extensions() pref.ExtensionDescriptors { return &fd.extensions }
+func (fd *fileDesc) Services() pref.ServiceDescriptors     { return &fd.services }
+func (fd *fileDesc) Format(s fmt.State, r rune)            { pfmt.FormatDesc(s, r, fd) }
+func (fd *fileDesc) ProtoType(pref.FileDescriptor)         {}
+func (fd *fileDesc) ProtoInternal(pragma.DoNotImplement)   {}
 
 type (
 	enumDesc struct {

+ 1 - 17
internal/fileinit/desc_lazy.go

@@ -333,7 +333,7 @@ func (fd *fileDesc) unmarshalFull(b []byte) {
 
 	var hasSyntax bool
 	var enumIdx, messageIdx, extensionIdx, serviceIdx int
-	fd.lazy = &fileLazy{byName: make(map[pref.FullName]pref.Descriptor)}
+	fd.lazy = &fileLazy{}
 	for len(b) > 0 {
 		num, typ, n := wire.ConsumeTag(b)
 		b = b[n:]
@@ -424,8 +424,6 @@ func (ed *enumDesc) unmarshalFull(b []byte, nb *nameBuilder) {
 			ed.lazy.values.list[i].unmarshalFull(b, nb, ed.parentFile, ed, i)
 		}
 	}
-
-	ed.parentFile.lazy.byName[ed.FullName()] = ed
 }
 
 func unmarshalEnumReservedRange(b []byte) (r [2]pref.EnumNumber) {
@@ -480,8 +478,6 @@ func (vd *enumValueDesc) unmarshalFull(b []byte, nb *nameBuilder, pf *fileDesc,
 			b = b[m:]
 		}
 	}
-
-	vd.parentFile.lazy.byName[vd.FullName()] = vd
 }
 
 func (md *messageDesc) unmarshalFull(b []byte, nb *nameBuilder) {
@@ -546,8 +542,6 @@ func (md *messageDesc) unmarshalFull(b []byte, nb *nameBuilder) {
 	if isMapEntry != md.isMapEntry {
 		panic("mismatching map entry property")
 	}
-
-	md.parentFile.lazy.byName[md.FullName()] = md.asDesc()
 }
 
 func (md *messageDesc) unmarshalOptions(b []byte, isMapEntry *bool) {
@@ -694,8 +688,6 @@ func (fd *fieldDesc) unmarshalFull(b []byte, nb *nameBuilder, pf *fileDesc, pd p
 		}
 		fd.messageType = ptype.PlaceholderMessage(pref.FullName(rawTypeName[1:]))
 	}
-
-	fd.parentFile.lazy.byName[fd.FullName()] = fd
 }
 
 func (fd *fieldDesc) unmarshalOptions(b []byte) {
@@ -744,8 +736,6 @@ func (od *oneofDesc) unmarshalFull(b []byte, nb *nameBuilder, pf *fileDesc, pd p
 			b = b[m:]
 		}
 	}
-
-	od.parentFile.lazy.byName[od.FullName()] = od
 }
 
 func (xd *extensionDesc) unmarshalFull(b []byte, nb *nameBuilder) {
@@ -790,8 +780,6 @@ func (xd *extensionDesc) unmarshalFull(b []byte, nb *nameBuilder) {
 			panic(err)
 		}
 	}
-
-	xd.parentFile.lazy.byName[xd.FullName()] = xd
 }
 
 func (xd *extensionDesc) unmarshalOptions(b []byte) {
@@ -842,8 +830,6 @@ func (sd *serviceDesc) unmarshalFull(b []byte, nb *nameBuilder) {
 			sd.lazy.methods.list[i].unmarshalFull(b, nb, sd.parentFile, sd, i)
 		}
 	}
-
-	sd.parentFile.lazy.byName[sd.FullName()] = sd
 }
 
 func (md *methodDesc) unmarshalFull(b []byte, nb *nameBuilder, pf *fileDesc, pd pref.Descriptor, i int) {
@@ -878,6 +864,4 @@ func (md *methodDesc) unmarshalFull(b []byte, nb *nameBuilder, pf *fileDesc, pd
 			b = b[m:]
 		}
 	}
-
-	md.parentFile.lazy.byName[md.FullName()] = md
 }

+ 3 - 3
internal/fileinit/fileinit_test.go

@@ -70,9 +70,9 @@ func TestInit(t *testing.T) {
 
 	// Verify that message descriptors for map entries have no Go type info.
 	mapEntryName := protoreflect.FullName("goproto.proto.test.TestAllTypes.MapInt32Int32Entry")
-	d := testpb.File_test_test_proto.DescriptorByName(mapEntryName)
-	if _, ok := d.(protoreflect.MessageDescriptor); !ok {
-		t.Errorf("message descriptor for %v not found", mapEntryName)
+	d := testpb.File_test_test_proto.Messages().ByName("TestAllTypes").Fields().ByName("map_int32_int32").MessageType()
+	if gotName, wantName := d.FullName(), mapEntryName; gotName != wantName {
+		t.Fatalf("looked up wrong descriptor: got %v, want %v", gotName, wantName)
 	}
 	if _, ok := d.(protoreflect.MessageType); ok {
 		t.Errorf("message descriptor for %v must not implement protoreflect.MessageType", mapEntryName)

+ 10 - 11
internal/prototype/placeholder_type.go

@@ -43,17 +43,16 @@ type placeholderFile struct {
 	placeholderName
 }
 
-func (t placeholderFile) Options() pref.ProtoMessage                     { return descopts.File }
-func (t placeholderFile) Path() string                                   { return t.path }
-func (t placeholderFile) Package() pref.FullName                         { return t.FullName() }
-func (t placeholderFile) Imports() pref.FileImports                      { return &emptyFiles }
-func (t placeholderFile) Enums() pref.EnumDescriptors                    { return &emptyEnums }
-func (t placeholderFile) Messages() pref.MessageDescriptors              { return &emptyMessages }
-func (t placeholderFile) Extensions() pref.ExtensionDescriptors          { return &emptyExtensions }
-func (t placeholderFile) Services() pref.ServiceDescriptors              { return &emptyServices }
-func (t placeholderFile) DescriptorByName(pref.FullName) pref.Descriptor { return nil }
-func (t placeholderFile) Format(s fmt.State, r rune)                     { pfmt.FormatDesc(s, r, t) }
-func (t placeholderFile) ProtoType(pref.FileDescriptor)                  {}
+func (t placeholderFile) Options() pref.ProtoMessage            { return descopts.File }
+func (t placeholderFile) Path() string                          { return t.path }
+func (t placeholderFile) Package() pref.FullName                { return t.FullName() }
+func (t placeholderFile) Imports() pref.FileImports             { return &emptyFiles }
+func (t placeholderFile) Enums() pref.EnumDescriptors           { return &emptyEnums }
+func (t placeholderFile) Messages() pref.MessageDescriptors     { return &emptyMessages }
+func (t placeholderFile) Extensions() pref.ExtensionDescriptors { return &emptyExtensions }
+func (t placeholderFile) Services() pref.ServiceDescriptors     { return &emptyServices }
+func (t placeholderFile) Format(s fmt.State, r rune)            { pfmt.FormatDesc(s, r, t) }
+func (t placeholderFile) ProtoType(pref.FileDescriptor)         {}
 
 type placeholderMessage struct {
 	placeholderName

+ 17 - 94
internal/prototype/protofile_type.go

@@ -44,7 +44,6 @@ type fileMeta struct {
 	es enumsMeta
 	xs extensionsMeta
 	ss servicesMeta
-	ds descriptorsMeta
 }
 type fileDesc struct{ f *File }
 
@@ -63,99 +62,23 @@ func newFile(f *File) fileDesc {
 	f.fileMeta = new(fileMeta)
 	return fileDesc{f}
 }
-func (t fileDesc) Parent() (pref.Descriptor, bool)                  { return nil, false }
-func (t fileDesc) Index() int                                       { return 0 }
-func (t fileDesc) Syntax() pref.Syntax                              { return t.f.Syntax }
-func (t fileDesc) Name() pref.Name                                  { return t.f.Package.Name() }
-func (t fileDesc) FullName() pref.FullName                          { return t.f.Package }
-func (t fileDesc) IsPlaceholder() bool                              { return false }
-func (t fileDesc) Options() pref.ProtoMessage                       { return altOptions(t.f.Options, descopts.File) }
-func (t fileDesc) Path() string                                     { return t.f.Path }
-func (t fileDesc) Package() pref.FullName                           { return t.f.Package }
-func (t fileDesc) Imports() pref.FileImports                        { return (*fileImports)(&t.f.Imports) }
-func (t fileDesc) Enums() pref.EnumDescriptors                      { return t.f.es.lazyInit(t, t.f.Enums) }
-func (t fileDesc) Messages() pref.MessageDescriptors                { return t.f.ms.lazyInit(t, t.f.Messages) }
-func (t fileDesc) Extensions() pref.ExtensionDescriptors            { return t.f.xs.lazyInit(t, t.f.Extensions) }
-func (t fileDesc) Services() pref.ServiceDescriptors                { return t.f.ss.lazyInit(t, t.f.Services) }
-func (t fileDesc) DescriptorByName(s pref.FullName) pref.Descriptor { return t.f.ds.lookup(t, s) }
-func (t fileDesc) Format(s fmt.State, r rune)                       { pfmt.FormatDesc(s, r, t) }
-func (t fileDesc) ProtoType(pref.FileDescriptor)                    {}
-func (t fileDesc) ProtoInternal(pragma.DoNotImplement)              {}
-
-// descriptorsMeta is a lazily initialized map of all descriptors declared in
-// the file by full name.
-type descriptorsMeta struct {
-	once sync.Once
-	m    map[pref.FullName]pref.Descriptor
-}
-
-func (m *descriptorsMeta) lookup(fd pref.FileDescriptor, s pref.FullName) pref.Descriptor {
-	m.once.Do(func() {
-		m.m = make(map[pref.FullName]pref.Descriptor)
-		m.initMap(fd)
-		delete(m.m, fd.Package()) // avoid registering the file descriptor itself
-	})
-	return m.m[s]
-}
-func (m *descriptorsMeta) initMap(d pref.Descriptor) {
-	m.m[d.FullName()] = d
-	if ds, ok := d.(interface {
-		Enums() pref.EnumDescriptors
-	}); ok {
-		for i := 0; i < ds.Enums().Len(); i++ {
-			m.initMap(ds.Enums().Get(i))
-		}
-	}
-	if ds, ok := d.(interface {
-		Values() pref.EnumValueDescriptors
-	}); ok {
-		for i := 0; i < ds.Values().Len(); i++ {
-			m.initMap(ds.Values().Get(i))
-		}
-	}
-	if ds, ok := d.(interface {
-		Messages() pref.MessageDescriptors
-	}); ok {
-		for i := 0; i < ds.Messages().Len(); i++ {
-			m.initMap(ds.Messages().Get(i))
-		}
-	}
-	if ds, ok := d.(interface {
-		Fields() pref.FieldDescriptors
-	}); ok {
-		for i := 0; i < ds.Fields().Len(); i++ {
-			m.initMap(ds.Fields().Get(i))
-		}
-	}
-	if ds, ok := d.(interface {
-		Oneofs() pref.OneofDescriptors
-	}); ok {
-		for i := 0; i < ds.Oneofs().Len(); i++ {
-			m.initMap(ds.Oneofs().Get(i))
-		}
-	}
-	if ds, ok := d.(interface {
-		Extensions() pref.ExtensionDescriptors
-	}); ok {
-		for i := 0; i < ds.Extensions().Len(); i++ {
-			m.initMap(ds.Extensions().Get(i))
-		}
-	}
-	if ds, ok := d.(interface {
-		Services() pref.ServiceDescriptors
-	}); ok {
-		for i := 0; i < ds.Services().Len(); i++ {
-			m.initMap(ds.Services().Get(i))
-		}
-	}
-	if ds, ok := d.(interface {
-		Methods() pref.MethodDescriptors
-	}); ok {
-		for i := 0; i < ds.Methods().Len(); i++ {
-			m.initMap(ds.Methods().Get(i))
-		}
-	}
-}
+func (t fileDesc) Parent() (pref.Descriptor, bool)       { return nil, false }
+func (t fileDesc) Index() int                            { return 0 }
+func (t fileDesc) Syntax() pref.Syntax                   { return t.f.Syntax }
+func (t fileDesc) Name() pref.Name                       { return t.f.Package.Name() }
+func (t fileDesc) FullName() pref.FullName               { return t.f.Package }
+func (t fileDesc) IsPlaceholder() bool                   { return false }
+func (t fileDesc) Options() pref.ProtoMessage            { return altOptions(t.f.Options, descopts.File) }
+func (t fileDesc) Path() string                          { return t.f.Path }
+func (t fileDesc) Package() pref.FullName                { return t.f.Package }
+func (t fileDesc) Imports() pref.FileImports             { return (*fileImports)(&t.f.Imports) }
+func (t fileDesc) Enums() pref.EnumDescriptors           { return t.f.es.lazyInit(t, t.f.Enums) }
+func (t fileDesc) Messages() pref.MessageDescriptors     { return t.f.ms.lazyInit(t, t.f.Messages) }
+func (t fileDesc) Extensions() pref.ExtensionDescriptors { return t.f.xs.lazyInit(t, t.f.Extensions) }
+func (t fileDesc) Services() pref.ServiceDescriptors     { return t.f.ss.lazyInit(t, t.f.Services) }
+func (t fileDesc) Format(s fmt.State, r rune)            { pfmt.FormatDesc(s, r, t) }
+func (t fileDesc) ProtoType(pref.FileDescriptor)         {}
+func (t fileDesc) ProtoInternal(pragma.DoNotImplement)   {}
 
 type messageMeta struct {
 	inheritedMeta

+ 0 - 27
internal/prototype/type_test.go

@@ -634,33 +634,6 @@ func testFileAccessors(t *testing.T, fd pref.FileDescriptor) {
 				},
 			},
 		},
-		"DescriptorByName:":                 nil,
-		"DescriptorByName:A":                nil,
-		"DescriptorByName:test":             nil,
-		"DescriptorByName:test.":            nil,
-		"DescriptorByName:test.A":           M{"FullName": pref.FullName("test.A")},
-		"DescriptorByName:test.A.key":       M{"FullName": pref.FullName("test.A.key")},
-		"DescriptorByName:test.A.A":         nil,
-		"DescriptorByName:test.A.field_one": nil,
-		"DescriptorByName:test.B.field_one": M{"FullName": pref.FullName("test.B.field_one")},
-		"DescriptorByName:test.B.O1":        M{"FullName": pref.FullName("test.B.O1")},
-		"DescriptorByName:test.B.O3":        nil,
-		"DescriptorByName:test.C.E1":        M{"FullName": pref.FullName("test.C.E1")},
-		"DescriptorByName:test.C.E1.FOO":    nil,
-		"DescriptorByName:test.C.FOO":       M{"FullName": pref.FullName("test.C.FOO")},
-		"DescriptorByName:test.C.Foo":       nil,
-		"DescriptorByName:test.C.BAZ":       nil,
-		"DescriptorByName:test.E1":          M{"FullName": pref.FullName("test.E1")},
-		"DescriptorByName:test.E1.FOO":      nil,
-		"DescriptorByName:test.FOO":         M{"FullName": pref.FullName("test.FOO")},
-		"DescriptorByName:test.Foo":         nil,
-		"DescriptorByName:test.BAZ":         nil,
-		"DescriptorByName:test.C.X":         M{"FullName": pref.FullName("test.C.X")},
-		"DescriptorByName:test.X":           M{"FullName": pref.FullName("test.X")},
-		"DescriptorByName:test.X.":          nil,
-		"DescriptorByName:test.S":           M{"FullName": pref.FullName("test.S")},
-		"DescriptorByName:test.S.M":         M{"FullName": pref.FullName("test.S.M")},
-		"DescriptorByName:test.M":           nil,
 	}
 	checkAccessors(t, "", reflect.ValueOf(fd), want)
 }

+ 6 - 20
reflect/protodesc/protodesc.go

@@ -311,18 +311,11 @@ func findMessageDescriptor(s string, r *protoregistry.Files) (protoreflect.Messa
 		return nil, errors.New("identifier name must be fully qualified with a leading dot: %v", s)
 	}
 	name := protoreflect.FullName(strings.TrimPrefix(s, "."))
-	switch m, err := r.FindDescriptorByName(name); {
-	case err == nil:
-		m, ok := m.(protoreflect.MessageDescriptor)
-		if !ok {
-			return nil, errors.New("resolved wrong type: got %v, want message", typeName(m))
-		}
-		return m, nil
-	case err == protoregistry.NotFound:
+	md, err := r.FindMessageByName(name)
+	if err != nil {
 		return prototype.PlaceholderMessage(name), nil
-	default:
-		return nil, err
 	}
+	return md, nil
 }
 
 func findEnumDescriptor(s string, r *protoregistry.Files) (protoreflect.EnumDescriptor, error) {
@@ -330,18 +323,11 @@ func findEnumDescriptor(s string, r *protoregistry.Files) (protoreflect.EnumDesc
 		return nil, errors.New("identifier name must be fully qualified with a leading dot: %v", s)
 	}
 	name := protoreflect.FullName(strings.TrimPrefix(s, "."))
-	switch e, err := r.FindDescriptorByName(name); {
-	case err == nil:
-		e, ok := e.(protoreflect.EnumDescriptor)
-		if !ok {
-			return nil, errors.New("resolved wrong type: got %T, want enum", typeName(e))
-		}
-		return e, nil
-	case err == protoregistry.NotFound:
+	ed, err := r.FindEnumByName(name)
+	if err != nil {
 		return prototype.PlaceholderEnum(name), nil
-	default:
-		return nil, err
 	}
+	return ed, nil
 }
 
 func typeName(t protoreflect.Descriptor) string {

+ 0 - 3
reflect/protoreflect/type.go

@@ -129,9 +129,6 @@ type FileDescriptor interface {
 	Extensions() ExtensionDescriptors
 	// Services is a list of the top-level service declarations.
 	Services() ServiceDescriptors
-	// DescriptorByName looks up any descriptor declared within this file
-	// by full name. It returns nil if not found.
-	DescriptorByName(FullName) Descriptor
 
 	isFileDescriptor
 }

+ 141 - 175
reflect/protoregistry/registry.go

@@ -18,7 +18,6 @@ package protoregistry
 import (
 	"fmt"
 	"reflect"
-	"sort"
 	"strings"
 
 	"github.com/golang/protobuf/v2/internal/errors"
@@ -44,25 +43,22 @@ var NotFound = errors.New("not found")
 // descriptors contained within them.
 // The Find and Range methods are safe for concurrent use.
 type Files struct {
-	filesByPackage filesByPackage
-	filesByPath    filesByPath
-}
-
-type (
-	filesByPackage struct {
-		// files is a list of files all in the same package.
-		files []protoreflect.FileDescriptor
-		// subs is a tree of files all in a sub-package scope.
-		// It also maps all top-level identifiers declared in files
-		// as the notProtoPackage sentinel value.
-		subs map[protoreflect.Name]*filesByPackage // invariant: len(Name) > 0
-	}
+	// The map of descs contains:
+	//	EnumDescriptor
+	//	MessageDescriptor
+	//	ExtensionDescriptor
+	//	ServiceDescriptor
+	//	*packageDescriptor
+	//
+	// Note that files are stored as a slice, since a package may contain
+	// multiple files.
+	descs       map[protoreflect.FullName]interface{}
 	filesByPath map[string][]protoreflect.FileDescriptor
-)
+}
 
-// notProtoPackage is a sentinel value to indicate that some identifier maps
-// to an actual protobuf declaration and is not a sub-package.
-var notProtoPackage = new(filesByPackage)
+type packageDescriptor struct {
+	files []protoreflect.FileDescriptor
+}
 
 // NewFiles returns a registry initialized with the provided set of files.
 // If there are duplicates, the first one takes precedence.
@@ -88,165 +84,124 @@ func NewFiles(files ...protoreflect.FileDescriptor) *Files {
 //
 // It is permitted for multiple files to have the same file path.
 func (r *Files) Register(files ...protoreflect.FileDescriptor) error {
+	if r.descs == nil {
+		r.descs = map[protoreflect.FullName]interface{}{
+			"": &packageDescriptor{},
+		}
+		r.filesByPath = make(map[string][]protoreflect.FileDescriptor)
+	}
 	var firstErr error
-fileLoop:
 	for _, file := range files {
-		if file.IsPlaceholder() {
-			continue // TODO: Should this be an error instead?
+		if err := r.registerFile(file); err != nil && firstErr == nil {
+			firstErr = err
 		}
-
-		// Register the file into the filesByPackage tree.
-		//
-		// The prototype package validates that a FileDescriptor is internally
-		// consistent such it does not have conflicts within itself.
-		// However, we need to ensure that the inserted file does not conflict
-		// with other previously inserted files.
-		pkg := file.Package()
-		root := &r.filesByPackage
-		for len(pkg) > 0 {
-			var prefix protoreflect.Name
-			prefix, pkg = splitPrefix(pkg)
-
-			// Add a new sub-package segment.
-			switch nextRoot := root.subs[prefix]; nextRoot {
-			case nil:
-				nextRoot = new(filesByPackage)
-				if root.subs == nil {
-					root.subs = make(map[protoreflect.Name]*filesByPackage)
-				}
-				root.subs[prefix] = nextRoot
-				root = nextRoot
-			case notProtoPackage:
-				if firstErr == nil {
-					name := strings.TrimSuffix(strings.TrimSuffix(string(file.Package()), string(pkg)), ".")
-					firstErr = errors.New("file %q has a name conflict over %v", file.Path(), name)
-				}
-				continue fileLoop
-			default:
-				root = nextRoot
-			}
+	}
+	return firstErr
+}
+func (r *Files) registerFile(file protoreflect.FileDescriptor) error {
+	for name := file.Package(); name != ""; name = name.Parent() {
+		switch r.descs[name].(type) {
+		case nil, *packageDescriptor:
+		default:
+			return errors.New("file %q has a name conflict over %v", file.Path(), name)
+		}
+	}
+	var err error
+	rangeRegisteredDescriptors(file, func(desc protoreflect.Descriptor) {
+		if r.descs[desc.FullName()] != nil {
+			err = errors.New("file %q has a name conflict over %v", file.Path(), desc.FullName())
 		}
+	})
+	if err != nil {
+		return err
+	}
 
-		// Check for top-level conflicts within the same package.
-		// The current file cannot add any top-level declaration that conflict
-		// with another top-level declaration or sub-package name.
-		var conflicts []protoreflect.Name
-		rangeTopLevelDeclarations(file, func(s protoreflect.Name) {
-			if root.subs[s] == nil {
-				if root.subs == nil {
-					root.subs = make(map[protoreflect.Name]*filesByPackage)
-				}
-				root.subs[s] = notProtoPackage
-			} else {
-				conflicts = append(conflicts, s)
-			}
-		})
-		if len(conflicts) > 0 {
-			// Remove inserted identifiers to make registration failure atomic.
-			sort.Slice(conflicts, func(i, j int) bool { return conflicts[i] < conflicts[j] })
-			rangeTopLevelDeclarations(file, func(s protoreflect.Name) {
-				i := sort.Search(len(conflicts), func(i int) bool { return conflicts[i] >= s })
-				if has := i < len(conflicts) && conflicts[i] == s; !has {
-					delete(root.subs, s) // remove everything not in conflicts
-				}
-			})
+	path := file.Path()
+	r.filesByPath[path] = append(r.filesByPath[path], file)
 
-			if firstErr == nil {
-				name := file.Package().Append(conflicts[0])
-				firstErr = errors.New("file %q has a name conflict over %v", file.Path(), name)
-			}
-			continue fileLoop
+	for name := file.Package(); name != ""; name = name.Parent() {
+		if r.descs[name] == nil {
+			r.descs[name] = &packageDescriptor{}
 		}
-		root.files = append(root.files, file)
-
-		// Register the file into the filesByPath map.
-		//
-		// There is no check for conflicts in file path since the path is
-		// heavily dependent on how protoc is invoked. When protoc is being
-		// invoked by different parties in a distributed manner, it is
-		// unreasonable to assume nor ensure that the path is unique.
-		if r.filesByPath == nil {
-			r.filesByPath = make(filesByPath)
-		}
-		r.filesByPath[file.Path()] = append(r.filesByPath[file.Path()], file)
 	}
-	return firstErr
+	p := r.descs[file.Package()].(*packageDescriptor)
+	p.files = append(p.files, file)
+	rangeRegisteredDescriptors(file, func(desc protoreflect.Descriptor) {
+		r.descs[desc.FullName()] = desc
+	})
+	return nil
 }
 
-// FindDescriptorByName looks up any descriptor (except files) by its full name.
-// Files are not handled since multiple file descriptors may belong in
-// the same package and have the same full name (see RangeFilesByPackage).
+// FindEnumByName looks up an enum by the enum's full name.
 //
-// This return (nil, NotFound) if not found.
-func (r *Files) FindDescriptorByName(name protoreflect.FullName) (protoreflect.Descriptor, error) {
+// This returns (nil, NotFound) if not found.
+func (r *Files) FindEnumByName(name protoreflect.FullName) (protoreflect.EnumDescriptor, error) {
 	if r == nil {
 		return nil, NotFound
 	}
-	pkg := name
-	root := &r.filesByPackage
-	for len(pkg) > 0 {
-		var prefix protoreflect.Name
-		prefix, pkg = splitPrefix(pkg)
-		switch nextRoot := root.subs[prefix]; nextRoot {
-		case nil:
-			return nil, NotFound
-		case notProtoPackage:
-			// Search current root's package for the descriptor.
-			for _, fd := range root.files {
-				if d := fd.DescriptorByName(name); d != nil {
-					return d, nil
-				}
-			}
-			return nil, NotFound
-		default:
-			root = nextRoot
-		}
+	if d, ok := r.descs[name].(protoreflect.EnumDescriptor); ok {
+		return d, nil
 	}
 	return nil, NotFound
 }
 
-// RangeFiles iterates over all registered files.
-// The iteration order is undefined.
-func (r *Files) RangeFiles(f func(protoreflect.FileDescriptor) bool) {
-	r.RangeFilesByPackage("", f) // empty package is a prefix for all packages
+// FindMessageByName looks up a message by the message's full name.
+//
+// This returns (nil, NotFound) if not found.
+func (r *Files) FindMessageByName(name protoreflect.FullName) (protoreflect.MessageDescriptor, error) {
+	if r == nil {
+		return nil, NotFound
+	}
+	if d, ok := r.descs[name].(protoreflect.MessageDescriptor); ok {
+		return d, nil
+	}
+	return nil, NotFound
 }
 
-// RangeFilesByPackage iterates over all registered files filtered by
-// the given proto package prefix. It iterates over files with an exact package
-// match before iterating over files with general prefix match.
-// The iteration order is undefined within exact matches or prefix matches.
-func (r *Files) RangeFilesByPackage(pkg protoreflect.FullName, f func(protoreflect.FileDescriptor) bool) {
+// FindExtensionByName looks up an extension field by the field's full name.
+// Note that this is the full name of the field as determined by
+// where the extension is declared and is unrelated to the full name of the
+// message being extended.
+//
+// This returns (nil, NotFound) if not found.
+func (r *Files) FindExtensionByName(name protoreflect.FullName) (protoreflect.ExtensionDescriptor, error) {
 	if r == nil {
-		return
+		return nil, NotFound
 	}
-	if strings.HasSuffix(string(pkg), ".") {
-		return // avoid edge case where splitPrefix allows trailing dot
+	if d, ok := r.descs[name].(protoreflect.ExtensionDescriptor); ok {
+		return d, nil
 	}
-	root := &r.filesByPackage
-	for len(pkg) > 0 && root != nil {
-		var prefix protoreflect.Name
-		prefix, pkg = splitPrefix(pkg)
-		root = root.subs[prefix]
+	return nil, NotFound
+}
+
+// FindServiceByName looks up a service by the service's full name.
+//
+// This returns (nil, NotFound) if not found.
+func (r *Files) FindServiceByName(name protoreflect.FullName) (protoreflect.ServiceDescriptor, error) {
+	if r == nil {
+		return nil, NotFound
+	}
+	if d, ok := r.descs[name].(protoreflect.ServiceDescriptor); ok {
+		return d, nil
 	}
-	rangeFiles(root, f)
+	return nil, NotFound
 }
-func rangeFiles(fs *filesByPackage, f func(protoreflect.FileDescriptor) bool) bool {
-	if fs == nil {
-		return true
-	}
-	// Iterate over exact matches.
-	for _, fd := range fs.files { // TODO: iterate non-deterministically
-		if !f(fd) {
-			return false
-		}
+
+// RangeFiles iterates over all registered files.
+// The iteration order is undefined.
+func (r *Files) RangeFiles(f func(protoreflect.FileDescriptor) bool) {
+	if r == nil {
+		return
 	}
-	// Iterate over prefix matches.
-	for _, fs := range fs.subs {
-		if !rangeFiles(fs, f) {
-			return false
+	for _, d := range r.descs {
+		if p, ok := d.(*packageDescriptor); ok {
+			for _, file := range p.files {
+				if !f(file) {
+					return
+				}
+			}
 		}
 	}
-	return true
 }
 
 // RangeFilesByPath iterates over all registered files filtered by
@@ -255,46 +210,57 @@ func (r *Files) RangeFilesByPath(path string, f func(protoreflect.FileDescriptor
 	if r == nil {
 		return
 	}
-	for _, fd := range r.filesByPath[path] { // TODO: iterate non-deterministically
-		if !f(fd) {
+	for _, file := range r.filesByPath[path] {
+		if !f(file) {
 			return
 		}
 	}
 }
 
-func splitPrefix(name protoreflect.FullName) (protoreflect.Name, protoreflect.FullName) {
-	if i := strings.IndexByte(string(name), '.'); i >= 0 {
-		return protoreflect.Name(name[:i]), name[i+len("."):]
+// RangeFilesByPackage iterates over all registered files in a give proto package.
+// The iteration order is undefined.
+func (r *Files) RangeFilesByPackage(name protoreflect.FullName, f func(protoreflect.FileDescriptor) bool) {
+	if r == nil {
+		return
+	}
+	p, ok := r.descs[name].(*packageDescriptor)
+	if !ok {
+		return
+	}
+	for _, file := range p.files {
+		if !f(file) {
+			return
+		}
 	}
-	return protoreflect.Name(name), ""
 }
 
-// rangeTopLevelDeclarations iterates over the name of all top-level
-// declarations in the proto file.
-func rangeTopLevelDeclarations(fd protoreflect.FileDescriptor, f func(protoreflect.Name)) {
+// rangeRegisteredDescriptors iterates over all descriptors in a file which
+// will be entered into the registry: enums, messages, extensions, and services.
+func rangeRegisteredDescriptors(fd protoreflect.FileDescriptor, f func(protoreflect.Descriptor)) {
+	rangeRegisteredMessageDescriptors(fd.Messages(), f)
 	for i := 0; i < fd.Enums().Len(); i++ {
 		e := fd.Enums().Get(i)
-		f(e.Name())
-
-		// TODO: Drop ranging over top-level enum values. The current
-		// implementation of fileinit.FileBuilder does not initialize the names
-		// for enum values in enums. Doing so reduces init time considerably.
-		// If we drop this, it means that conflict checks in the registry
-		// is not complete. However, this may be okay since the most common
-		// reason for a conflict is due to vendored proto files, which are
-		// most certainly going to have a name conflict on the parent enum.
-		for i := 0; i < e.Values().Len(); i++ {
-			f(e.Values().Get(i).Name())
-		}
-	}
-	for i := 0; i < fd.Messages().Len(); i++ {
-		f(fd.Messages().Get(i).Name())
+		f(e)
 	}
 	for i := 0; i < fd.Extensions().Len(); i++ {
-		f(fd.Extensions().Get(i).Name())
+		f(fd.Extensions().Get(i))
 	}
 	for i := 0; i < fd.Services().Len(); i++ {
-		f(fd.Services().Get(i).Name())
+		f(fd.Services().Get(i))
+	}
+}
+func rangeRegisteredMessageDescriptors(messages protoreflect.MessageDescriptors, f func(protoreflect.Descriptor)) {
+	for i := 0; i < messages.Len(); i++ {
+		md := messages.Get(i)
+		f(md)
+		rangeRegisteredMessageDescriptors(md.Messages(), f)
+		for i := 0; i < md.Enums().Len(); i++ {
+			e := md.Enums().Get(i)
+			f(e)
+		}
+		for i := 0; i < md.Extensions().Len(); i++ {
+			f(md.Extensions().Get(i))
+		}
 	}
 }
 

+ 65 - 80
reflect/protoregistry/registry_test.go

@@ -16,6 +16,7 @@ import (
 	pref "github.com/golang/protobuf/v2/reflect/protoreflect"
 	preg "github.com/golang/protobuf/v2/reflect/protoregistry"
 
+	test2pb "github.com/golang/protobuf/v2/internal/testprotos/test"
 	testpb "github.com/golang/protobuf/v2/reflect/protoregistry/testprotos"
 )
 
@@ -29,10 +30,6 @@ func TestFiles(t *testing.T) {
 			inFile  *ptype.File
 			wantErr string
 		}
-		testFindDesc struct {
-			inName pref.FullName
-			wantOk bool
-		}
 		testRangePkg struct {
 			inPkg     pref.FullName
 			wantFiles []file
@@ -45,7 +42,6 @@ func TestFiles(t *testing.T) {
 
 	tests := []struct {
 		files      []testFile
-		findDescs  []testFindDesc
 		rangePkgs  []testRangePkg
 		rangePaths []testRangePath
 	}{{
@@ -63,27 +59,20 @@ func TestFiles(t *testing.T) {
 			inPkg: "nothing",
 		}, {
 			inPkg: "",
-			wantFiles: []file{
-				{"", "foo.bar"},
-				{"", "foo.bar"},
-				{"foo/bar/test.proto", "foo.bar.baz"},
-				{"foo/bar/test.proto", "my.test"},
-				{"", "my.test.package"},
-				{"foo/bar/baz/../test.proto", "my.test"},
-			},
 		}, {
 			inPkg: ".",
 		}, {
 			inPkg: "foo",
-			wantFiles: []file{
-				{"", "foo.bar"},
-				{"", "foo.bar"},
-				{"foo/bar/test.proto", "foo.bar.baz"},
-			},
 		}, {
 			inPkg: "foo.",
 		}, {
 			inPkg: "foo..",
+		}, {
+			inPkg: "foo.bar",
+			wantFiles: []file{
+				{"", "foo.bar"},
+				{"", "foo.bar"},
+			},
 		}, {
 			inPkg: "foo.bar.baz",
 			wantFiles: []file{
@@ -181,23 +170,6 @@ func TestFiles(t *testing.T) {
 					Values: []ptype.EnumValue{{Name: "EnumValue", Number: 0}},
 				}},
 			},
-		}, {
-			// Conflict over a single declaration.
-			inFile: &ptype.File{
-				Syntax:  pref.Proto2,
-				Package: "fizz.buzz",
-				Enums: []ptype.Enum{{
-					Name:   "Enum1",
-					Values: []ptype.EnumValue{{Name: "EnumValue1", Number: 0}},
-				}, {
-					Name:   "Enum2",
-					Values: []ptype.EnumValue{{Name: "EnumValue2", Number: 0}},
-				}, {
-					Name:   "Enum3",
-					Values: []ptype.EnumValue{{Name: "Enum", Number: 0}}, // conflict
-				}},
-			},
-			wantErr: "name conflict over fizz.buzz.Enum",
 		}, {
 			// Previously failed registration should not pollute the namespace.
 			inFile: &ptype.File{
@@ -226,28 +198,6 @@ func TestFiles(t *testing.T) {
 				}},
 			},
 		}},
-
-		findDescs: []testFindDesc{
-			{"", false},
-			{"Enum", false},
-			{"Message", true},
-			{"Message.", false},
-			{"Message.Message", true},
-			{"Message.Message.Message", true},
-			{"Message.Message.Message.Message", false},
-			{"fizz.buzz", false},
-			{"fizz.buzz.Enum", true},
-			{"fizz.buzz.Enum1", true},
-			{"fizz.buzz.Enum1.EnumValue", false},
-			{"fizz.buzz.EnumValue", true},
-			{"fizz.buzz.Message", true},
-			{"fizz.buzz.Message.Field", true},
-			{"fizz.buzz.Message.Oneof", true},
-			{"fizz.buzz.Extension", true},
-			{"fizz.buzz.Service", true},
-			{"fizz.buzz.Service.Method", true},
-			{"fizz.buzz.Method", false},
-		},
 	}}
 
 	sortFiles := cmpopts.SortSlices(func(x, y file) bool {
@@ -267,29 +217,6 @@ func TestFiles(t *testing.T) {
 				}
 			}
 
-			for _, tc := range tt.findDescs {
-				got, err := files.FindDescriptorByName(tc.inName)
-				if (got == nil) == (err == nil) {
-					if tc.wantOk {
-						t.Errorf("FindDescriptorByName(%v) = (%v, %v), want (non-nil, nil)", tc.inName, got, err)
-					} else {
-						t.Errorf("FindDescriptorByName(%v) = (%v, %v), want (nil, NotFound)", tc.inName, got, err)
-					}
-				}
-
-				gotName := pref.FullName("<nil>")
-				if got != nil {
-					gotName = got.FullName()
-				}
-				wantName := pref.FullName("<nil>")
-				if tc.wantOk {
-					wantName = tc.inName
-				}
-				if gotName != wantName {
-					t.Errorf("FindDescriptorByName(%v) = %v, want %v", tc.inName, gotName, wantName)
-				}
-			}
-
 			for _, tc := range tt.rangePkgs {
 				var gotFiles []file
 				files.RangeFilesByPackage(tc.inPkg, func(fd pref.FileDescriptor) bool {
@@ -315,6 +242,64 @@ func TestFiles(t *testing.T) {
 	}
 }
 
+func TestFilesLookup(t *testing.T) {
+	files := []pref.FileDescriptor{
+		test2pb.File_test_test_proto,
+		test2pb.File_test_test_import_proto,
+	}
+	r := preg.NewFiles(files...)
+	checkEnums := func(enums pref.EnumDescriptors) {
+		for i := 0; i < enums.Len(); i++ {
+			want := enums.Get(i)
+			if got, err := r.FindEnumByName(want.FullName()); err != nil {
+				t.Errorf("FindEnumByName(%q): unexpected error: %v", want.FullName(), err)
+			} else if got != want {
+				t.Errorf("FindEnumByName(%q): found descriptor %v (%p), %p", want.FullName(), got.FullName(), got, want)
+			}
+		}
+	}
+	checkExtensions := func(exts pref.ExtensionDescriptors) {
+		for i := 0; i < exts.Len(); i++ {
+			want := exts.Get(i)
+			if got, err := r.FindExtensionByName(want.FullName()); err != nil {
+				t.Errorf("FindExtensionByName(%q): unexpected error: %v", want.FullName(), err)
+			} else if got != want {
+				t.Errorf("FindExtensionByName(%q): found descriptor %v (%p), %p", want.FullName(), got.FullName(), got, want)
+			}
+		}
+	}
+	var checkMessages func(pref.MessageDescriptors)
+	checkMessages = func(messages pref.MessageDescriptors) {
+		for i := 0; i < messages.Len(); i++ {
+			want := messages.Get(i)
+			if got, err := r.FindMessageByName(want.FullName()); err != nil {
+				t.Errorf("FindMessageByName(%q): unexpected error: %v", want.FullName(), err)
+			} else if got != want {
+				t.Errorf("FindMessageByName(%q): found descriptor %v (%p), %p", want.FullName(), got.FullName(), got, want)
+			}
+			checkEnums(want.Enums())
+			checkExtensions(want.Extensions())
+			checkMessages(want.Messages())
+		}
+	}
+	checkServices := func(services pref.ServiceDescriptors) {
+		for i := 0; i < services.Len(); i++ {
+			want := services.Get(i)
+			if got, err := r.FindServiceByName(want.FullName()); err != nil {
+				t.Errorf("FindServiceByName(%q): unexpected error: %v", want.FullName(), err)
+			} else if got != want {
+				t.Errorf("FindServiceByName(%q): found descriptor %v (%p), %p", want.FullName(), got.FullName(), got, want)
+			}
+		}
+	}
+	for _, fd := range files {
+		checkEnums(fd.Enums())
+		checkExtensions(fd.Extensions())
+		checkMessages(fd.Messages())
+		checkServices(fd.Services())
+	}
+}
+
 func TestTypes(t *testing.T) {
 	// Suffix 1 in registry, 2 in parent, 3 in resolver.
 	mt1 := (&testpb.Message1{}).ProtoReflect().Type()