Update protoc to v22.0 (#100)

This repo uses `protoc` for verification -- to make sure that things
like options interpretation and source code info match what the
reference compiler produces.

This updates the version of `protoc` used to the latest release v22.0.
This uncovered discrepancies: changes/fixes in `protoc` that required
changes to protocompile code to match. This includes a very minor
change to the canonical binary format for options and more significant
improvements to comment attribution in source code info.

Author: jhump

Makefile

@@ -16,7 +16,7 @@ UNAME_OS := $(shell uname -s)
 UNAME_ARCH := $(shell uname -m)
 
 # NB: this must be kept in sync with constant in internal/benchmarks.
-PROTOC_VERSION ?= 21.7
+PROTOC_VERSION ?= 22.0
 PROTOC_DIR := $(abspath ./internal/testdata/protoc/$(PROTOC_VERSION))
 PROTOC := $(PROTOC_DIR)/bin/protoc
 

go.mod

@@ -6,7 +6,7 @@ require (
 	github.com/google/go-cmp v0.5.9
 	github.com/stretchr/testify v1.8.1
 	golang.org/x/sync v0.0.0-20210220032951-036812b2e83c
-	google.golang.org/protobuf v1.28.2-0.20220831092852-f930b1dc76e8
+	google.golang.org/protobuf v1.28.2-0.20230222093303-bc1253ad3743
 )
 
 require (

go.sum

@@ -18,8 +18,8 @@ golang.org/x/sync v0.0.0-20210220032951-036812b2e83c h1:5KslGYwFpkhGh+Q16bwMP3cO
 golang.org/x/sync v0.0.0-20210220032951-036812b2e83c/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
 golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
 google.golang.org/protobuf v1.26.0-rc.1/go.mod h1:jlhhOSvTdKEhbULTjvd4ARK9grFBp09yW+WbY/TyQbw=
-google.golang.org/protobuf v1.28.2-0.20220831092852-f930b1dc76e8 h1:KR8+MyP7/qOlV+8Af01LtjL04bu7on42eVsxT4EyBQk=
-google.golang.org/protobuf v1.28.2-0.20220831092852-f930b1dc76e8/go.mod h1:HV8QOd/L58Z+nl8r43ehVNZIU/HEI6OcFqwMG9pJV4I=
+google.golang.org/protobuf v1.28.2-0.20230222093303-bc1253ad3743 h1:yqElulDvOF26oZ2O+2/aoX7mQ8DY/6+p39neytrycd8=
+google.golang.org/protobuf v1.28.2-0.20230222093303-bc1253ad3743/go.mod h1:HV8QOd/L58Z+nl8r43ehVNZIU/HEI6OcFqwMG9pJV4I=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
 gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=

internal/benchmarks/benchmark_test.go

@@ -51,7 +51,7 @@ import (
 
 const (
 	// NB: this must be kept in sync with PROTOC_VERSION in Makefile.
-	protocVersion = "21.7"
+	protocVersion = "22.0"
 
 	googleapisCommit = "cb6fbe8784479b22af38c09a5039d8983e894566"
 )

internal/testdata/all.protoset

@@ -58,10 +58,14 @@ message /* detached message name */ /* request with a capital R */ Request // tr
 		// this is a custom option
 		option (testprotos.mfubar) = false;
 
-		optional double dbl = 1;
-		optional float flt = 2;
+		optional double dbl = 1; /* trailing comment for dbl */ /* detached comment */ /* leading comment for flt */ optional float flt = 2;
 
-		option no_standard_descriptor_accessor = false;
+		option no_standard_descriptor_accessor = false; /* weird trailing comment
+		                                                   for the option that gets
+		                                                   classified as detached
+		                                                   since it's on the same
+		                                                   line as the following
+		                                                   element */ option deprecated=true;
 
 		// Leading comment...
 		optional string str = 3;
@@ -76,8 +80,7 @@ message /* detached message name */ /* request with a capital R */ Request // tr
 
 		MARIO = 1 [(testprotos.evfubars) = -314, (testprotos.evfubar) = 278];
 		LUIGI = 2 [ (testprotos.evfubaruf) = 100, /* swoosh! */ (testprotos.evfubaru)=200];
-		PEACH = 3;
-		BOWSER = 4;
+		PEACH = 3; /* peach trailer */ /* bowser leader */ BOWSER = 4;
 
 		option (testprotos.efubars) = -321;
 
@@ -128,7 +131,14 @@ Request
 }
 // after extend block
 
-message /* name leading comment */ AnEmptyMessage /* name trailing comment */ { /* trailer for AnEmptyMessage */ }
+message /* name leading comment */ AnEmptyMessage /* name trailing comment */ { /* detached comment inside AnEmptyMessage */ }
+
+/*
+ * Tests javadoc style comment, where every line in block comment has leading
+ * asterisk that should be stripped.
+ */
+message AnotherEmptyMessage  { /* trailer for AnotherEmptyMessage */
+}
 
 // Service comment
 service /* service name */ RpcService {
@@ -141,6 +151,10 @@ service /* service name */ RpcService {
 	option(testprotos.sfubar).name= "bob";
 	option deprecated = false; // DEPRECATED!
 
+	/**
+	 * Another javadoc-style comment.
+	 * This one has the double-asterisk on first line, like javadoc.
+	 */
 	option (testprotos.sfubare) = VALUE;
 
 	// Method comment

internal/testdata/desc_test_comments.proto

@@ -68,4 +68,9 @@ extend google.protobuf.FileOptions {
 }
 
 // a file option!
-option (flfubar) = "foobar"; // line comment with no trailing newline
+// TODO: After https://github.com/protocolbuffers/protobuf/pull/12082 makes it into
+//       a protoc release, remove the newline at the end of the file to make the
+//       following comment true. (For now, we need the newline in order for protoc
+//       to produce source code info we can match with protocompile since we don't
+//       have the same bug.)
+option (flfubar) = "foobar"; // line comment with no trailing newline

internal/testdata/desc_test_complex.protoset

@@ -784,14 +784,66 @@ func (interp *interpreter) interpretOptions(fqn string, element, opts proto.Mess
 	return nil, nil
 }
 
+// isKnownField returns true if the given option is for a known field of the
+// given options message descriptor and will be serialized using the expected
+// wire type for that known field.
 func isKnownField(desc protoreflect.MessageDescriptor, opt *interpretedOption) bool {
 	var num int32
 	if len(opt.pathPrefix) > 0 {
 		num = opt.pathPrefix[0]
 	} else {
 		num = opt.number
 	}
-	return desc.Fields().ByNumber(protoreflect.FieldNumber(num)) != nil
+	fd := desc.Fields().ByNumber(protoreflect.FieldNumber(num))
+	if fd == nil {
+		return false
+	}
+
+	// Before the full wire type check, we do a quick check that will usually pass
+	// and allow us to short-circuit the logic below.
+	if fd.IsList() == opt.repeated && fd.Kind() == opt.kind {
+		return true
+	}
+
+	// We figure out the wire type this interpreted field will use when serialized.
+	var wireType protowire.Type
+	switch {
+	case len(opt.pathPrefix) > 0:
+		// If path prefix exists, this field is nested inside a message.
+		// And messages use bytes wire type.
+		wireType = protowire.BytesType
+	case opt.repeated && opt.packed && canPack(opt.kind):
+		// Packed repeated numeric scalars use bytes wire type.
+		wireType = protowire.BytesType
+	default:
+		wireType = wireTypeForKind(opt.kind)
+	}
+
+	// And then we see if the wire type we just determined is compatible with
+	// the field descriptor we found.
+	if fd.IsList() && canPack(fd.Kind()) && wireType == protowire.BytesType {
+		// Even if fd.IsPacked() is false, bytes type is still accepted for
+		// repeated scalar numerics, so that changing a repeated field from
+		// packed to not-packed (or vice versa) is a compatible change.
+		return true
+	}
+	return wireType == wireTypeForKind(fd.Kind())
+}
+
+func wireTypeForKind(kind protoreflect.Kind) protowire.Type {
+	switch kind {
+	case protoreflect.StringKind, protoreflect.BytesKind, protoreflect.MessageKind:
+		return protowire.BytesType
+	case protoreflect.GroupKind:
+		return protowire.StartGroupType
+	case protoreflect.Fixed32Kind, protoreflect.Sfixed32Kind, protoreflect.FloatKind:
+		return protowire.Fixed32Type
+	case protoreflect.Fixed64Kind, protoreflect.Sfixed64Kind, protoreflect.DoubleKind:
+		return protowire.Fixed64Type
+	default:
+		// everything else uses varint
+		return protowire.VarintType
+	}
 }
 
 func cloneInto(dest proto.Message, src proto.Message, res linker.Resolver) error {

internal/testdata/desc_test_options.proto

@@ -651,17 +651,8 @@ func (sci *sourceCodeInfo) maybeDonate(prevInfo ast.NodeInfo, info ast.NodeInfo,
 		// nothing to donate
 		return ast.EmptyComments, nil
 	}
-	if !sci.extraComments {
-		// Mirroring protoc for now: if tokens on the same line, attribution
-		// is ambiguous so drop everything.
-		// This condition can be dropped (and we can stop dropping comments)
-		// once this PR is released:
-		//   https://github.com/protocolbuffers/protobuf/pull/10660
-		if prevInfo.End().Line == info.Start().Line {
-			return ast.EmptyComments, nil
-		}
-	}
-	if lead[0].Index(0).Start().Line > prevInfo.End().Line+1 {
+	firstCommentPos := lead[0].Index(0)
+	if firstCommentPos.Start().Line > prevInfo.End().Line+1 {
 		// first comment is detached from previous token, so can't be a trailing comment
 		return ast.EmptyComments, lead
 	}
@@ -671,16 +662,26 @@ func (sci *sourceCodeInfo) maybeDonate(prevInfo ast.NodeInfo, info ast.NodeInfo,
 	}
 	// there is only one element in lead
 	comment := lead[0]
-	if comment.Index(comment.Len()-1).End().Line < info.Start().Line-1 {
+	lastCommentPos := comment.Index(comment.Len() - 1)
+	if lastCommentPos.End().Line < info.Start().Line-1 {
 		// there is a blank line between the comments and subsequent token, so
 		// we can donate the comment to previous token
 		return comment, nil
 	}
-	if txt := info.RawText(); len(txt) == 1 && strings.ContainsAny(txt, "}]),;") {
-		// token is a symbol for the end of a scope, which doesn't need a leading comment
+	if txt := info.RawText(); txt == "" || (len(txt) == 1 && strings.ContainsAny(txt, "}]),;")) {
+		// token is a symbol for the end of a scope or EOF, which doesn't need a leading comment
+		if !sci.extraComments && txt != "" &&
+			firstCommentPos.Start().Line == prevInfo.End().Line &&
+			lastCommentPos.End().Line == info.Start().Line {
+			// protoc does not donate if prev and next token are on the same line since it's
+			// ambiguous which one should get the comment; so we mirror that here
+			return ast.EmptyComments, lead
+		}
+		// But with extra comments, we always donate in this situation in order to capture
+		// more comments. Because otherwise, these comments are lost since these symbols
+		// don't map to a location in source code info.
 		return comment, nil
 	}
-
 	// cannot donate
 	return ast.EmptyComments, lead
 }
@@ -697,14 +698,6 @@ func (sci *sourceCodeInfo) maybeAttach(prevInfo ast.NodeInfo, info ast.NodeInfo,
 		attachedToPrevious := comment.Index(0).Start().Line == prevInfo.End().Line
 		attachedToNext := comment.Index(comment.Len()-1).End().Line == info.Start().Line
 		if attachedToPrevious && attachedToNext {
-			if !sci.extraComments {
-				// Mirroring protoc for now: if comment starts on line of previous token
-				// and ends on line of next, attribution is ambiguous so drop everything.
-				// This condition can be dropped (and we can stop dropping comments)
-				// once this PR is released:
-				//   https://github.com/protocolbuffers/protobuf/pull/10660
-				return nil, ast.EmptyComments
-			}
 			// Since attachment is ambiguous, leave it detached.
 			return lead, ast.EmptyComments
 		}
@@ -787,9 +780,10 @@ func (sci *sourceCodeInfo) combineComments(comments comments) string {
 			for _, l := range lines {
 				if first {
 					first = false
-				} else {
-					buf.WriteByte('\n')
+					buf.WriteString(l)
+					continue
 				}
+				buf.WriteByte('\n')
 
 				// strip a prefix of whitespace followed by '*'
 				j := 0
@@ -805,7 +799,7 @@ func (sci *sourceCodeInfo) combineComments(comments comments) string {
 				case l[j] == '*':
 					l = l[j+1:]
 				case j > 0:
-					l = " " + l[j:]
+					l = l[j:]
 				}
 
 				buf.WriteString(l)