Skip to content

Commit 25e6f5d

Browse files
jhumpjhump
authored
protoparse: fix scoping issue for extendees and field type names; also fix articles in error messages (#520)
1 parent 060cc04 commit 25e6f5d

File tree

3 files changed

+139
-30
lines changed

3 files changed

+139
-30
lines changed

‎desc/protoparse/linker.go

+45-14
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ func (l *linker) createDescriptorPool() error {
152152
// info than panic with a nil dereference below
153153
node = ast.NewNoSourceNode(file2)
154154
}
155-
if err := l.errs.handleErrorWithPos(node.Start(), "duplicate symbol %s: already defined as %s in %q", k, descriptorType(desc1), file1); err != nil {
155+
if err := l.errs.handleErrorWithPos(node.Start(), "duplicate symbol %s: already defined as %s in %q", k, descriptorTypeWithArticle(desc1), file1); err != nil {
156156
return err
157157
}
158158
}
@@ -267,7 +267,7 @@ func addToPool(r *parseResult, pool map[string]proto.Message, errs *errorHandler
267267
suffix = "; protobuf uses C++ scoping rules for enum values, so they exist in the scope enclosing the enum"
268268
}
269269
// TODO: also include the source location for the conflicting symbol
270-
if err := errs.handleErrorWithPos(node.Start(), "duplicate symbol %s: already defined as %s%s", fqn, descriptorType(d), suffix); err != nil {
270+
if err := errs.handleErrorWithPos(node.Start(), "duplicate symbol %s: already defined as %s%s", fqn, descriptorTypeWithArticle(d), suffix); err != nil {
271271
return err
272272
}
273273
}
@@ -305,6 +305,36 @@ func descriptorType(m proto.Message) string {
305305
}
306306
}
307307

