Repository: jhump/protoreflect (go)

Issue:
protoparse: allows option allow_alias = true in enums that have no aliases
This is a case where protoparse will accept a proto source file that protoc would reject.

protoc does not allow the use of `option allow_alias = true` in enums unless the values actually include an alias (e.g. two values with same number).

For example:

```proto
// test.proto
syntax = "proto3";
enum Foo {
  option allow_alias = true;
  BAR = 0;
}
```
This yields the following error with protoc:
```
test.proto:7:1: "Foo" declares support for enum aliases but no enum values share field numbers. Please remove the unnecessary 'option allow_alias = true;' declaration.
```

Pull request:
protoparse: do not allow allow_alias enum option if the enum does not actually have aliases
This resolves #357.

Patch:
diff --git a/desc/protoparse/validate.go b/desc/protoparse/validate.go
index bf97b8f..353af6b 100644
--- a/desc/protoparse/validate.go
+++ b/desc/protoparse/validate.go
@@ -200,21 +200,22 @@ func validateEnum(res *parseResult, isProto3 bool, prefix string, ed *dpb.EnumDe
 	}
 
 	allowAlias := false
+	var allowAliasOpt *dpb.UninterpretedOption
 	if index, err := findOption(res, scope, ed.Options.GetUninterpretedOption(), "allow_alias"); err != nil {
 		return err
 	} else if index >= 0 {
-		opt := ed.Options.UninterpretedOption[index]
+		allowAliasOpt = ed.Options.UninterpretedOption[index]
 		valid := false
-		if opt.IdentifierValue != nil {
-			if opt.GetIdentifierValue() == "true" {
+		if allowAliasOpt.IdentifierValue != nil {
+			if allowAliasOpt.GetIdentifierValue() == "true" {
 				allowAlias = true
 				valid = true
-			} else if opt.GetIdentifierValue() == "false" {
+			} else if allowAliasOpt.GetIdentifierValue() == "false" {
 				valid = true
 			}
 		}
 		if !valid {
-			optNode := res.getOptionNode(opt)
+			optNode := res.getOptionNode(allowAliasOpt)
 			if err := res.errs.handleErrorWithPos(optNode.GetValue().Start(), "%s: expecting bool value for allow_alias option", scope); err != nil {
 				return err
 			}
@@ -228,17 +229,27 @@ func validateEnum(res *parseResult, isProto3 bool, prefix string, ed *dpb.EnumDe
 		}
 	}
 
-	if !allowAlias {
-		// make sure all value numbers are distinct
-		vals := map[int32]string{}
-		for _, evd := range ed.Value {
-			if existing := vals[evd.GetNumber()]; existing != "" {
+	// check for aliases
+	vals := map[int32]string{}
+	hasAlias := false
+	for _, evd := range ed.Value {
+		existing := vals[evd.GetNumber()]
+		if existing != "" {
+			if allowAlias {
+				hasAlias = true
+			} else {
 				evNode := res.getEnumValueNode(evd)
 				if err := res.errs.handleErrorWithPos(evNode.GetNumber().Start(), "%s: values %s and %s both have the same numeric value %d; use allow_alias option if intentional", scope, existing, evd.GetName(), evd.GetNumber()); err != nil {
 					return err
 				}
 			}
-			vals[evd.GetNumber()] = evd.GetName()
+		}
+		vals[evd.GetNumber()] = evd.GetName()
+	}
+	if allowAlias && !hasAlias {
+		optNode := res.getOptionNode(allowAliasOpt)
+		if err := res.errs.handleErrorWithPos(optNode.GetValue().Start(), "%s: allow_alias is true but no values are aliases", scope); err != nil {
+			return err
 		}
 	}
 


Test patch:
diff --git a/desc/protoparse/options_test.go b/desc/protoparse/options_test.go
index 19f5d68..d2c1b4e 100644
--- a/desc/protoparse/options_test.go
+++ b/desc/protoparse/options_test.go
@@ -85,7 +85,7 @@ func TestOptionsInUnlinkedFiles(t *testing.T) {
 		},
 		{
 			// enum options
-			contents: `enum Test { option allow_alias = true; option deprecated = true; option (must.link) = 123.456; ZERO = 0; }`,
+			contents: `enum Test { option allow_alias = true; option deprecated = true; option (must.link) = 123.456; ZERO = 0; ZILCH = 0; }`,
 			uninterpreted: map[string]interface{}{
 				"Test:(must.link)": 123.456,
 			},
diff --git a/desc/protoparse/validate_test.go b/desc/protoparse/validate_test.go
index bd5c06b..872a8d5 100644
--- a/desc/protoparse/validate_test.go
+++ b/desc/protoparse/validate_test.go
@@ -150,6 +150,14 @@ func TestBasicValidation(t *testing.T) {
 			contents: `enum Foo { option allow_alias = true; V1 = 1; V2 = 1; }`,
 			succeeds: true,
 		},
+		{
+			contents: `enum Foo { option allow_alias = false; V1 = 1; V2 = 2; }`,
+			succeeds: true,
+		},
+		{
+			contents: `enum Foo { option allow_alias = true; V1 = 1; V2 = 2; }`,
+			errMsg:   `test.proto:1:33: enum Foo: allow_alias is true but no values are aliases`,
+		},
 		{
 			contents: `syntax = "proto3"; enum Foo { V1 = 0; reserved 1 to 20; reserved "V2"; }`,
 			succeeds: true,