Repository: jhump/protoreflect (go)

Issue:
protoparse: ParseFiles allows duplicate import statements
protoc does not allow duplicate import statements but ParseFiles does.

```protobuf
syntax = "proto3";

import "google/protobuf/descriptor.proto";
import "google/protobuf/descriptor.proto";

// protoc error:
// a.proto:4:1: Import "google/protobuf/descriptor.proto" was listed twice.
```


Pull request:
protoparse: reject duplicate imports
fixes #456 

This makes ParseFiles reject files with duplicate import statements. ~~The error returned doesn't have position data for the import statement because I didn't immediately find where to get a dependency's node~~.

```protobuf
syntax = "proto3";

import "google/protobuf/descriptor.proto";
import "google/protobuf/descriptor.proto";

// protoc error:
// a.proto:4:1: Import "google/protobuf/descriptor.proto" was listed twice.

// protoparse error:
// a.proto:4:1: "google/protobuf/descriptor.proto" was already imported at a.proto:3:1
```


Patch:
diff --git a/desc/protoparse/lexer.go b/desc/protoparse/lexer.go
index 155126c..482e6e3 100644
--- a/desc/protoparse/lexer.go
+++ b/desc/protoparse/lexer.go
@@ -289,7 +289,7 @@ func (l *protoLex) Lex(lval *protoSymType) int {
 			}
 			// integer! (decimal or octal)
 			base := 10
