Skip to content

Commit

Permalink
protoparse: fix scoping issue for extendees and field type names; als…
Browse files Browse the repository at this point in the history 
…o fix articles in error messages (#520)
  • Loading branch information
@jhump
jhump authored Jul 26, 2022
1 parent 060cc04 commit 25e6f5d
Show file tree
Hide file tree
Showing 3 changed files with 139 additions and 30 deletions.
59 changes: 45 additions & 14 deletions desc/protoparse/linker.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ func (l *linker) createDescriptorPool() error {
// info than panic with a nil dereference below
node = ast.NewNoSourceNode(file2)
}
if err := l.errs.handleErrorWithPos(node.Start(), "duplicate symbol %s: already defined as %s in %q", k, descriptorType(desc1), file1); err != nil {
if err := l.errs.handleErrorWithPos(node.Start(), "duplicate symbol %s: already defined as %s in %q", k, descriptorTypeWithArticle(desc1), file1); err != nil {
return err
}
}
Expand Down Expand Up @@ -267,7 +267,7 @@ func addToPool(r *parseResult, pool map[string]proto.Message, errs *errorHandler
suffix = "; protobuf uses C++ scoping rules for enum values, so they exist in the scope enclosing the enum"
}
// TODO: also include the source location for the conflicting symbol
if err := errs.handleErrorWithPos(node.Start(), "duplicate symbol %s: already defined as %s%s", fqn, descriptorType(d), suffix); err != nil {
if err := errs.handleErrorWithPos(node.Start(), "duplicate symbol %s: already defined as %s%s", fqn, descriptorTypeWithArticle(d), suffix); err != nil {
return err
}
}
Expand Down Expand Up @@ -305,6 +305,36 @@ func descriptorType(m proto.Message) string {
}
}

func descriptorTypeWithArticle(m proto.Message) string {
switch m := m.(type) {
case *dpb.DescriptorProto:
return "a message"
case *dpb.DescriptorProto_ExtensionRange:
return "an extension range"
case *dpb.FieldDescriptorProto:
if m.GetExtendee() == "" {
return "a field"
} else {
return "an extension"
}
case *dpb.EnumDescriptorProto:
return "an enum"
case *dpb.EnumValueDescriptorProto:
return "an enum value"
case *dpb.ServiceDescriptorProto:
return "a service"
case *dpb.MethodDescriptorProto:
return "a method"
case *dpb.FileDescriptorProto:
return "a file"
case *dpb.OneofDescriptorProto:
return "a oneof"
default:
// shouldn't be possible
return fmt.Sprintf("a %T", m)
}
}

