Skip to content

Commit

Permalink
protoparse: fix symbol resolution to correctly mimic protoc behavior,…
Browse files Browse the repository at this point in the history 
… which is more strict (#393)
  • Loading branch information
@jhump
jhump authored May 27, 2021
1 parent 2837af4 commit 05026f3
Show file tree
Hide file tree
Showing 2 changed files with 129 additions and 52 deletions.
173 changes: 122 additions & 51 deletions desc/protoparse/linker.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -361,10 +361,13 @@ func (l *linker) resolveFieldTypes(r *parseResult, fd *dpb.FileDescriptorProto,
elemType := "field"
if fld.GetExtendee() != "" {
elemType = "extension"
fqn, dsc, _ := l.resolve(fd, fld.GetExtendee(), isMessage, scopes)
fqn, dsc, _ := l.resolve(fd, fld.GetExtendee(), true, scopes)
if dsc == nil {
return l.errs.handleErrorWithPos(node.FieldExtendee().Start(), "unknown extendee type %s", fld.GetExtendee())
}
if dsc == sentinelMissingSymbol {
return l.errs.handleErrorWithPos(node.FieldExtendee().Start(), "unknown extendee type %s; resolved to %s which is not defined; consider using a leading dot", fld.GetExtendee(), fqn)
}
extd, ok := dsc.(*dpb.DescriptorProto)
if !ok {
otherType := descriptorType(dsc)
Expand Down Expand Up @@ -412,10 +415,13 @@ func (l *linker) resolveFieldTypes(r *parseResult, fd *dpb.FileDescriptorProto,
return nil
}

fqn, dsc, proto3 := l.resolve(fd, fld.GetTypeName(), isType, scopes)
fqn, dsc, proto3 := l.resolve(fd, fld.GetTypeName(), true, scopes)
if dsc == nil {
return l.errs.handleErrorWithPos(node.FieldType().Start(), "%s: unknown type %s", scope, fld.GetTypeName())
}
if dsc == sentinelMissingSymbol {
return l.errs.handleErrorWithPos(node.FieldType().Start(), "%s: unknown type %s; resolved to %s which is not defined; consider using a leading dot", scope, fld.GetTypeName(), fqn)
}
switch dsc := dsc.(type) {
case *dpb.DescriptorProto:
fld.TypeName = proto.String("." + fqn)
Expand Down Expand Up @@ -454,11 +460,15 @@ func (l *linker) resolveServiceTypes(r *parseResult, fd *dpb.FileDescriptorProto
}
scope := fmt.Sprintf("method %s.%s", thisName, mtd.GetName())
node := r.getMethodNode(mtd)
fqn, dsc, _ := l.resolve(fd, mtd.GetInputType(), isMessage, scopes)
fqn, dsc, _ := l.resolve(fd, mtd.GetInputType(), true, scopes)
if dsc == nil {
if err := l.errs.handleErrorWithPos(node.GetInputType().Start(), "%s: unknown request type %s", scope, mtd.GetInputType()); err != nil {
return err
}
} else if dsc == sentinelMissingSymbol {
if err := l.errs.handleErrorWithPos(node.GetInputType().Start(), "%s: unknown request type %s; resolved to %s which is not defined; consider using a leading dot", scope, mtd.GetInputType(), fqn); err != nil {
return err
}
} else if _, ok := dsc.(*dpb.DescriptorProto); !ok {
otherType := descriptorType(dsc)
if err := l.errs.handleErrorWithPos(node.GetInputType().Start(), "%s: invalid request type: %s is a %s, not a message", scope, fqn, otherType); err != nil {
Expand All @@ -468,11 +478,16 @@ func (l *linker) resolveServiceTypes(r *parseResult, fd *dpb.FileDescriptorProto
mtd.InputType = proto.String("." + fqn)
}

fqn, dsc, _ = l.resolve(fd, mtd.GetOutputType(), isMessage, scopes)
// TODO: make input and output type resolution more DRY
fqn, dsc, _ = l.resolve(fd, mtd.GetOutputType(), true, scopes)
if dsc == nil {
if err := l.errs.handleErrorWithPos(node.GetOutputType().Start(), "%s: unknown response type %s", scope, mtd.GetOutputType()); err != nil {
return err
}
} else if dsc == sentinelMissingSymbol {
if err := l.errs.handleErrorWithPos(node.GetOutputType().Start(), "%s: unknown response type %s; resolved to %s which is not defined; consider using a leading dot", scope, mtd.GetOutputType(), fqn); err != nil {
return err
}
} else if _, ok := dsc.(*dpb.DescriptorProto); !ok {
otherType := descriptorType(dsc)
if err := l.errs.handleErrorWithPos(node.GetOutputType().Start(), "%s: invalid response type: %s is a %s, not a message", scope, fqn, otherType); err != nil {
Expand All @@ -495,13 +510,19 @@ opts:
for _, nm := range opt.Name {
if nm.GetIsExtension() {
node := r.getOptionNamePartNode(nm)
fqn, dsc, _ := l.resolve(fd, nm.GetNamePart(), isField, scopes)
fqn, dsc, _ := l.resolve(fd, nm.GetNamePart(), false, scopes)
if dsc == nil {
if err := l.errs.handleErrorWithPos(node.Start(), "%sunknown extension %s", scope, nm.GetNamePart()); err != nil {
return err
}
continue opts
}
if dsc == sentinelMissingSymbol {
if err := l.errs.handleErrorWithPos(node.Start(), "%sunknown extension %s; resolved to %s which is not defined; consider using a leading dot", scope, nm.GetNamePart(), fqn); err != nil {
return err
}
continue opts
}
if ext, ok := dsc.(*dpb.FieldDescriptorProto); !ok {
otherType := descriptorType(dsc)
if err := l.errs.handleErrorWithPos(node.Start(), "%sinvalid extension: %s is a %s, not an extension", scope, nm.GetNamePart(), otherType); err != nil {
Expand All @@ -521,47 +542,41 @@ opts:
return nil
}

func (l *linker) resolve(fd *dpb.FileDescriptorProto, name string, allowed func(proto.Message) bool, scopes []scope) (fqn string, element proto.Message, proto3 bool) {
func (l *linker) resolve(fd *dpb.FileDescriptorProto, name string, onlyTypes bool, scopes []scope) (fqn string, element proto.Message, proto3 bool) {
if strings.HasPrefix(name, ".") {
// already fully-qualified
d, proto3 := l.findSymbol(fd, name[1:], false, map[*dpb.FileDescriptorProto]struct{}{})
d, proto3 := l.findSymbol(fd, name[1:], name[1:], false, map[*dpb.FileDescriptorProto]struct{}{})
if d != nil {
return name[1:], d, proto3
}
} else {
// unqualified, so we look in the enclosing (last) scope first and move
// towards outermost (first) scope, trying to resolve the symbol
var bestGuess proto.Message
var bestGuessFqn string
var bestGuessProto3 bool
for i := len(scopes) - 1; i >= 0; i-- {
fqn, d, proto3 := scopes[i](name)
if d != nil {
if allowed(d) {
return fqn, d, proto3
} else if bestGuess == nil {
bestGuess = d
bestGuessFqn = fqn
bestGuessProto3 = proto3
}
return "", nil, false
}
// unqualified, so we look in the enclosing (last) scope first and move
// towards outermost (first) scope, trying to resolve the symbol
pos := strings.IndexByte(name, '.')
firstName := name
if pos > 0 {
firstName = name[:pos]
}
var bestGuess proto.Message
var bestGuessFqn string
var bestGuessProto3 bool
for i := len(scopes) - 1; i >= 0; i-- {
fqn, d, proto3 := scopes[i](firstName, name)
if d != nil {
if !onlyTypes || isType(d) {
return fqn, d, proto3
} else if bestGuess == nil {
bestGuess = d
bestGuessFqn = fqn
bestGuessProto3 = proto3
}
}
// we return best guess, even though it was not an allowed kind of
// descriptor, so caller can print a better error message (e.g.
// indicating that the name was found but that it's the wrong type)
return bestGuessFqn, bestGuess, bestGuessProto3
}
return "", nil, false
}

func isField(m proto.Message) bool {
_, ok := m.(*dpb.FieldDescriptorProto)
return ok
}

func isMessage(m proto.Message) bool {
_, ok := m.(*dpb.DescriptorProto)
return ok
// we return best guess, even though it was not an allowed kind of
// descriptor, so caller can print a better error message (e.g.
// indicating that the name was found but that it's the wrong type)
return bestGuessFqn, bestGuess, bestGuessProto3
}

func isType(m proto.Message) bool {
Expand All @@ -574,47 +589,103 @@ func isType(m proto.Message) bool {

// scope represents a lexical scope in a proto file in which messages and enums
// can be declared.
type scope func(symbol string) (fqn string, element proto.Message, proto3 bool)
type scope func(firstName, fullName string) (fqn string, element proto.Message, proto3 bool)

func fileScope(fd *dpb.FileDescriptorProto, l *linker) scope {
// we search symbols in this file, but also symbols in other files that have
// the same package as this file or a "parent" package (in protobuf,
// packages are a hierarchy like C++ namespaces)
prefixes := internal.CreatePrefixList(fd.GetPackage())
return func(name string) (string, proto.Message, bool) {
return func(firstName, fullName string) (string, proto.Message, bool) {
prefixMatch := false
for _, prefix := range prefixes {
var n string
var n1, n string
if prefix == "" {
n = name
// exhausted all prefixes, so it must be in this one
n1, n = fullName, fullName
} else {
n = prefix + "." + name
n = prefix + "." + fullName
if prefixMatch {
// it must be in this prefix, so search for full
// name (first name must be a package component)
n1 = n
} else {
n1 = prefix + "." + firstName
}
}
d, proto3 := l.findSymbol(fd, n, false, map[*dpb.FileDescriptorProto]struct{}{})
d, proto3 := l.findSymbol(fd, n1, n, false, map[*dpb.FileDescriptorProto]struct{}{})
if d != nil {
return n, d, proto3
}
if prefixMatch {
// we were supposed to find the symbol with this prefix but
// didn't, so we need to go ahead and return sentinel value
return n, sentinelMissingSymbol, false
}
if strings.HasSuffix(prefix, "."+firstName) {
// the first name matches the end of this prefix, which means
// the *next* scope is the match
prefixMatch = true
}
}
return "", nil, false
}
}

func messageScope(messageName string, proto3 bool, filePool map[string]proto.Message) scope {
return func(name string) (string, proto.Message, bool) {
n := messageName + "." + name
if d, ok := filePool[n]; ok {
return func(firstName, fullName string) (string, proto.Message, bool) {
n1 := messageName + "." + firstName
n := messageName + "." + fullName
d := findSymbolInPool(n1, n, filePool)
if d != nil {
return n, d, proto3
}
return "", nil, false
}
}

func (l *linker) findSymbol(fd *dpb.FileDescriptorProto, name string, public bool, checked map[*dpb.FileDescriptorProto]struct{}) (element proto.Message, proto3 bool) {
func findSymbolInPool(firstName, fullName string, pool map[string]proto.Message) proto.Message {
d, ok := pool[firstName]
if !ok {
return nil
}
if firstName == fullName {
return d
}
if !isAggregateDescriptor(d) {
// can't possibly find the rest of full name if
// the first name indicated a leaf descriptor
return nil
}
d, ok = pool[fullName]
if !ok {
return sentinelMissingSymbol
}
return d
}

func isAggregateDescriptor(m proto.Message) bool {
switch m.(type) {
case *dpb.DescriptorProto, *dpb.EnumDescriptorProto, *dpb.ServiceDescriptorProto:
return true
default:
return false
}
}

// This value is a bogus/nil value, but results in a non-nil
// proto.Message interface value. So we use it as a sentinel
// to indicate "stop searching for symbol... because it
// definitively does not exist".
var sentinelMissingSymbol = (*dpb.DescriptorProto)(nil)

func (l *linker) findSymbol(fd *dpb.FileDescriptorProto, firstName, fullName string, public bool, checked map[*dpb.FileDescriptorProto]struct{}) (element proto.Message, proto3 bool) {
if _, ok := checked[fd]; ok {
// already checked this one
return nil, false
}
checked[fd] = struct{}{}
d := l.descriptorPool[fd][name]
d := findSymbolInPool(firstName, fullName, l.descriptorPool[fd])
if d != nil {
return d, isProto3(fd)
}
Expand All @@ -629,7 +700,7 @@ func (l *linker) findSymbol(fd *dpb.FileDescriptorProto, name string, public boo
// we'll catch this error later
continue
}
if d, proto3 := l.findSymbol(depres.fd, name, true, checked); d != nil {
if d, proto3 := l.findSymbol(depres.fd, firstName, fullName, true, checked); d != nil {
return d, proto3
}
}
Expand All @@ -640,7 +711,7 @@ func (l *linker) findSymbol(fd *dpb.FileDescriptorProto, name string, public boo
// we'll catch this error later
continue
}
if d, proto3 := l.findSymbol(depres.fd, name, true, checked); d != nil {
if d, proto3 := l.findSymbol(depres.fd, firstName, fullName, true, checked); d != nil {
return d, proto3
}
}
Expand Down
8 changes: 7 additions & 1 deletion desc/protoparse/linker_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ func TestLinkerValidation(t *testing.T) {
"foo2.proto": "package fu.baz; import \"foo3.proto\"; message fizzle{ }",
"foo3.proto": "package fu.baz; message baz{ }",
},
"foo.proto:1:70: field fu.baz.foobar.a: unknown type baz",
"foo.proto:1:70: field fu.baz.foobar.a: unknown type baz; resolved to fu.baz which is not defined; consider using a leading dot",
},
{
map[string]string{
Expand Down Expand Up @@ -349,6 +349,12 @@ func TestLinkerValidation(t *testing.T) {
},
"", // should succeed
},
{
map[string]string{
"foo.proto": `syntax = "proto3"; package com.google; import "google/protobuf/wrappers.proto"; message Foo { google.protobuf.StringValue str = 1; }`,
},
"foo.proto:1:95: field com.google.Foo.str: unknown type google.protobuf.StringValue; resolved to com.google.protobuf.StringValue which is not defined; consider using a leading dot",
},
}
for i, tc := range testCases {
acc := func(filename string) (io.ReadCloser, error) {
Expand Down

0 comments on commit 05026f3

Please sign in to comment.