-			if token[0] == '0' && len(token) > 1 && token[1] >= '0' && token[1] <= '7' {
+			if token[0] == '0' {
 				base = 8
 			}
 			ui, err := strconv.ParseUint(token, base, 64)
diff --git a/desc/protoparse/linker.go b/desc/protoparse/linker.go
index 4b4c1d8..4a1a4a5 100644
--- a/desc/protoparse/linker.go
+++ b/desc/protoparse/linker.go
@@ -180,6 +180,11 @@ func addMessageToPool(r *parseResult, pool map[string]proto.Message, errs *error
 		return err
 	}
 	prefix = fqn + "."
+	for _, ood := range md.OneofDecl {
+		if err := addOneofToPool(r, pool, errs, prefix, ood); err != nil {
+			return err
+		}
+	}
 	for _, fld := range md.Field {
 		if err := addFieldToPool(r, pool, errs, prefix, fld); err != nil {
 			return err
@@ -208,6 +213,11 @@ func addFieldToPool(r *parseResult, pool map[string]proto.Message, errs *errorHa
 	return addToPool(r, pool, errs, fqn, fld)
 }
 
+func addOneofToPool(r *parseResult, pool map[string]proto.Message, errs *errorHandler, prefix string, ood *dpb.OneofDescriptorProto) error {
+	fqn := prefix + ood.GetName()
+	return addToPool(r, pool, errs, fqn, ood)
+}
+
 func addEnumToPool(r *parseResult, pool map[string]proto.Message, errs *errorHandler, prefix string, ed *dpb.EnumDescriptorProto) error {
 	fqn := prefix + ed.GetName()
 	if err := addToPool(r, pool, errs, fqn, ed); err != nil {
@@ -281,6 +291,8 @@ func descriptorType(m proto.Message) string {
 		return "method"
 	case *dpb.FileDescriptorProto:
 		return "file"
+	case *dpb.OneofDescriptorProto:
+		return "oneof"
 	default:
 		// shouldn't be possible
 		return fmt.Sprintf("%T", m)
diff --git a/desc/protoparse/validate.go b/desc/protoparse/validate.go
index 353af6b..2dd9161 100644
--- a/desc/protoparse/validate.go
+++ b/desc/protoparse/validate.go
@@ -14,6 +14,10 @@ func validateBasic(res *parseResult, containsErrors bool) {
 	fd := res.fd
 	isProto3 := fd.GetSyntax() == "proto3"
 
+	if validateImports(res) != nil {
+		return
+	}
+
 	for _, md := range fd.MessageType {
 		if validateMessage(res, isProto3, "", md, containsErrors) != nil {
 			return
@@ -33,6 +37,27 @@ func validateBasic(res *parseResult, containsErrors bool) {
 	}
 }
 
+func validateImports(res *parseResult) error {
+	fileNode := res.root
+	if fileNode == nil {
+		return nil
+	}
+	imports := make(map[string]*ast.SourcePos, len(fileNode.Decls))
+	for _, decl := range fileNode.Decls {
+		imp, ok := decl.(*ast.ImportNode)
+		if !ok {
+			continue
+		}
+
+		name := imp.Name.AsString()
+		if imports[name] != nil {
+			return res.errs.handleErrorWithPos(imp.Start(), `%q was already imported at %v`, name, imports[name])
+		}
+		imports[name] = imp.Start()
+	}
+	return nil
+}
+
 func validateMessage(res *parseResult, isProto3 bool, prefix string, md *dpb.DescriptorProto, containsErrors bool) error {
 	nextPrefix := md.GetName() + "."
 


Test patch:
diff --git a/desc/protoparse/lexer_test.go b/desc/protoparse/lexer_test.go
index fdf8538..5f67241 100644
--- a/desc/protoparse/lexer_test.go
+++ b/desc/protoparse/lexer_test.go
@@ -261,6 +261,8 @@ func TestLexerErrors(t *testing.T) {
 		{str: "\U0010FFFF", errMsg: "invalid character"},
 		{str: "// foo \x00", errMsg: "invalid control character"},
 		{str: "/* foo \x00", errMsg: "invalid control character"},
+		{str: "09", errMsg: "invalid syntax in octal integer value: 09"},
+		{str: "0f", errMsg: "invalid syntax in octal integer value: 0f"},
 	}
 	for i, tc := range testCases {
 		l := newTestLexer(strings.NewReader(tc.str))
diff --git a/desc/protoparse/linker_test.go b/desc/protoparse/linker_test.go
index 657ac26..3324acd 100644
--- a/desc/protoparse/linker_test.go
+++ b/desc/protoparse/linker_test.go
@@ -535,6 +535,35 @@ func TestLinkerValidation(t *testing.T) {
 			},
 			`foo.proto:7:16: message Baz: option (foo).buzz: oneof "bar" already has field "baz" set`,
 		},
+		{
+			map[string]string{
+				"a.proto": "syntax = \"proto3\";\n" +
+					"message m{\n" +
+					"  oneof z{\n" +
+					"    int64 z=1;\n" +
+					"  }\n" +
+					"}",
+			},
+			`a.proto:4:5: duplicate symbol m.z: already defined as oneof`,
+		},
+		{
+			map[string]string{
+				"a.proto": "syntax=\"proto3\";\nmessage m{\n" +
+					"  string z = 1;\n" +
+					"  oneof z{int64 b=2;}\n" +
+					"}",
+			},
+			`a.proto:3:3: duplicate symbol m.z: already defined as oneof`,
+		},
+		{
+			map[string]string{
+				"a.proto": "syntax=\"proto3\";\nmessage m{\n" +
+					"  oneof z{int64 a=1;}\n" +
+					"  oneof z{int64 b=2;}\n" +
+					"}",
+			},
+			`a.proto:4:3: duplicate symbol m.z: already defined as oneof`,
+		},
 		{
 			map[string]string{
 				"foo.proto": "syntax = \"proto3\";\n" +
diff --git a/desc/protoparse/validate_test.go b/desc/protoparse/validate_test.go
index ac515af..dfc8595 100644
--- a/desc/protoparse/validate_test.go
+++ b/desc/protoparse/validate_test.go
@@ -382,6 +382,10 @@ func TestBasicValidation(t *testing.T) {
 			contents: `option (opt) = {m []};`,
 			succeeds: true,
 		},
+		{
+			contents: `syntax = "proto3"; import "google/protobuf/descriptor.proto"; import "google/protobuf/descriptor.proto";`,
+			errMsg:   `test.proto:1:63: "google/protobuf/descriptor.proto" was already imported at test.proto:1:20`,
+		},
 	}
 
 	for i, tc := range testCases {
diff --git a/internal/testprotos/make_protos.sh b/internal/testprotos/make_protos.sh
index b66d229..66b4a95 100755
--- a/internal/testprotos/make_protos.sh
+++ b/internal/testprotos/make_protos.sh
@@ -15,6 +15,12 @@ case "${PROTOC_OS}" in
     exit 1
 esac
 
+# This is for macs with M1 chips. Precompiled binaries for osx/amd64 are not available for download, so for that case
+# we download the x86_64 version instead. This will work as long as rosetta2 is installed.
+if [ "$PROTOC_OS" = "osx" ] && [ "$PROTOC_ARCH" = "arm64" ]; then
+  PROTOC_ARCH="x86_64"
+fi
+
 PROTOC="./protoc/bin/protoc"
 
 if [[ "$(${PROTOC} --version 2>/dev/null)" != "libprotoc ${PROTOC_VERSION}" ]]; then