Repository: jhump/protoreflect (go)

Issue:
C++ scoping rules for enums are not enforced by protoparse
```proto
// foo.proto

// This happens regardless of whether syntax is proto2 or proto3.
syntax = "proto3";

package foo;

message Bar1 {
  enum Baz1 {
    ZERO = 0;
    ONE = 1;
  }
  enum Baz2 {
    ZERO = 0;
    ONE = 1;
  }
}

enum Baz1 {
  ZERO = 0;
  ONE = 1;
}

enum Baz2 {
  ZERO = 0;
  ONE = 1;
}
```

With `buf`:

```
$ buf build
# no error
```

With `protoc`:

```
$ protoc -o /dev/null foo.proto
foo.proto:14:5: "ZERO" is already defined in "foo.Bar1".
foo.proto:14:5: Note that enum values use C++ scoping rules, meaning that enum values are siblings of their type, not children of it.  Therefore, "ZERO" must be unique within "foo.Bar1", not just within "Baz2".
foo.proto:15:5: "ONE" is already defined in "foo.Bar1".
foo.proto:15:5: Note that enum values use C++ scoping rules, meaning that enum values are siblings of their type, not children of it.  Therefore, "ONE" must be unique within "foo.Bar1", not just within "Baz2".
foo.proto:25:3: "ZERO" is already defined in "foo".
foo.proto:25:3: Note that enum values use C++ scoping rules, meaning that enum values are siblings of their type, not children of it.  Therefore, "ZERO" must be unique within "foo", not just within "Baz2".
foo.proto:26:3: "ONE" is already defined in "foo".
foo.proto:26:3: Note that enum values use C++ scoping rules, meaning that enum values are siblings of their type, not children of it.  Therefore, "ONE" must be unique within "foo", not just within "Baz2".
```

I verified this happens before the most recent scoping changes as well, so this is independent of that.

Pull request:
protoparse: c++ scoping rules for enum values
Fixes #400.

Patch:
diff --git a/desc/protoparse/linker.go b/desc/protoparse/linker.go
index 45893e2..ab72933 100644
--- a/desc/protoparse/linker.go
+++ b/desc/protoparse/linker.go
@@ -213,7 +213,10 @@ func addEnumToPool(r *parseResult, pool map[string]proto.Message, errs *errorHan
 		return err
 	}
 	for _, evd := range ed.Value {
-		vfqn := fqn + "." + evd.GetName()
+		// protobuf name-scoping rules for enum values follow C++ scoping rules:
+		// the enum value name is a symbol in the *parent* scope (the one
+		// enclosing the enum).
+		vfqn := prefix + evd.GetName()
 		if err := addToPool(r, pool, errs, vfqn, evd); err != nil {
 			return err
 		}
@@ -238,7 +241,16 @@ func addServiceToPool(r *parseResult, pool map[string]proto.Message, errs *error
 func addToPool(r *parseResult, pool map[string]proto.Message, errs *errorHandler, fqn string, dsc proto.Message) error {
 	if d, ok := pool[fqn]; ok {
 		node := r.nodes[dsc]
-		if err := errs.handleErrorWithPos(node.Start(), "duplicate symbol %s: already defined as %s", fqn, descriptorType(d)); err != nil {
+		_, additionIsEnumVal := dsc.(*dpb.EnumValueDescriptorProto)
+		_, existingIsEnumVal := d.(*dpb.EnumValueDescriptorProto)
+		// because of weird scoping for enum values, provide more context in error message
+		// if this conflict is with an enum value
+		var suffix string
+		if additionIsEnumVal || existingIsEnumVal {
+			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 {
 			return err
 		}
 	}


Test patch:
diff --git a/desc/protoparse/linker_test.go b/desc/protoparse/linker_test.go
index 0773cb8..b9ab970 100644
--- a/desc/protoparse/linker_test.go
+++ b/desc/protoparse/linker_test.go
@@ -111,6 +111,12 @@ func TestLinkerValidation(t *testing.T) {
 			},
 			`foo.proto:1:8: cycle found in imports: "foo.proto" -> "foo2.proto" -> "foo.proto"`,
 		},
+		{
+			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`,
+		},
 		{
 			map[string]string{
 				"foo.proto": "message foo {} enum foo { V = 0; }",
diff --git a/desc/protoparse/reporting_test.go b/desc/protoparse/reporting_test.go
index 4c8b95b..f8a870c 100644
--- a/desc/protoparse/reporting_test.go
+++ b/desc/protoparse/reporting_test.go
@@ -104,7 +104,7 @@ func TestErrorReporting(t *testing.T) {
 					`,
 			},
 			expectedErrs: []string{
-				"test.proto:8:49: duplicate symbol Bar.BAZ: already defined as enum value",
+				"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.Foo: already defined as method",
 				"test.proto:12:58: method Bar.Foo: unknown request type Frob",