Repository: jhump/protoreflect
Pull request #520 (2022-07-26 18:52:24)

protoparse: fix scoping issue for extendees and field type names; also fix articles in error messages
There was a bug in the scoping rules, related to how relative references to type names in `extends` blocks and field types are resolved.

I've added more tests, which assert similar behavior as `protoc`. This required a one-line logic change. But while in here and editing tests, I noticed use of the wrong article in error messages, e.g. "a extension" instead of "an extension". So I figured I'd fix that, too. (Sadly, more of the changes in this branch are for the article fix than the scope fix.)

jhump: @amckinney, this is the scope issue I mentioned to you.

Patch:
diff --git a/desc/protoparse/linker.go b/desc/protoparse/linker.go
index 30e306f..1fea1c9 100644
--- a/desc/protoparse/linker.go
+++ b/desc/protoparse/linker.go
@@ -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
 				}
 			}
@@ -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
 		}
 	}
@@ -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{}{}
@@ -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
@@ -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
@@ -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
 }
@@ -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 {
@@ -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 {
@@ -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)
 	}
@@ -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


Test patch:
diff --git a/desc/protoparse/linker_test.go b/desc/protoparse/linker_test.go
index 339cb42..9b3935b 100644
--- a/desc/protoparse/linker_test.go
+++ b/desc/protoparse/linker_test.go
@@ -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{
@@ -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{
@@ -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{
@@ -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{
@@ -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{
@@ -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) {
@@ -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 {
diff --git a/desc/protoparse/reporting_test.go b/desc/protoparse/reporting_test.go
index a952e42..459cd83 100644
--- a/desc/protoparse/reporting_test.go
+++ b/desc/protoparse/reporting_test.go
@@ -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",
 			},
@@ -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",
 			},