func (l *linker) resolveReferences() error {
l.extensions = map[string]map[int32]string{}
l.usedImports = map[*dpb.FileDescriptorProto]map[string]struct{}{}
Expand Down Expand Up @@ -369,7 +399,7 @@ func (l *linker) resolveMessageTypes(r *parseResult, fd *dpb.FileDescriptorProto
// Strangely, when protoc resolves extension names, it uses the *enclosing* scope
// instead of the message's scope. So if the message contains an extension named "i",
// an option cannot refer to it as simply "i" but must qualify it (at a minimum "Msg.i").
// So we don't add this messages scope to our scopes slice until *after* we do options.
// So we don't add this message's scope to our scopes slice until *after* we do options.
if md.Options != nil {
if err := l.resolveOptions(r, fd, "message", fqn, md.Options.UninterpretedOption, scopes); err != nil {
return err
Expand Down Expand Up @@ -435,8 +465,7 @@ func (l *linker) resolveFieldTypes(r *parseResult, fd *dpb.FileDescriptorProto,
}
extd, ok := dsc.(*dpb.DescriptorProto)
if !ok {
otherType := descriptorType(dsc)
return l.errs.handleErrorWithPos(node.FieldExtendee().Start(), "extendee is invalid: %s is a %s, not a message", fqn, otherType)
return l.errs.handleErrorWithPos(node.FieldExtendee().Start(), "extendee is invalid: %s is %s, not a message", fqn, descriptorTypeWithArticle(dsc))
}
fld.Extendee = proto.String("." + fqn)
// make sure the tag number is in range
Expand Down Expand Up @@ -503,8 +532,7 @@ func (l *linker) resolveFieldTypes(r *parseResult, fd *dpb.FileDescriptorProto,
// the type was tentatively unset, but now we know it's actually an enum
fld.Type = dpb.FieldDescriptorProto_TYPE_ENUM.Enum()
default:
otherType := descriptorType(dsc)
return l.errs.handleErrorWithPos(node.FieldType().Start(), "%s: invalid type: %s is a %s, not a message or enum", scope, fqn, otherType)
return l.errs.handleErrorWithPos(node.FieldType().Start(), "%s: invalid type: %s is %s, not a message or enum", scope, fqn, descriptorTypeWithArticle(dsc))
}
return nil
}
Expand Down Expand Up @@ -539,8 +567,7 @@ func (l *linker) resolveServiceTypes(r *parseResult, fd *dpb.FileDescriptorProto
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 {
if err := l.errs.handleErrorWithPos(node.GetInputType().Start(), "%s: invalid request type: %s is %s, not a message", scope, fqn, descriptorTypeWithArticle(dsc)); err != nil {
return err
}
} else {
Expand All @@ -558,8 +585,7 @@ func (l *linker) resolveServiceTypes(r *parseResult, fd *dpb.FileDescriptorProto
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 {
if err := l.errs.handleErrorWithPos(node.GetOutputType().Start(), "%s: invalid response type: %s is %s, not a message", scope, fqn, descriptorTypeWithArticle(dsc)); err != nil {
return err
}
} else {
Expand Down Expand Up @@ -663,8 +689,7 @@ func (l *linker) resolveExtensionName(name string, fd *dpb.FileDescriptorProto,
return "", fmt.Errorf("unknown extension %s; resolved to %s which is not defined; consider using a leading dot", name, fqn)
}
if ext, ok := dsc.(*dpb.FieldDescriptorProto); !ok {
otherType := descriptorType(dsc)
return "", fmt.Errorf("invalid extension: %s is a %s, not an extension", name, otherType)
return "", fmt.Errorf("invalid extension: %s is %s, not an extension", name, descriptorTypeWithArticle(dsc))
} else if ext.GetExtendee() == "" {
return "", fmt.Errorf("invalid extension: %s is a field but not an extension", name)
}
Expand Down Expand Up @@ -693,7 +718,13 @@ func (l *linker) resolve(fd *dpb.FileDescriptorProto, name string, onlyTypes boo
for i := len(scopes) - 1; i >= 0; i-- {
fqn, d, proto3 := scopes[i](firstName, name)
if d != nil {
if !onlyTypes || isType(d) {
// In `protoc`, it will skip a match of the wrong type and move on
// to the next scope, but only if the reference is unqualified. So
// we mirror that behavior here. When we skip and move on, we go
// ahead and save the match of the wrong type so we can at least use
// it to construct a better error in the event that we don't find
// any match of the right type.
if !onlyTypes || isType(d) || firstName != name {
return fqn, d, proto3
} else if bestGuess == nil {
bestGuess = d
Expand Down
100 changes: 89 additions & 11 deletions desc/protoparse/linker_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,26 +137,26 @@ func TestLinkerValidation(t *testing.T) {
map[string]string{
"foo.proto": "enum foo { bar = 1; baz = 2; } enum fu { bar = 1; baz = 2; }",
},
`foo.proto:1:42: duplicate symbol bar: already defined as enum value; protobuf uses C++ scoping rules for enum values, so they exist in the scope enclosing the enum`,
`foo.proto:1:42: duplicate symbol bar: already defined as an enum value; protobuf uses C++ scoping rules for enum values, so they exist in the scope enclosing the enum`,
},
{
map[string]string{
"foo.proto": "message foo {} enum foo { V = 0; }",
},
"foo.proto:1:16: duplicate symbol foo: already defined as message",
"foo.proto:1:16: duplicate symbol foo: already defined as a message",
},
{
map[string]string{
"foo.proto": "message foo { optional string a = 1; optional string a = 2; }",
},
"foo.proto:1:38: duplicate symbol foo.a: already defined as field",
"foo.proto:1:38: duplicate symbol foo.a: already defined as a field",
},
{
map[string]string{
"foo.proto": "message foo {}",
"foo2.proto": "enum foo { V = 0; }",
},
"foo2.proto:1:1: duplicate symbol foo: already defined as message in \"foo.proto\"",
"foo2.proto:1:1: duplicate symbol foo: already defined as a message in \"foo.proto\"",
},
{
map[string]string{
Expand Down Expand Up @@ -185,7 +185,7 @@ func TestLinkerValidation(t *testing.T) {
message DescriptorProto { }
`,
},
`google/protobuf/descriptor.proto: duplicate symbol google.protobuf.DescriptorProto: already defined as message in "foo.proto"`,
`google/protobuf/descriptor.proto: duplicate symbol google.protobuf.DescriptorProto: already defined as a message in "foo.proto"`,
},
{
map[string]string{
Expand Down Expand Up @@ -583,7 +583,7 @@ func TestLinkerValidation(t *testing.T) {
" }\n" +
"}",
},
`a.proto:4:5: duplicate symbol m.z: already defined as oneof`,
`a.proto:4:5: duplicate symbol m.z: already defined as a oneof`,
},
{
map[string]string{
Expand All @@ -593,7 +593,7 @@ func TestLinkerValidation(t *testing.T) {
" oneof z{int64 b=2;}\n" +
"}",
},
`a.proto:3:3: duplicate symbol m.z: already defined as oneof`,
`a.proto:3:3: duplicate symbol m.z: already defined as a oneof`,
},
{
map[string]string{
Expand Down Expand Up @@ -718,7 +718,7 @@ func TestLinkerValidation(t *testing.T) {
" oneof z{int64 b=2;}\n" +
"}",
},
`a.proto:4:3: duplicate symbol m.z: already defined as oneof`,
`a.proto:4:3: duplicate symbol m.z: already defined as a oneof`,
},
{
map[string]string{
Expand Down Expand Up @@ -906,6 +906,84 @@ func TestLinkerValidation(t *testing.T) {
},
"foo.proto:4:26: field foobar: option json_name is not allowed on extensions",
},
{
map[string]string{
"foo.proto": "syntax = \"proto3\";\n" +
"package foo.foo;\n" +
"import \"other.proto\";\n" +
"service Foo { rpc Bar (Baz) returns (Baz); }\n" +
"message Baz {\n" +
" foo.Foo.Bar f = 1;\n" +
"}\n",
"other.proto": "syntax = \"proto3\";\n" +
"package foo;\n" +
"message Foo {\n" +
" enum Bar { ZED = 0; }\n" +
"}\n",
},
"foo.proto:6:3: field foo.foo.Baz.f: invalid type: foo.foo.Foo.Bar is a method, not a message or enum",
},
{
map[string]string{
"foo.proto": "syntax = \"proto3\";\n" +
"import \"google/protobuf/descriptor.proto\";\n" +
"message Foo {\n" +
" enum Bar { ZED = 0; }\n" +
" message Foo {\n" +
" extend google.protobuf.MessageOptions {\n" +
" string Bar = 30000;\n" +
" }\n" +
" Foo.Bar f = 1;\n" +
" }\n" +
"}\n",
},
"foo.proto:9:5: field Foo.Foo.f: invalid type: Foo.Foo.Bar is an extension, not a message or enum",
},
{
map[string]string{
"foo.proto": "syntax = \"proto3\";\n" +
"import \"google/protobuf/descriptor.proto\";\n" +
"extend google.protobuf.ServiceOptions {\n" +
" string Bar = 30000;\n" +
"}\n" +
"message Empty {}\n" +
"service Foo {\n" +
" option (Bar) = \"blah\";\n" +
" rpc Bar (Empty) returns (Empty);\n" +
"}\n",
},
"", // should succeed
},
{
map[string]string{
"foo.proto": "syntax = \"proto3\";\n" +
"import \"google/protobuf/descriptor.proto\";\n" +
"extend google.protobuf.MethodOptions {\n" +
" string Bar = 30000;\n" +
"}\n" +
"message Empty {}\n" +
"service Foo {\n" +
" rpc Bar (Empty) returns (Empty) { option (Bar) = \"blah\"; }\n" +
"}\n",
},
"foo.proto:8:44: method Foo.Bar: invalid extension: Bar is a method, not an extension",
},
{
map[string]string{
"foo.proto": "syntax = \"proto3\";\n" +
"import \"google/protobuf/descriptor.proto\";\n" +
"enum Bar { ZED = 0; }\n" +
"message Foo {\n" +
" extend google.protobuf.MessageOptions {\n" +
" string Bar = 30000;\n" +
" }\n" +
" message Foo {\n" +
" Bar f = 1;\n" +
" }\n" +
"}\n",
},
"", // should succeed
},
}
for i, tc := range testCases {
acc := func(filename string) (io.ReadCloser, error) {
Expand Down Expand Up @@ -1070,9 +1148,9 @@ func TestSyntheticOneOfCollisions(t *testing.T) {
testutil.Eq(t, ErrInvalidSource, err)

expected := []string{
`foo2.proto:2:1: duplicate symbol Foo: already defined as message in "foo1.proto"`,
`foo2.proto:3:3: duplicate symbol Foo._bar: already defined as oneof in "foo1.proto"`,
`foo2.proto:3:3: duplicate symbol Foo.bar: already defined as field in "foo1.proto"`,
`foo2.proto:2:1: duplicate symbol Foo: already defined as a message in "foo1.proto"`,
`foo2.proto:3:3: duplicate symbol Foo._bar: already defined as a oneof in "foo1.proto"`,
`foo2.proto:3:3: duplicate symbol Foo.bar: already defined as a field in "foo1.proto"`,
}
var actual []string
for _, err := range errs {
Expand Down
10 changes: 5 additions & 5 deletions desc/protoparse/reporting_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,9 +104,9 @@ func TestErrorReporting(t *testing.T) {
`,
},
expectedErrs: []string{
"test.proto:8:49: duplicate symbol BAZ: already defined as enum value; protobuf uses C++ scoping rules for enum values, so they exist in the scope enclosing the enum",
"test.proto:10:41: duplicate symbol Bar: already defined as enum",
"test.proto:12:49: duplicate symbol Bar.Foobar: already defined as method",
"test.proto:8:49: duplicate symbol BAZ: already defined as an enum value; protobuf uses C++ scoping rules for enum values, so they exist in the scope enclosing the enum",
"test.proto:10:41: duplicate symbol Bar: already defined as an enum",
"test.proto:12:49: duplicate symbol Bar.Foobar: already defined as a method",
"test.proto:12:61: method Bar.Foobar: unknown request type Frob",
"test.proto:12:76: method Bar.Foobar: unknown response type Nitz",
},
Expand Down Expand Up @@ -168,8 +168,8 @@ func TestErrorReporting(t *testing.T) {
`,
},
expectedErrs: []string{
"test2.proto:4:41: duplicate symbol Bar: already defined as service in \"test1.proto\"",
"test2.proto:7:41: duplicate symbol Foo: already defined as message in \"test1.proto\"",
"test2.proto:4:41: duplicate symbol Bar: already defined as a service in \"test1.proto\"",
"test2.proto:7:41: duplicate symbol Foo: already defined as a message in \"test1.proto\"",
"test1.proto:8:75: method Bar.Frob: unknown response type Nitz",
"test2.proto:8:82: method Foo.DoSomething: invalid response type: Foo is a service, not a message",
},
Expand Down

0 comments on commit 25e6f5d

Please sign in to comment.