Repository: jhump/protoreflect (go)

Issue:
protoparse: Comments dropped if a leading period is used in the field type.
I got a bizarre bug report (googleapis/api-linter#407) this morning:

```proto
import "google/protobuf/timestamp.proto";

message Foo {
  // This is a comment.
  google.protobuf.Timestamp create_time = 1;

  // This is also a comment.
  .google.protobuf.Timestamp update_time = 2;
}
```

The first comment is preserved, the second one is lost. Removing the leading period fixes the issue. I was able to independently verify this in my own testing, also.

Pull request:
fix comments for fields that start with a dot
Proto3 optional fields with a fully-qualified type name would start with a dot. Due to how comments were handled for punctuation (which a dot was considered to be), the leading comments would mistakenly be attributed as either detached comments or as a trailing comment for the prior element (which might not even be represented in the protobuf descriptor model, such as the opening `{` for a message body, and thus completely inaccessible).

This fix is kind of gross, but it works. The whole way comments for punctuation are treated is kind of a gross hack, so this just puts a band-aid on it :/

Fixes #293 

Patch:
diff --git a/desc/protoparse/lexer.go b/desc/protoparse/lexer.go
index b0b5f5b..6cf2f06 100644
--- a/desc/protoparse/lexer.go
+++ b/desc/protoparse/lexer.go
@@ -199,7 +199,7 @@ func (l *protoLex) Lex(lval *protoSymType) int {
 			// decimal literals could start with a dot
 			cn, _, err := l.input.readRune()
 			if err != nil {
-				l.setRune(lval)
+				l.setDot(lval)
 				return int(c)
 			}
 			if cn >= '0' && cn <= '9' {
@@ -215,7 +215,7 @@ func (l *protoLex) Lex(lval *protoSymType) int {
 				return _FLOAT_LIT
 			}
 			l.input.unreadRune(cn)
-			l.setRune(lval)
+			l.setDot(lval)
 			return int(c)
 		}
 
@@ -360,14 +360,22 @@ func (l *protoLex) newBasicNode() basicNode {
 	}
 }
 
-func (l *protoLex) setPrev(n terminalNode) {
+func (l *protoLex) setPrev(n terminalNode, isDot bool) {
 	nStart := n.start().Line
 	if _, ok := n.(*basicNode); ok {
-		// if the node is a simple rune, don't attribute comments to it
-		// HACK: adjusting the start line makes leading comments appear
-		// detached so logic below will naturally associated trailing
-		// comment to previous symbol
-		nStart += 2
+		// This is really gross, but there are many cases where we don't want
+		// to attribute comments to punctuation (like commas, equals, semicolons)
+		// and would instead prefer to attribute comments to a more meaningful
+		// element in the AST.
+		//
+		// So if it's a simple node OTHER THAN PERIOD (since that is not just
+		// punctuation but typically part of a qualified identifier), don't
+		// attribute comments to it. We do that with this TOTAL HACK: adjusting
+		// the start line makes leading comments appear detached so logic below
+		// will naturally associated trailing comment to previous symbol
+		if !isDot {
+			nStart += 2
+		}
 	}
 	if l.prevSym != nil && len(n.leadingComments()) > 0 && l.prevSym.end().Line < nStart {
 		// we may need to re-attribute the first comment to
@@ -431,28 +439,34 @@ func (l *protoLex) setPrev(n terminalNode) {
 
 func (l *protoLex) setString(lval *protoSymType, val string) {
 	lval.s = &stringLiteralNode{basicNode: l.newBasicNode(), val: val}
-	l.setPrev(lval.s)
+	l.setPrev(lval.s, false)
 }
 
 func (l *protoLex) setIdent(lval *protoSymType, val string) {
 	lval.id = &identNode{basicNode: l.newBasicNode(), val: val}
-	l.setPrev(lval.id)
+	l.setPrev(lval.id, false)
 }
 
 func (l *protoLex) setInt(lval *protoSymType, val uint64) {
 	lval.i = &intLiteralNode{basicNode: l.newBasicNode(), val: val}
-	l.setPrev(lval.i)
+	l.setPrev(lval.i, false)
 }
 
 func (l *protoLex) setFloat(lval *protoSymType, val float64) {
 	lval.f = &floatLiteralNode{basicNode: l.newBasicNode(), val: val}
-	l.setPrev(lval.f)
+	l.setPrev(lval.f, false)
 }
 
 func (l *protoLex) setRune(lval *protoSymType) {
 	b := l.newBasicNode()
 	lval.b = &b
-	l.setPrev(lval.b)
+	l.setPrev(lval.b, false)
+}
+
+func (l *protoLex) setDot(lval *protoSymType) {
+	b := l.newBasicNode()
+	lval.b = &b
+	l.setPrev(lval.b, true)
 }
 
 func (l *protoLex) setError(lval *protoSymType, err error) {


Test patch:
diff --git a/desc/protoparse/parser_test.go b/desc/protoparse/parser_test.go
index 8334924..5f310e3 100644
--- a/desc/protoparse/parser_test.go
+++ b/desc/protoparse/parser_test.go
@@ -537,3 +537,25 @@ func TestParseFilesWithDependencies(t *testing.T) {
 		}
 	})
 }
+
+func TestParseCommentsBeforeDot(t *testing.T) {
+	accessor := FileContentsFromMap(map[string]string{
+		"test.proto": `
+syntax = "proto3";
+message Foo {
+  // leading comments
+  .Foo foo = 1;
+}
+`,
+	})
+
+	p := Parser{
+		Accessor:              accessor,
+		IncludeSourceCodeInfo: true,
+	}
+	fds, err := p.ParseFiles("test.proto")
+	testutil.Ok(t, err)
+
+	comment := fds[0].GetMessageTypes()[0].GetFields()[0].GetSourceInfo().GetLeadingComments()
+	testutil.Eq(t, " leading comments\n", comment)
+}