verify that reserved names are valid identifiers (#65)

bufbuild · Oct 13, 2022 · e606fac · e606fac
1 parent 94b7e85
commit e606fac
Show file tree

Hide file tree

Showing 2 changed files with 152 additions and 0 deletions.
diff --git a/parser/validate.go b/parser/validate.go
@@ -203,6 +203,14 @@ func validateMessage(res *result, isProto3 bool, name protoreflect.FullName, md
 	// or reserved ranges or reserved names
 	rsvdNames := map[string]struct{}{}
 	for _, n := range md.ReservedName {
+		// validate reserved name while we're here
+		if !isIdentifier(n) {
+			node := findMessageReservedNameNode(res.MessageNode(md), n)
+			nodeInfo := res.file.NodeInfo(node)
+			if err := handler.HandleErrorf(nodeInfo.Start(), "%s: reserved name %q is not a valid identifier", scope, n); err != nil {
+				return err
+			}
+		}
 		rsvdNames[n] = struct{}{}
 	}
 	fieldTags := map[int32]string{}
@@ -242,6 +250,60 @@ func validateMessage(res *result, isProto3 bool, name protoreflect.FullName, md
 	return nil
 }
 
+func isIdentifier(s string) bool {
+	if len(s) == 0 {
+		return false
+	}
+	for i, r := range s {
+		if i == 0 && r >= '0' && r <= '9' {
+			// can't start with number
+			return false
+		}
+		// alphanumeric and underscore ok; everything else bad
+		switch {
+		case r >= '0' && r <= '9':
+		case r >= 'a' && r <= 'z':
+		case r >= 'A' && r <= 'Z':
+		case r == '_':
+		default:
+			return false
+		}
+	}
+	return true
+}
+
+func findMessageReservedNameNode(msgNode ast.MessageDeclNode, name string) ast.Node {
+	var decls []ast.MessageElement
+	switch msgNode := msgNode.(type) {
+	case *ast.MessageNode:
+		decls = msgNode.Decls
+	case *ast.GroupNode:
+		decls = msgNode.Decls
+	default:
+		// leave decls empty
+	}
+	return findReservedNameNode(msgNode, decls, name)
+}
+
+func findReservedNameNode[T ast.Node](parent ast.Node, decls []T, name string) ast.Node {
+	for _, decl := range decls {
+		// NB: We have to convert to empty interface first, before we can do a type
+		// assertion because type assertions on type parameters aren't allowed. (The
+		// compiler cannot yet know whether T is an interface type or not.)
+		rsvd, ok := any(decl).(*ast.ReservedNode)
+		if !ok {
+			continue
+		}
+		for _, rsvdName := range rsvd.Names {
+			if rsvdName.AsString() == name {
+				return rsvdName
+			}
+		}
+	}
+	// couldn't find it? Instead of puking, report position of the parent.
+	return parent
+}
+
 func validateEnum(res *result, isProto3 bool, name protoreflect.FullName, ed *descriptorpb.EnumDescriptorProto, handler *reporter.Handler) error {
 	scope := fmt.Sprintf("enum %s", name)
 
@@ -331,6 +393,14 @@ func validateEnum(res *result, isProto3 bool, name protoreflect.FullName, ed *de
 	// or reserved ranges or reserved names
 	rsvdNames := map[string]struct{}{}
 	for _, n := range ed.ReservedName {
+		// validate reserved name while we're here
+		if !isIdentifier(n) {
+			node := findEnumReservedNameNode(res.EnumNode(ed), n)
+			nodeInfo := res.file.NodeInfo(node)
+			if err := handler.HandleErrorf(nodeInfo.Start(), "%s: reserved name %q is not a valid identifier", scope, n); err != nil {
+				return err
+			}
+		}
 		rsvdNames[n] = struct{}{}
 	}
 	for _, ev := range ed.Value {
@@ -354,6 +424,15 @@ func validateEnum(res *result, isProto3 bool, name protoreflect.FullName, ed *de
 	return nil
 }
 
+func findEnumReservedNameNode(enumNode ast.Node, name string) ast.Node {
+	var decls []ast.EnumElement
+	if enumNode, ok := enumNode.(*ast.EnumNode); ok {
+		decls = enumNode.Decls
+		// if not the right type, we leave decls empty
+	}
+	return findReservedNameNode(enumNode, decls, name)
+}
+
 func validateField(res *result, isProto3 bool, name protoreflect.FullName, fld *descriptorpb.FieldDescriptorProto, handler *reporter.Handler) error {
 	scope := fmt.Sprintf("field %s", name)
 

diff --git a/parser/validate_test.go b/parser/validate_test.go
@@ -512,6 +512,79 @@ func TestBasicValidation(t *testing.T) {
 					   } } } }`,
 			expectedErr: `test.proto:10:55: message nesting depth must be less than 32`,
 		},
+		"failure_message_invalid_reserved_name": {
+			contents: `syntax = "proto3";
+					   message Foo {
+					     reserved "foo", "b_a_r9", " blah ";
+					   }`,
+			expectedErr: `test.proto:3:72: message Foo: reserved name " blah " is not a valid identifier`,
+		},
+		"failure_message_invalid_reserved_name2": {
+			contents: `syntax = "proto3";
+					   message Foo {
+					     reserved "foo", "_bar123", "123";
+					   }`,
+			expectedErr: `test.proto:3:73: message Foo: reserved name "123" is not a valid identifier`,
+		},
+		"failure_message_invalid_reserved_name3": {
+			contents: `syntax = "proto3";
+					   message Foo {
+					     reserved "foo" "_bar123" "@y!!";
+					   }`,
+			expectedErr: `test.proto:3:55: message Foo: reserved name "foo_bar123@y!!" is not a valid identifier`,
+		},
+		"failure_message_invalid_reserved_name4": {
+			contents: `syntax = "proto3";
+					   message Foo {
+					     reserved "";
+					   }`,
+			expectedErr: `test.proto:3:55: message Foo: reserved name "" is not a valid identifier`,
+		},
+		"success_message_reserved_name": {
+			contents: `syntax = "proto3";
+					   message Foo {
+					     reserved "foo", "_bar123", "A_B_C_1_2_3";
+					   }`,
+		},
+		"failure_enum_invalid_reserved_name": {
+			contents: `syntax = "proto3";
+					   enum Foo {
+					     BAR = 0;
+					     reserved "foo", "b_a_r9", " blah ";
+					   }`,
+			expectedErr: `test.proto:4:72: enum Foo: reserved name " blah " is not a valid identifier`,
+		},
+		"failure_enum_invalid_reserved_name2": {
+			contents: `syntax = "proto3";
+					   enum Foo {
+					     BAR = 0;
+					     reserved "foo", "_bar123", "123";
+					   }`,
+			expectedErr: `test.proto:4:73: enum Foo: reserved name "123" is not a valid identifier`,
+		},
+		"failure_enum_invalid_reserved_name3": {
+			contents: `syntax = "proto3";
+					   enum Foo {
+					     BAR = 0;
+					     reserved "foo" "_bar123" "@y!!";
+					   }`,
+			expectedErr: `test.proto:4:55: enum Foo: reserved name "foo_bar123@y!!" is not a valid identifier`,
+		},
+		"failure_enum_invalid_reserved_name4": {
+			contents: `syntax = "proto3";
+					   enum Foo {
+					     BAR = 0;
+					     reserved "";
+					   }`,
+			expectedErr: `test.proto:4:55: enum Foo: reserved name "" is not a valid identifier`,
+		},
+		"success_enum_reserved_name": {
+			contents: `syntax = "proto3";
+					   enum Foo {
+					     BAR = 0;
+					     reserved "foo", "_bar123", "A_B_C_1_2_3";
+					   }`,
+		},
 	}
 
 	for name, tc := range testCases {