Repository: jhump/protoreflect
Pull request #317 (2020-04-27 12:25:54)

protoparse: add WarningReporter; add warnings for missing syntax
Add `WarningReporter` optional field to `Parser`, for collecting warnings that occur during parse operation.

Adds one actual warning message, for when a file does not have a `syntax` declaration.

TODO: use this mechanism to warn about unused imports.


Patch:
diff --git a/desc/protoparse/ast.go b/desc/protoparse/ast.go
index 9175ad9..6980e93 100644
--- a/desc/protoparse/ast.go
+++ b/desc/protoparse/ast.go
@@ -1,5 +1,7 @@
 package protoparse
 
+import "fmt"
+
 // This file defines all of the nodes in the proto AST.
 
 // SourcePos identifies a location in a proto source file.
@@ -9,6 +11,13 @@ type SourcePos struct {
 	Offset    int
 }
 
+func (pos SourcePos) String() string {
+	if pos.Line <= 0 || pos.Col <= 0 {
+		return pos.Filename
+	}
+	return fmt.Sprintf("%s:%d:%d", pos.Filename, pos.Line, pos.Col)
+}
+
 func unknownPos(filename string) *SourcePos {
 	return &SourcePos{Filename: filename}
 }
diff --git a/desc/protoparse/descriptor_protos.go b/desc/protoparse/descriptor_protos.go
index 6721375..79973ca 100644
--- a/desc/protoparse/descriptor_protos.go
+++ b/desc/protoparse/descriptor_protos.go
@@ -31,6 +31,8 @@ func (r *parseResult) createFileDescriptor(filename string, file *fileNode) {
 		if isProto3 {
 			fd.Syntax = proto.String(file.syntax.syntax.val)
 		}
+	} else {
+		r.errs.warn(file.start(), ErrNoSyntax)
 	}
 
 	for _, decl := range file.decls {
diff --git a/desc/protoparse/errors.go b/desc/protoparse/errors.go
index 45d7997..79e2adc 100644
--- a/desc/protoparse/errors.go
+++ b/desc/protoparse/errors.go
@@ -11,29 +11,43 @@ import (
 // always returns nil.
 var ErrInvalidSource = errors.New("parse failed: invalid proto source")
 
+// ErrNoSyntax is a sentinel error that may be passed to a warning reporter.
+// The error the reporter receives will be wrapped with source position that
+// indicates the file that had no syntax statement.
+var ErrNoSyntax = errors.New("no syntax specified; defaulting to proto2 syntax")
+
 // ErrorReporter is responsible for reporting the given error. If the reporter
 // returns a non-nil error, parsing/linking will abort with that error. If the
 // reporter returns nil, parsing will continue, allowing the parser to try to
 // report as many syntax and/or link errors as it can find.
 type ErrorReporter func(err ErrorWithPos) error
 
+// WarningReporter is responsible for reporting the given warning. This is used
+// for indicating non-error messages to the calling program for things that do
+// not cause the parse to fail but are considered bad practice. Though they are
+// just warnings, the details are supplied to the reporter via an error type.
+type WarningReporter func(ErrorWithPos)
+
 func defaultErrorReporter(err ErrorWithPos) error {
 	// abort parsing after first error encountered
 	return err
 }
 
 type errorHandler struct {
-	reporter     ErrorReporter
+	errReporter  ErrorReporter
 	errsReported int
 	err          error
+
+	warnReporter WarningReporter
 }
 
-func newErrorHandler(reporter ErrorReporter) *errorHandler {
-	if reporter == nil {
-		reporter = defaultErrorReporter
+func newErrorHandler(errRep ErrorReporter, warnRep WarningReporter) *errorHandler {
+	if errRep == nil {
+		errRep = defaultErrorReporter
 	}
 	return &errorHandler{
-		reporter: reporter,
+		errReporter:  errRep,
+		warnReporter: warnRep,
 	}
 }
 
@@ -42,7 +56,7 @@ func (h *errorHandler) handleErrorWithPos(pos *SourcePos, format string, args ..
 		return h.err
 	}
 	h.errsReported++
-	err := h.reporter(errorWithPos(pos, format, args...))
+	err := h.errReporter(errorWithPos(pos, format, args...))
 	h.err = err
 	return err
 }
@@ -53,12 +67,18 @@ func (h *errorHandler) handleError(err error) error {
 	}
 	if ewp, ok := err.(ErrorWithPos); ok {
 		h.errsReported++
-		err = h.reporter(ewp)
+		err = h.errReporter(ewp)
 	}
 	h.err = err
 	return err
 }
 
+func (h *errorHandler) warn(pos *SourcePos, err error) {
+	if h.warnReporter != nil {
+		h.warnReporter(ErrorWithSourcePos{Pos: pos, Underlying: err})
+	}
+}
+
 func (h *errorHandler) getError() error {
 	if h.errsReported > 0 && h.err == nil {
 		return ErrInvalidSource
@@ -95,10 +115,7 @@ type ErrorWithSourcePos struct {
 // Error implements the error interface
 func (e ErrorWithSourcePos) Error() string {
 	sourcePos := e.GetPosition()
-	if sourcePos.Line <= 0 || sourcePos.Col <= 0 {
-		return fmt.Sprintf("%s: %v", sourcePos.Filename, e.Underlying)
-	}
-	return fmt.Sprintf("%s:%d:%d: %v", sourcePos.Filename, sourcePos.Line, sourcePos.Col, e.Underlying)
+	return fmt.Sprintf("%s: %v", sourcePos, e.Underlying)
 }
 
 // GetPosition implements the ErrorWithPos interface, supplying a location in
diff --git a/desc/protoparse/lexer.go b/desc/protoparse/lexer.go
index 0a697ba..ce3d2f6 100644
--- a/desc/protoparse/lexer.go
+++ b/desc/protoparse/lexer.go
@@ -60,10 +60,6 @@ type protoLex struct {
 	comments   []comment
 }
 
-func newTestLexer(in io.Reader) *protoLex {
-	return newLexer(in, "test.proto", newErrorHandler(nil))
-}
-
 func newLexer(in io.Reader, filename string, errs *errorHandler) *protoLex {
 	return &protoLex{
 		input:    &runeReader{rr: bufio.NewReader(in)},
diff --git a/desc/protoparse/parser.go b/desc/protoparse/parser.go
index b54c3ee..6025bd0 100644
--- a/desc/protoparse/parser.go
+++ b/desc/protoparse/parser.go
@@ -154,6 +154,9 @@ type Parser struct {
 	// The reporter is not invoked for system or I/O errors, only for syntax and
 	// link errors.
 	ErrorReporter ErrorReporter
+
+	// A custom reporter of warnings. If not specified, warning messages are ignored.
+	WarningReporter WarningReporter
 }
 
 // ParseFiles parses the named files into descriptors. The returned slice has
@@ -200,7 +203,7 @@ func (p Parser) ParseFiles(filenames ...string) ([]*desc.FileDescriptor, error)
 
 	protos := map[string]*parseResult{}
 	results := &parseResults{resultsByFilename: protos}
-	errs := newErrorHandler(p.ErrorReporter)
+	errs := newErrorHandler(p.ErrorReporter, p.WarningReporter)
 	parseProtoFiles(accessor, filenames, errs, true, true, results, p.LookupImport)
 	if err := errs.getError(); err != nil {
 		return nil, err
@@ -268,7 +271,7 @@ func (p Parser) ParseFilesButDoNotLink(filenames ...string) ([]*dpb.FileDescript
 	}
 
 	protos := map[string]*parseResult{}
-	errs := newErrorHandler(p.ErrorReporter)
+	errs := newErrorHandler(p.ErrorReporter, p.WarningReporter)
 	parseProtoFiles(accessor, filenames, errs, false, p.ValidateUnlinkedFiles, &parseResults{resultsByFilename: protos}, p.LookupImport)
 	if err := errs.getError(); err != nil {
 		return nil, err
@@ -285,7 +288,7 @@ func (p Parser) ParseFilesButDoNotLink(filenames ...string) ([]*dpb.FileDescript
 			// parsing options will be best effort
 			pr.lenient = true
 			// we don't want the real error reporter see any errors
-			pr.errs.reporter = func(err ErrorWithPos) error {
+			pr.errs.errReporter = func(err ErrorWithPos) error {
 				return err
 			}
 			_ = interpretFileOptions(pr, poorFileDescriptorish{FileDescriptorProto: fd})


Test patch:
diff --git a/desc/protoparse/lexer_test.go b/desc/protoparse/lexer_test.go
index 25d2b24..f217605 100644
--- a/desc/protoparse/lexer_test.go
+++ b/desc/protoparse/lexer_test.go
@@ -1,6 +1,7 @@
 package protoparse
 
 import (
+	"io"
 	"strings"
 	"testing"
 
@@ -223,3 +224,7 @@ func TestLexerErrors(t *testing.T) {
 		testutil.Require(t, strings.Contains(sym.err.Error(), tc.errMsg), "case %d: expected message to contain %q but does not: %q", i, tc.errMsg, sym.err.Error())
 	}
 }
+
+func newTestLexer(in io.Reader) *protoLex {
+	return newLexer(in, "test.proto", newErrorHandler(nil, nil))
+}
diff --git a/desc/protoparse/parser_test.go b/desc/protoparse/parser_test.go
index 0ac8abc..bbf6e77 100644
--- a/desc/protoparse/parser_test.go
+++ b/desc/protoparse/parser_test.go
@@ -161,7 +161,7 @@ func parseFileForTest(filename string) (*parseResult, error) {
 	defer func() {
 		_ = f.Close()
 	}()
-	errs := newErrorHandler(nil)
+	errs := newErrorHandler(nil, nil)
 	res := parseProto(filename, f, errs, true)
 	return res, errs.getError()
 }
diff --git a/desc/protoparse/error_reporting_test.go b/desc/protoparse/reporting_test.go
similarity index 86%
rename from desc/protoparse/error_reporting_test.go
rename to desc/protoparse/reporting_test.go
index f6853bd..b3cfd87 100644
--- a/desc/protoparse/error_reporting_test.go
+++ b/desc/protoparse/reporting_test.go
@@ -2,6 +2,7 @@ package protoparse
 
 import (
 	"errors"
+	"fmt"
 	"strings"
 	"testing"
 
@@ -220,3 +221,52 @@ func TestErrorReporting(t *testing.T) {
 		}
 	}
 }
+
+func TestWarningReporting(t *testing.T) {
+	type msg struct {
+		pos  SourcePos
+		text string
+	}
+	var msgs []msg
+	rep := func(warn ErrorWithPos) {
+		msgs = append(msgs, msg{
+			pos: warn.GetPosition(), text: warn.Unwrap().Error(),
+		})
+	}
+
+	testCases := []struct {
+		source          string
+		expectedNotices []string
+	}{
+		{
+			source: `syntax = "proto2"; message Foo {}`,
+		},
+		{
+			source: `syntax = "proto3"; message Foo {}`,
+		},
+		{
+			source: `message Foo {}`,
+			expectedNotices: []string{
+				"test.proto:1:1: no syntax specified; defaulting to proto2 syntax",
+			},
+		},
+	}
+	for _, testCase := range testCases {
+		accessor := FileContentsFromMap(map[string]string{
+			"test.proto": testCase.source,
+		})
+		p := Parser{
+			Accessor:        accessor,
+			WarningReporter: rep,
+		}
+		msgs = nil
+		_, err := p.ParseFiles("test.proto")
+		testutil.Ok(t, err)
+
+		actualNotices := make([]string, len(msgs))
+		for j, msg := range msgs {
+			actualNotices[j] = fmt.Sprintf("%s: %s", msg.pos, msg.text)
+		}
+		testutil.Eq(t, testCase.expectedNotices, actualNotices)
+	}
+}
diff --git a/desc/protoparse/validate_test.go b/desc/protoparse/validate_test.go
index d8d267b..b8b03a8 100644
--- a/desc/protoparse/validate_test.go
+++ b/desc/protoparse/validate_test.go
@@ -243,7 +243,7 @@ func TestBasicValidation(t *testing.T) {
 	}
 
 	for i, tc := range testCases {
-		errs := newErrorHandler(nil)
+		errs := newErrorHandler(nil, nil)
 		_ = parseProto("test.proto", strings.NewReader(tc.contents), errs, true)
 		err := errs.getError()
 		if tc.succeeds {