308+
func descriptorTypeWithArticle(m proto.Message) string {
309+
switch m := m.(type) {
310+
case *dpb.DescriptorProto:
311+
return "a message"
312+
case *dpb.DescriptorProto_ExtensionRange:
313+
return "an extension range"
314+
case *dpb.FieldDescriptorProto:
315+
if m.GetExtendee() == "" {
316+
return "a field"
317+
} else {
318+
return "an extension"
319+
}
320+
case *dpb.EnumDescriptorProto:
321+
return "an enum"
322+
case *dpb.EnumValueDescriptorProto:
323+
return "an enum value"
324+
case *dpb.ServiceDescriptorProto:
325+
return "a service"
326+
case *dpb.MethodDescriptorProto:
327+
return "a method"
328+
case *dpb.FileDescriptorProto:
329+
return "a file"
330+
case *dpb.OneofDescriptorProto:
331+
return "a oneof"
332+
default:
333+
// shouldn't be possible
334+
return fmt.Sprintf("a %T", m)
335+
}
336+
}
337+
308338
func (l *linker) resolveReferences() error {
309339
l.extensions = map[string]map[int32]string{}
310340
l.usedImports = map[*dpb.FileDescriptorProto]map[string]struct{}{}
@@ -369,7 +399,7 @@ func (l *linker) resolveMessageTypes(r *parseResult, fd *dpb.FileDescriptorProto
369399
// Strangely, when protoc resolves extension names, it uses the *enclosing* scope
370400
// instead of the message's scope. So if the message contains an extension named "i",
371401
// an option cannot refer to it as simply "i" but must qualify it (at a minimum "Msg.i").
372-
// So we don't add this messages scope to our scopes slice until *after* we do options.
402+
// So we don't add this message's scope to our scopes slice until *after* we do options.
373403
if md.Options != nil {
374404
if err := l.resolveOptions(r, fd, "message", fqn, md.Options.UninterpretedOption, scopes); err != nil {
375405
return err
@@ -435,8 +465,7 @@ func (l *linker) resolveFieldTypes(r *parseResult, fd *dpb.FileDescriptorProto,
435465
}
436466
extd, ok := dsc.(*dpb.DescriptorProto)
437467
if !ok {
438-
otherType := descriptorType(dsc)
439-
return l.errs.handleErrorWithPos(node.FieldExtendee().Start(), "extendee is invalid: %s is a %s, not a message", fqn, otherType)
468+
return l.errs.handleErrorWithPos(node.FieldExtendee().Start(), "extendee is invalid: %s is %s, not a message", fqn, descriptorTypeWithArticle(dsc))
440469
}
441470
fld.Extendee = proto.String("." + fqn)
442471
// make sure the tag number is in range
@@ -503,8 +532,7 @@ func (l *linker) resolveFieldTypes(r *parseResult, fd *dpb.FileDescriptorProto,
503532
// the type was tentatively unset, but now we know it's actually an enum
504533
fld.Type = dpb.FieldDescriptorProto_TYPE_ENUM.Enum()
505534
default:
506-
otherType := descriptorType(dsc)
507-
return l.errs.handleErrorWithPos(node.FieldType().Start(), "%s: invalid type: %s is a %s, not a message or enum", scope, fqn, otherType)
535+
return l.errs.handleErrorWithPos(node.FieldType().Start(), "%s: invalid type: %s is %s, not a message or enum", scope, fqn, descriptorTypeWithArticle(dsc))
508536
}
509537
return nil
510538
}
@@ -539,8 +567,7 @@ func (l *linker) resolveServiceTypes(r *parseResult, fd *dpb.FileDescriptorProto
539567
return err
540568
}
541569
} else if _, ok := dsc.(*dpb.DescriptorProto); !ok {
542-
otherType := descriptorType(dsc)
543-
if err := l.errs.handleErrorWithPos(node.GetInputType().Start(), "%s: invalid request type: %s is a %s, not a message", scope, fqn, otherType); err != nil {
570+
if err := l.errs.handleErrorWithPos(node.GetInputType().Start(), "%s: invalid request type: %s is %s, not a message", scope, fqn, descriptorTypeWithArticle(dsc)); err != nil {
544571
return err
545572
}
546573
} else {
@@ -558,8 +585,7 @@ func (l *linker) resolveServiceTypes(r *parseResult, fd *dpb.FileDescriptorProto
558585
return err
559586
}
560587
} else if _, ok := dsc.(*dpb.DescriptorProto); !ok {
561-
otherType := descriptorType(dsc)
562-
if err := l.errs.handleErrorWithPos(node.GetOutputType().Start(), "%s: invalid response type: %s is a %s, not a message", scope, fqn, otherType); err != nil {
588+
if err := l.errs.handleErrorWithPos(node.GetOutputType().Start(), "%s: invalid response type: %s is %s, not a message", scope, fqn, descriptorTypeWithArticle(dsc)); err != nil {
563589
return err
564590
}
565591
} else {
@@ -663,8 +689,7 @@ func (l *linker) resolveExtensionName(name string, fd *dpb.FileDescriptorProto,
663689
return "", fmt.Errorf("unknown extension %s; resolved to %s which is not defined; consider using a leading dot", name, fqn)
664690
}
665691
if ext, ok := dsc.(*dpb.FieldDescriptorProto); !ok {
666-
otherType := descriptorType(dsc)
667-
return "", fmt.Errorf("invalid extension: %s is a %s, not an extension", name, otherType)
692+
return "", fmt.Errorf("invalid extension: %s is %s, not an extension", name, descriptorTypeWithArticle(dsc))
668693
} else if ext.GetExtendee() == "" {
669694
return "", fmt.Errorf("invalid extension: %s is a field but not an extension", name)
670695
}
@@ -693,7 +718,13 @@ func (l *linker) resolve(fd *dpb.FileDescriptorProto, name string, onlyTypes boo
693718
for i := len(scopes) - 1; i >= 0; i-- {
694719
fqn, d, proto3 := scopes[i](firstName, name)
695720
if d != nil {
696-
if !onlyTypes || isType(d) {
721+
// In `protoc`, it will skip a match of the wrong type and move on
722+
// to the next scope, but only if the reference is unqualified. So
723+
// we mirror that behavior here. When we skip and move on, we go
724+
// ahead and save the match of the wrong type so we can at least use
725+
// it to construct a better error in the event that we don't find
726+
// any match of the right type.
727+
if !onlyTypes || isType(d) || firstName != name {
697728
return fqn, d, proto3
698729
} else if bestGuess == nil {
699730
bestGuess = d

‎desc/protoparse/linker_test.go

+89-11
Original file line numberDiff line numberDiff line change
@@ -137,26 +137,26 @@ func TestLinkerValidation(t *testing.T) {
137137
map[string]string{
138138
"foo.proto": "enum foo { bar = 1; baz = 2; } enum fu { bar = 1; baz = 2; }",
139139
},
140-
`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`,
140+
`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`,
141141
},
142142
{
143143
map[string]string{
144144
"foo.proto": "message foo {} enum foo { V = 0; }",
145145
},
146-
"foo.proto:1:16: duplicate symbol foo: already defined as message",
146+
"foo.proto:1:16: duplicate symbol foo: already defined as a message",
147147
},
148148
{
149149
map[string]string{
150150
"foo.proto": "message foo { optional string a = 1; optional string a = 2; }",
151151
},
152-
"foo.proto:1:38: duplicate symbol foo.a: already defined as field",
152+
"foo.proto:1:38: duplicate symbol foo.a: already defined as a field",
153153
},
154154
{
155155
map[string]string{
156156
"foo.proto": "message foo {}",
157157
"foo2.proto": "enum foo { V = 0; }",
158158
},
159-
"foo2.proto:1:1: duplicate symbol foo: already defined as message in \"foo.proto\"",
159+
"foo2.proto:1:1: duplicate symbol foo: already defined as a message in \"foo.proto\"",
160160
},
161161
{
162162
map[string]string{
@@ -185,7 +185,7 @@ func TestLinkerValidation(t *testing.T) {
185185
message DescriptorProto { }
186186
`,
187187
},
188-
`google/protobuf/descriptor.proto: duplicate symbol google.protobuf.DescriptorProto: already defined as message in "foo.proto"`,
188+
`google/protobuf/descriptor.proto: duplicate symbol google.protobuf.DescriptorProto: already defined as a message in "foo.proto"`,
189189
},
190190
{
191191
map[string]string{
@@ -583,7 +583,7 @@ func TestLinkerValidation(t *testing.T) {
583583
" }\n" +
584584
"}",
585585
},
586-
`a.proto:4:5: duplicate symbol m.z: already defined as oneof`,
586+
`a.proto:4:5: duplicate symbol m.z: already defined as a oneof`,
587587
},
588588
{
589589
map[string]string{
@@ -593,7 +593,7 @@ func TestLinkerValidation(t *testing.T) {
593593
" oneof z{int64 b=2;}\n" +
594594
"}",
595595
},
596-
`a.proto:3:3: duplicate symbol m.z: already defined as oneof`,
596+
`a.proto:3:3: duplicate symbol m.z: already defined as a oneof`,
597597
},
598598
{
599599
map[string]string{
@@ -718,7 +718,7 @@ func TestLinkerValidation(t *testing.T) {
718718
" oneof z{int64 b=2;}\n" +
719719
"}",
720720
},
721-
`a.proto:4:3: duplicate symbol m.z: already defined as oneof`,
721+
`a.proto:4:3: duplicate symbol m.z: already defined as a oneof`,
722722
},
723723
{
724724
map[string]string{
@@ -906,6 +906,84 @@ func TestLinkerValidation(t *testing.T) {
906906
},
907907
"foo.proto:4:26: field foobar: option json_name is not allowed on extensions",
908908
},
909+
{
910+
map[string]string{
911+
"foo.proto": "syntax = \"proto3\";\n" +
912+
"package foo.foo;\n" +
913+
"import \"other.proto\";\n" +
914+
"service Foo { rpc Bar (Baz) returns (Baz); }\n" +
915+
"message Baz {\n" +
916+
" foo.Foo.Bar f = 1;\n" +
917+
"}\n",
918+
"other.proto": "syntax = \"proto3\";\n" +
919+
"package foo;\n" +
920+
"message Foo {\n" +
921+
" enum Bar { ZED = 0; }\n" +
922+
"}\n",
923+
},
924+
"foo.proto:6:3: field foo.foo.Baz.f: invalid type: foo.foo.Foo.Bar is a method, not a message or enum",
925+
},
926+
{
927+
map[string]string{
928+
"foo.proto": "syntax = \"proto3\";\n" +
929+
"import \"google/protobuf/descriptor.proto\";\n" +
930+
"message Foo {\n" +
931+
" enum Bar { ZED = 0; }\n" +
932+
" message Foo {\n" +
933+
" extend google.protobuf.MessageOptions {\n" +
934+
" string Bar = 30000;\n" +
935+
" }\n" +
936+
" Foo.Bar f = 1;\n" +
937+
" }\n" +
938+
"}\n",
939+
},
940+
"foo.proto:9:5: field Foo.Foo.f: invalid type: Foo.Foo.Bar is an extension, not a message or enum",
941+
},
942+
{
943+
map[string]string{
944+
"foo.proto": "syntax = \"proto3\";\n" +
945+
"import \"google/protobuf/descriptor.proto\";\n" +
946+
"extend google.protobuf.ServiceOptions {\n" +
947+
" string Bar = 30000;\n" +
948+
"}\n" +
949+
"message Empty {}\n" +
950+
"service Foo {\n" +
951+
" option (Bar) = \"blah\";\n" +
952+
" rpc Bar (Empty) returns (Empty);\n" +
953+
"}\n",
954+
},
955+
"", // should succeed
956+
},
957+
{
958+
map[string]string{
959+
"foo.proto": "syntax = \"proto3\";\n" +
960+
"import \"google/protobuf/descriptor.proto\";\n" +
961+
"extend google.protobuf.MethodOptions {\n" +
962+
" string Bar = 30000;\n" +
963+
"}\n" +
964+
"message Empty {}\n" +
965+
"service Foo {\n" +
966+
" rpc Bar (Empty) returns (Empty) { option (Bar) = \"blah\"; }\n" +
967+
"}\n",
968+
},
969+
"foo.proto:8:44: method Foo.Bar: invalid extension: Bar is a method, not an extension",
970+
},
971+
{
972+
map[string]string{
973+
"foo.proto": "syntax = \"proto3\";\n" +
974+
"import \"google/protobuf/descriptor.proto\";\n" +
975+
"enum Bar { ZED = 0; }\n" +
976+
"message Foo {\n" +
977+
" extend google.protobuf.MessageOptions {\n" +
978+
" string Bar = 30000;\n" +
979+
" }\n" +
980+
" message Foo {\n" +
981+
" Bar f = 1;\n" +
982+
" }\n" +
983+
"}\n",
984+
},
985+
"", // should succeed
986+
},
909987
}
910988
for i, tc := range testCases {
911989
acc := func(filename string) (io.ReadCloser, error) {
@@ -1070,9 +1148,9 @@ func TestSyntheticOneOfCollisions(t *testing.T) {
10701148
testutil.Eq(t, ErrInvalidSource, err)
10711149

10721150
expected := []string{
1073-
`foo2.proto:2:1: duplicate symbol Foo: already defined as message in "foo1.proto"`,
1074-
`foo2.proto:3:3: duplicate symbol Foo._bar: already defined as oneof in "foo1.proto"`,
1075-
`foo2.proto:3:3: duplicate symbol Foo.bar: already defined as field in "foo1.proto"`,
1151+
`foo2.proto:2:1: duplicate symbol Foo: already defined as a message in "foo1.proto"`,
1152+
`foo2.proto:3:3: duplicate symbol Foo._bar: already defined as a oneof in "foo1.proto"`,
1153+
`foo2.proto:3:3: duplicate symbol Foo.bar: already defined as a field in "foo1.proto"`,
10761154
}
10771155
var actual []string
10781156
for _, err := range errs {

‎desc/protoparse/reporting_test.go

+5-5
Original file line numberDiff line numberDiff line change
@@ -104,9 +104,9 @@ func TestErrorReporting(t *testing.T) {
104104
`,
105105
},
106106
expectedErrs: []string{
107-
"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",
108-
"test.proto:10:41: duplicate symbol Bar: already defined as enum",
109-
"test.proto:12:49: duplicate symbol Bar.Foobar: already defined as method",
107+
"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",
108+
"test.proto:10:41: duplicate symbol Bar: already defined as an enum",
109+
"test.proto:12:49: duplicate symbol Bar.Foobar: already defined as a method",
110110
"test.proto:12:61: method Bar.Foobar: unknown request type Frob",
111111
"test.proto:12:76: method Bar.Foobar: unknown response type Nitz",
112112
},
@@ -168,8 +168,8 @@ func TestErrorReporting(t *testing.T) {
168168
`,
169169
},
170170
expectedErrs: []string{
171-
"test2.proto:4:41: duplicate symbol Bar: already defined as service in \"test1.proto\"",
172-
"test2.proto:7:41: duplicate symbol Foo: already defined as message in \"test1.proto\"",
171+
"test2.proto:4:41: duplicate symbol Bar: already defined as a service in \"test1.proto\"",
172+
"test2.proto:7:41: duplicate symbol Foo: already defined as a message in \"test1.proto\"",
173173
"test1.proto:8:75: method Bar.Frob: unknown response type Nitz",
174174
"test2.proto:8:82: method Foo.DoSomething: invalid response type: Foo is a service, not a message",
175175
},

0 commit comments

Comments
 (0)