Repository: jhump/protoreflect
Pull request #414 (2021-08-17 14:46:58)

fix more inconsistencies between protoparse and protoc
Addresses #413. The linked issue mentioned three separate problems: two issues were programs that protoparse would accept but protoc would reject; one was the reverse, a case that protoparse rejects but protoc accepts.

They are addressed in three distinct commits on this branch.

WillAbides: 👋 It looks like I was duplicating your effort in #414 and #415. Even though I'm not a contributor here, I'll do a review since I happened to have been looking at the same code recently.
jhump: @WillAbides, sorry if I was stepping on your toes a bit. Hopefully these changes make sense. My change for the "Need space between number and identifier" issue was to more closely mirror how protoc tokenizes the document -- so I made the change in the code that reads a number during lex'ing (instead of enforcing this later during parsing). The benefit is that it would also catch anywhere else in the grammar where an identifier/keyword is allowed to follow a numeric literal (which, TBH, there may be none).


WillAbides: My toes are completely unharmed.

Making the change in lexing seems like the better approach. Whether there are other instances of this or not, it's usually better to fix problems at the source.

Patch:
diff --git a/desc/protoparse/lexer.go b/desc/protoparse/lexer.go
index faac16e..7c6d6a2 100644
--- a/desc/protoparse/lexer.go
+++ b/desc/protoparse/lexer.go
@@ -221,7 +221,7 @@ func (l *protoLex) Lex(lval *protoSymType) int {
 
 		l.offset += n
 		l.adjustPos(c)
-		if strings.ContainsRune("\n\r\t ", c) {
+		if strings.ContainsRune("\n\r\t\f\v ", c) {
 			l.ws = append(l.ws, c)
 			continue
 		}
@@ -236,11 +236,10 @@ func (l *protoLex) Lex(lval *protoSymType) int {
 			}
 			if cn >= '0' && cn <= '9' {
 				l.adjustPos(cn)
-				token := []rune{c, cn}
-				token = l.readNumber(token, false, true)
-				f, err := strconv.ParseFloat(string(token), 64)
+				token := l.readNumber(c, cn)
+				f, err := strconv.ParseFloat(token, 64)
 				if err != nil {
-					l.setError(lval, err)
+					l.setError(lval, numError(err, "float", token))
 					return _ERROR
 				}
 				l.setFloat(lval, f)
@@ -266,66 +265,42 @@ func (l *protoLex) Lex(lval *protoSymType) int {
 
 		if c >= '0' && c <= '9' {
 			// integer or float literal
-			if c == '0' {
-				cn, _, err := l.input.readRune()
+			token := l.readNumber(c)
+			if strings.HasPrefix(token, "0x") || strings.HasPrefix(token, "0X") {
+				// hexadecimal
+				ui, err := strconv.ParseUint(token[2:], 16, 64)
 				if err != nil {
-					l.setInt(lval, 0)
-					return _INT_LIT
-				}
-				if cn == 'x' || cn == 'X' {
-					cnn, _, err := l.input.readRune()
-					if err != nil {
-						l.input.unreadRune(cn)
-						l.setInt(lval, 0)
-						return _INT_LIT
-					}
-					if (cnn >= '0' && cnn <= '9') || (cnn >= 'a' && cnn <= 'f') || (cnn >= 'A' && cnn <= 'F') {
-						// hexadecimal!
-						l.adjustPos(cn, cnn)
-						token := []rune{cnn}
-						token = l.readHexNumber(token)
-						ui, err := strconv.ParseUint(string(token), 16, 64)
-						if err != nil {
-							l.setError(lval, err)
-							return _ERROR
-						}
-						l.setInt(lval, ui)
-						return _INT_LIT
-					}
-					l.input.unreadRune(cnn)
-					l.input.unreadRune(cn)
-					l.setInt(lval, 0)
-					return _INT_LIT
-				} else {
-					l.input.unreadRune(cn)
+					l.setError(lval, numError(err, "hexadecimal integer", token[2:]))
+					return _ERROR
 				}
+				l.setInt(lval, ui)
+				return _INT_LIT
 			}
-			token := []rune{c}
-			token = l.readNumber(token, true, true)
-			numstr := string(token)
-			if strings.Contains(numstr, ".") || strings.Contains(numstr, "e") || strings.Contains(numstr, "E") {
+			if strings.Contains(token, ".") || strings.Contains(token, "e") || strings.Contains(token, "E") {
 				// floating point!
-				f, err := strconv.ParseFloat(numstr, 64)
+				f, err := strconv.ParseFloat(token, 64)
 				if err != nil {
-					l.setError(lval, err)
+					l.setError(lval, numError(err, "float", token))
 					return _ERROR
 				}
 				l.setFloat(lval, f)
 				return _FLOAT_LIT
 			}
 			// integer! (decimal or octal)
-			ui, err := strconv.ParseUint(numstr, 0, 64)
+			ui, err := strconv.ParseUint(token, 0, 64)
 			if err != nil {
+				kind := "integer"
 				if numErr, ok := err.(*strconv.NumError); ok && numErr.Err == strconv.ErrRange {
 					// if it's too big to be an int, parse it as a float
 					var f float64
-					f, err = strconv.ParseFloat(numstr, 64)
+					kind = "float"
+					f, err = strconv.ParseFloat(token, 64)
 					if err == nil {
 						l.setFloat(lval, f)
 						return _FLOAT_LIT
 					}
 				}
-				l.setError(lval, err)
+				l.setError(lval, numError(err, kind, token))
 				return _ERROR
 			}
 			l.setInt(lval, ui)
@@ -525,79 +500,47 @@ func (l *protoLex) setError(lval *protoSymType, err error) {
 	lval.err = l.addSourceError(err)
 }
 
-func (l *protoLex) readNumber(sofar []rune, allowDot bool, allowExp bool) []rune {
+func (l *protoLex) readNumber(sofar ...rune) string {
 	token := sofar
+	allowExpSign := false
 	for {
 		c, _, err := l.input.readRune()
 		if err != nil {
 			break
 		}
-		if c == '.' {
-			if !allowDot {
-				l.input.unreadRune(c)
-				break
-			}
-			allowDot = false
-		} else if c == 'e' || c == 'E' {
-			if !allowExp {
-				l.input.unreadRune(c)
-				break
-			}
-			allowExp = false
-			cn, _, err := l.input.readRune()
-			if err != nil {
-				l.input.unreadRune(c)
-				break
-			}
-			if cn == '-' || cn == '+' {
-				cnn, _, err := l.input.readRune()
-				if err != nil {
-					l.input.unreadRune(cn)
-					l.input.unreadRune(c)
-					break
-				}
-				if cnn < '0' || cnn > '9' {
-					l.input.unreadRune(cnn)
-					l.input.unreadRune(cn)
-					l.input.unreadRune(c)
-					break
-				}
-				l.adjustPos(c)
-				token = append(token, c)
-				c, cn = cn, cnn
-			} else if cn < '0' || cn > '9' {
-				l.input.unreadRune(cn)
-				l.input.unreadRune(c)
-				break
-			}
-			l.adjustPos(c)
-			token = append(token, c)
-			c = cn
-		} else if c < '0' || c > '9' {
+		if (c == '-' || c == '+') && !allowExpSign {
 			l.input.unreadRune(c)
 			break
 		}
-		l.adjustPos(c)
-		token = append(token, c)
-	}
-	return token
-}
-
-func (l *protoLex) readHexNumber(sofar []rune) []rune {
-	token := sofar
-	for {
-		c, _, err := l.input.readRune()
-		if err != nil {
-			break
-		}
-		if (c < 'a' || c > 'f') && (c < 'A' || c > 'F') && (c < '0' || c > '9') {
+		allowExpSign = false
+		if c != '.' && c != '_' && (c < '0' || c > '9') &&
+			(c < 'a' || c > 'z') && (c < 'A' || c > 'Z') &&
+			c != '-' && c != '+' {
+			// no more chars in the number token
 			l.input.unreadRune(c)
 			break
 		}
+		if c == 'e' || c == 'E' {
+			// scientific notation char can be followed by
+			// an exponent sign
+			allowExpSign = true
+		}
 		l.adjustPos(c)
 		token = append(token, c)
 	}
-	return token
+	return string(token)
+}
+
+func numError(err error, kind, s string) error {
+	ne, ok := err.(*strconv.NumError)
+	if !ok {
+		return err
+	}
+	if ne.Err == strconv.ErrRange {
+		return fmt.Errorf("value out of range for %s: %s", kind, s)
+	}
+	// syntax error
+	return fmt.Errorf("invalid syntax in %s value: %s", kind, s)
 }
 
 func (l *protoLex) readIdentifier(sofar []rune) []rune {
diff --git a/desc/protoparse/linker.go b/desc/protoparse/linker.go
index 2aa32f0..4b4c1d8 100644
--- a/desc/protoparse/linker.go
+++ b/desc/protoparse/linker.go
@@ -487,22 +487,26 @@ func (l *linker) resolveFieldTypes(r *parseResult, fd *dpb.FileDescriptorProto,
 }
 
 func (l *linker) resolveServiceTypes(r *parseResult, fd *dpb.FileDescriptorProto, prefix string, sd *dpb.ServiceDescriptorProto, scopes []scope) error {
-	thisName := prefix + sd.GetName()
+	svcFqn := prefix + sd.GetName()
 	if sd.Options != nil {
-		if err := l.resolveOptions(r, fd, "service", thisName, proto.MessageName(sd.Options), sd.Options.UninterpretedOption, scopes); err != nil {
+		if err := l.resolveOptions(r, fd, "service", svcFqn, proto.MessageName(sd.Options), sd.Options.UninterpretedOption, scopes); err != nil {
 			return err
 		}
 	}
 
+	// not a message, but same scoping rules for nested elements as if it were
+	scope := messageScope(svcFqn, isProto3(fd), l, fd)
+	scopes = append(scopes, scope)
+
 	for _, mtd := range sd.Method {
 		if mtd.Options != nil {
-			if err := l.resolveOptions(r, fd, "method", thisName+"."+mtd.GetName(), proto.MessageName(mtd.Options), mtd.Options.UninterpretedOption, scopes); err != nil {
+			if err := l.resolveOptions(r, fd, "method", svcFqn+"."+mtd.GetName(), proto.MessageName(mtd.Options), mtd.Options.UninterpretedOption, scopes); err != nil {
 				return err
 			}
 		}
-		scope := fmt.Sprintf("method %s.%s", thisName, mtd.GetName())
+		scope := fmt.Sprintf("method %s.%s", svcFqn, mtd.GetName())
 		node := r.getMethodNode(mtd)
-		fqn, dsc, _ := l.resolve(fd, mtd.GetInputType(), true, scopes)
+		fqn, dsc, _ := l.resolve(fd, mtd.GetInputType(), false, scopes)
 		if dsc == nil {
 			if err := l.errs.handleErrorWithPos(node.GetInputType().Start(), "%s: unknown request type %s", scope, mtd.GetInputType()); err != nil {
 				return err
@@ -521,7 +525,7 @@ func (l *linker) resolveServiceTypes(r *parseResult, fd *dpb.FileDescriptorProto
 		}
 
 		// TODO: make input and output type resolution more DRY
-		fqn, dsc, _ = l.resolve(fd, mtd.GetOutputType(), true, scopes)
+		fqn, dsc, _ = l.resolve(fd, mtd.GetOutputType(), false, scopes)
 		if dsc == nil {
 			if err := l.errs.handleErrorWithPos(node.GetOutputType().Start(), "%s: unknown response type %s", scope, mtd.GetOutputType()); err != nil {
 				return err


Test patch:
diff --git a/desc/protoparse/lexer_test.go b/desc/protoparse/lexer_test.go
index bfab1e6..e48f37b 100644
--- a/desc/protoparse/lexer_test.go
+++ b/desc/protoparse/lexer_test.go
@@ -53,9 +53,9 @@ foo
 	syntax = "proto2";
 
 	// some strange cases
-	1.543g12 /* trailing line comment */
+	1.543 g12 /* trailing line comment */
 	000.000
-	0.1234.5678.
+	0.1234 .5678 .
 	12e12
 
 	Random_identifier_with_numbers_0123456789_and_letters...
@@ -69,6 +69,8 @@ foo
 
 	// this is an attached leading comment
 	foo
+
+	1.23e+20+20
 	// a trailing comment for last element
 
 	// comment attached to no tokens (upcoming token is EOF!)
@@ -143,17 +145,20 @@ foo
 		{t: _STRING_LIT, line: 41, col: 18, span: 8, v: "proto2"},
 		{t: ';', line: 41, col: 26, span: 1, v: nil},
 		{t: _FLOAT_LIT, line: 44, col: 9, span: 5, v: 1.543, comments: []string{"// some strange cases\n"}},
-		{t: _NAME, line: 44, col: 14, span: 3, v: "g12"},
+		{t: _NAME, line: 44, col: 15, span: 3, v: "g12"},
 		{t: _FLOAT_LIT, line: 45, col: 9, span: 7, v: 0.0, comments: []string{"/* trailing line comment */"}, trailCount: 1},
 		{t: _FLOAT_LIT, line: 46, col: 9, span: 6, v: 0.1234},
-		{t: _FLOAT_LIT, line: 46, col: 15, span: 5, v: 0.5678},
-		{t: '.', line: 46, col: 20, span: 1, v: nil},
+		{t: _FLOAT_LIT, line: 46, col: 16, span: 5, v: 0.5678},
+		{t: '.', line: 46, col: 22, span: 1, v: nil},
 		{t: _FLOAT_LIT, line: 47, col: 9, span: 5, v: 12e12},
 		{t: _NAME, line: 49, col: 9, span: 53, v: "Random_identifier_with_numbers_0123456789_and_letters"},
 		{t: '.', line: 49, col: 62, span: 1, v: nil},
 		{t: '.', line: 49, col: 63, span: 1, v: nil},
 		{t: '.', line: 49, col: 64, span: 1, v: nil},
 		{t: _NAME, line: 59, col: 9, span: 3, v: "foo", comments: []string{"// this is a trailing comment\n", "// that spans multiple lines\n", "// over two in fact!\n", "/*\n\t * this is a detached comment\n\t * with lots of extra words and stuff...\n\t */", "// this is an attached leading comment\n"}, trailCount: 3},
+		{t: _FLOAT_LIT, line: 61, col: 9, span: 8, v: 1.23e+20},
+		{t: '+', line: 61, col: 17, span: 1, v: nil},
+		{t: _INT_LIT, line: 61, col: 18, span: 2, v: uint64(20)},
 	}
 
 	for i, exp := range expected {
@@ -176,17 +181,19 @@ foo
 		case _FLOAT_LIT:
 			n = sym.f
 			val = sym.f.Val
+		case _ERROR:
+			val = sym.err
 		default:
 			n = sym.b
 			val = nil
 		}
-		testutil.Eq(t, exp.t, tok, "case %d: wrong token type (case %v)", i, exp.v)
+		testutil.Eq(t, exp.t, tok, "case %d: wrong token type (expecting value %v, got %v)", i, exp.v, val)
 		testutil.Eq(t, exp.v, val, "case %d: wrong token value", i)
 		testutil.Eq(t, exp.line, n.Start().Line, "case %d: wrong line number", i)
-		testutil.Eq(t, exp.col, n.Start().Col, "case %d: wrong column number", i)
+		testutil.Eq(t, exp.col, n.Start().Col, "case %d: wrong column number (on line %d)", i, exp.line)
 		testutil.Eq(t, exp.line, n.End().Line, "case %d: wrong end line number", i)
 		testutil.Eq(t, exp.col+exp.span, n.End().Col, "case %d: wrong end column number", i)
-		if exp.trailCount > 0 {
+		if prev != nil {
 			testutil.Eq(t, exp.trailCount, len(prev.TrailingComments()), "case %d: wrong number of trailing comments", i)
 		}
 		testutil.Eq(t, len(exp.comments)-exp.trailCount, len(n.LeadingComments()), "case %d: wrong number of comments", i)
@@ -228,6 +235,13 @@ func TestLexerErrors(t *testing.T) {
 		{str: `"foobar\U00110000foo"`, errMsg: "unicode escape is out of range"},
 		{str: "'foobar\nbaz'", errMsg: "encountered end-of-line"},
 		{str: "'foobar\000baz'", errMsg: "null character ('\\0') not allowed"},
+		{str: `1.543g12`, errMsg: "invalid syntax"},
+		{str: `0.1234.5678.`, errMsg: "invalid syntax"},
+		{str: `0x987.345aaf`, errMsg: "invalid syntax"},
+		{str: `0.987.345`, errMsg: "invalid syntax"},
+		{str: `0.987e34e-20`, errMsg: "invalid syntax"},
+		{str: `0.987e-345e20`, errMsg: "invalid syntax"},
+		{str: `.987to123`, errMsg: "invalid syntax"},
 		{str: `/* foobar`, errMsg: "unexpected EOF"},
 	}
 	for i, tc := range testCases {
@@ -237,6 +251,7 @@ func TestLexerErrors(t *testing.T) {
 		testutil.Eq(t, _ERROR, tok)
 		testutil.Require(t, sym.err != nil)
 		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())
+		t.Logf("case %d: %v", i, sym.err)
 	}
 }
 
diff --git a/desc/protoparse/linker_test.go b/desc/protoparse/linker_test.go
index 287dc66..ae3add9 100644
--- a/desc/protoparse/linker_test.go
+++ b/desc/protoparse/linker_test.go
@@ -166,6 +166,18 @@ func TestLinkerValidation(t *testing.T) {
 			},
 			"foo.proto:1:41: extendee is invalid: fu.baz.foobar is a service, not a message",
 		},
+		{
+			map[string]string{
+				"foo.proto": "message foo{} message bar{} service foobar{ rpc foo(foo) returns (bar); }",
+			},
+			"foo.proto:1:53: method foobar.foo: invalid request type: foobar.foo is a method, not a message",
+		},
+		{
+			map[string]string{
+				"foo.proto": "message foo{} message bar{} service foobar{ rpc foo(bar) returns (foo); }",
+			},
+			"foo.proto:1:67: method foobar.foo: invalid response type: foobar.foo is a method, not a message",
+		},
 		{
 			map[string]string{
 				"foo.proto": "package fu.baz; message foobar{ extensions 1; } extend foobar { optional string a = 2; }",
@@ -196,7 +208,7 @@ func TestLinkerValidation(t *testing.T) {
 					extend google.protobuf.ServiceOptions        { optional string svc_foo = 12000; }
 					extend google.protobuf.MethodOptions         { optional string mtd_foo = 12000; }
 					option (fil_foo) = "file";
-					message Bar {
+					message Foo {
 						option (msg_foo) = "message";
 						oneof foo {
 							option (oof_foo) = "oneof";
@@ -210,7 +222,7 @@ func TestLinkerValidation(t *testing.T) {
 					}
 					service FooService {
 						option (svc_foo) = "service";
-						rpc Bar(Bar) returns (Bar) {
+						rpc Bar(Foo) returns (Foo) {
 							option (mtd_foo) = "method";
 						}
 					}
diff --git a/desc/protoparse/reporting_test.go b/desc/protoparse/reporting_test.go
index 8757578..a952e42 100644
--- a/desc/protoparse/reporting_test.go
+++ b/desc/protoparse/reporting_test.go
@@ -98,17 +98,17 @@ func TestErrorReporting(t *testing.T) {
 						BAZ = 2;
 					}
 					service Bar {
-						rpc Foo (Foo) returns (Foo);
-						rpc Foo (Frob) returns (Nitz);
+						rpc Foobar (Foo) returns (Foo);
+						rpc Foobar (Frob) returns (Nitz);
 					}
 					`,
 			},
 			expectedErrs: []string{
 				"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",
-				"test.proto:12:73: method Bar.Foo: unknown response type Nitz",
+				"test.proto:12:49: duplicate symbol Bar.Foobar: already defined as method",
+				"test.proto:12:61: method Bar.Foobar: unknown request type Frob",
+				"test.proto:12:76: method Bar.Foobar: unknown response type Nitz",
 			},
 		},
 		{
diff --git a/desc/protoparse/validate_test.go b/desc/protoparse/validate_test.go
index 3f85de9..be014c8 100644
--- a/desc/protoparse/validate_test.go
+++ b/desc/protoparse/validate_test.go
@@ -352,6 +352,10 @@ func TestBasicValidation(t *testing.T) {
 		},
 		{
 			contents: `0.0.0.0.0`,
+			errMsg:   `test.proto:1:1: invalid syntax in float value: 0.0.0.0.0`,
+		},
+		{
+			contents: `0.0`,
 			errMsg:   `test.proto:1:1: syntax error: unexpected float literal`,
 		},
 	}