Repository: googleapis/api-linter (go)

Issue:
api-linter only prints first build error
Given the following file:

```proto
syntax = "proto3";

package foo;

message One {
  // First build error, no semicolon.
  int64 one = 1
}

message Two {
  // Second build error, no semicolon.
  int64 two = 2
}

```

The following is outputted from `protoc`, `buf`, and `api-linter`:

```
$ protoc -o /dev/null foo.proto
foo.proto:8:1: Expected ";".
foo.proto:13:1: Expected ";".

# also buf image build -o /dev/null
$ buf check lint
foo.proto:8:1:syntax error: unexpected '}', expecting ';' or '['
foo.proto:13:1:syntax error: unexpected '}', expecting ';' or '['

$ api-linter foo.proto
2019/11/07 12:16:45 foo.proto:8:1: syntax error: unexpected '}', expecting ';' or '['
```

The issue is that api-linter is not using https://godoc.org/github.com/jhump/protoreflect/desc/protoparse#ErrorReporter. This means that if a file has multiple build errors, they will only be reported one at a time, which can lead to only fixing single issues at a time and a lot of back-and-forth with editing your files.

It would also be nice if these issues were not printed as log statements, and the messages did not have the prefix " " after the column if possible, but this may not be something that `api-linter` wants to do.

Pull request:
Print multiple build errors
Fixes #314.

Feel free to commandeer this, first time contributing to this repository so not sure if this is how you want tests written etc. This also just combines the errors into a string and prints them out, nothing too fancy here. However it works:

```
$ api-linter cmd/api-linter/internal/testdata/build_errors.proto
2019/11/08 13:17:24 cmd/api-linter/internal/testdata/build_errors.proto:8:1: syntax error: unexpected '}', expecting ';' or '['
cmd/api-linter/internal/testdata/build_errors.proto:13:1: syntax error: unexpected '}', expecting ';' or '['
```

Patch:
diff --git a/cmd/api-linter/cli.go b/cmd/api-linter/cli.go
index 4084b97..ac559f0 100644
--- a/cmd/api-linter/cli.go
+++ b/cmd/api-linter/cli.go
@@ -16,10 +16,12 @@ package main
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"os"
 	"strings"
+	"sync"
 
 	"github.com/golang/protobuf/proto"
 	dpb "github.com/golang/protobuf/protoc-gen-go/descriptor"
@@ -121,11 +123,21 @@ func (c *cli) lint(rules lint.RuleRegistry, configs lint.Configs) error {
 			return nil, fmt.Errorf("%q is not found", name)
 		}
 	}
+	var errorsWithPos []protoparse.ErrorWithPos
+	var lock sync.Mutex
 	// Parse proto files into `protoreflect` file descriptors.
 	p := protoparse.Parser{
 		ImportPaths:           c.ProtoImportPaths,
 		IncludeSourceCodeInfo: true,
 		LookupImport:          lookupImport,
+		ErrorReporter: func(errorWithPos protoparse.ErrorWithPos) error {
+			// Protoparse isn't concurrent right now but just to be safe for the future.
+			lock.Lock()
+			errorsWithPos = append(errorsWithPos, errorWithPos)
+			lock.Unlock()
+			// Continue parsing. The error returned will be protoparse.ErrInvalidSource.
+			return nil
+		},
 	}
 	// Resolve file absolute paths to relative ones.
 	protoFiles, err := protoparse.ResolveFilenames(c.ProtoImportPaths, c.ProtoFiles...)
@@ -134,6 +146,17 @@ func (c *cli) lint(rules lint.RuleRegistry, configs lint.Configs) error {
 	}
 	fd, err := p.ParseFiles(protoFiles...)
 	if err != nil {
+		if err == protoparse.ErrInvalidSource {
+			if len(errorsWithPos) == 0 {
+				return errors.New("got protoparse.ErrInvalidSource but no ErrorWithPos errors")
+			}
+			// TODO: There's multiple ways to deal with this but this prints all the errors at least
+			errStrings := make([]string, len(errorsWithPos))
+			for i, errorWithPos := range errorsWithPos {
+				errStrings[i] = errorWithPos.Error()
+			}
+			return errors.New(strings.Join(errStrings, "\n"))
+		}
 		return err
 	}
 


Test patch:
diff --git a/cmd/api-linter/integration_test.go b/cmd/api-linter/integration_test.go
index 70da604..dd490ba 100644
--- a/cmd/api-linter/integration_test.go
+++ b/cmd/api-linter/integration_test.go
@@ -159,6 +159,19 @@ func TestRules_DisabledByConfig(t *testing.T) {
 	}
 }
 
+func TestBuildErrors(t *testing.T) {
+	expected := `internal/testdata/build_errors.proto:8:1: syntax error: unexpected '}', expecting ';' or '['
+internal/testdata/build_errors.proto:13:1: syntax error: unexpected '}', expecting ';' or '['`
+	err := runCLI([]string{"internal/testdata/build_errors.proto"})
+	if err == nil {
+		t.Fatal("expected build error for build_errors.proto")
+	}
+	actual := err.Error()
+	if expected != actual {
+		t.Fatalf("expected %q, got %q", expected, actual)
+	}
+}
+
 func runLinter(t *testing.T, protoContent, configContent string) string {
 	tempDir, err := ioutil.TempDir("", "test")
 	if err != nil {
diff --git a/cmd/api-linter/internal/testdata/build_errors.proto b/cmd/api-linter/internal/testdata/build_errors.proto
new file mode 100644
index 0000000..1ffdf4d
--- /dev/null
+++ b/cmd/api-linter/internal/testdata/build_errors.proto
@@ -0,0 +1,13 @@
+syntax = "proto3";
+
+package foo;
+
+message Bar1 {
+  // first build error
+  string baz1 = 1
+}
+
+message Bar2 {
+  // second build error
+  string baz2 = 1
+}