compiler/protogen, cmd/protoc-gen-go: use alternative comments API

This is a breaking change. High-level protogen API changes:
* remove GeneratedFile.PrintLeadingComments method
* add {Message,Field,Oneof,Enum,EnumValue,Service,Method}.Comments field
* add CommentSet and Comments type

CL/183157 added protoreflect.SourceLocations and it was discovered
that there can actually be duplicate locations for certain paths.
For that reason, we decided not to expose any helper methods
for looking up locations by path since it is unclear which location
to return if multiple matches.

The protogen.GeneratedFile.PrintLeadingComments has a similar dilemma
where it also needs to figure out what to do when duplicates exist.
Previously, it just chooses the first one with comments,
which may not be the right choice in a given context.

Analysis of current PrintLeadingComments usage shows that it is only
ever used (except once) for descriptor declarations.
In the case of descriptor declarations, they are guaranteed by protoc
to have only location.

Thus, we avoid the duplicate location problem by:
* Providing a CommentSet for every descriptor. The CommentSet contains
a set of leading and trailing comments of the Comments type.
* The Comments.String method knows how to interpret the comments
as provided by protoc and format them as // prefixed line comments.
* Values of the Comments type can be passed to the P method.

We drop direct support printing leading comments for non-descriptor locations,
but the exposure of the Comments type makes it easy for users to manually
handle other types of comments themselves.

Change-Id: Id4851456dc4e64d76bd6a30e8ad6137408dfb27a
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/189198
Reviewed-by: Damien Neil <dneil@google.com>

cmd/protoc-gen-go-grpc/internal_gengogrpc/grpc.go

@@ -71,9 +71,9 @@ func genService(gen *protogen.Plugin, file *protogen.File, g *protogen.Generated
 	g.Annotate(clientName, service.Location)
 	g.P("type ", clientName, " interface {")
 	for _, method := range service.Methods {
-		g.PrintLeadingComments(method.Location)
 		g.Annotate(clientName+"."+method.GoName, method.Location)
-		g.P(clientSignature(g, method))
+		g.P(method.Comments.Leading,
+			clientSignature(g, method))
 	}
 	g.P("}")
 	g.P()
@@ -117,9 +117,9 @@ func genService(gen *protogen.Plugin, file *protogen.File, g *protogen.Generated
 	g.Annotate(serverType, service.Location)
 	g.P("type ", serverType, " interface {")
 	for _, method := range service.Methods {
-		g.PrintLeadingComments(method.Location)
 		g.Annotate(serverType+"."+method.GoName, method.Location)
-		g.P(serverSignature(g, method))
+		g.P(method.Comments.Leading,
+			serverSignature(g, method))
 	}
 	g.P("}")
 	g.P()

cmd/protoc-gen-go/internal_gengo/main.go

@@ -151,11 +151,15 @@ func GenerateFile(gen *protogen.Plugin, file *protogen.File) *protogen.Generated
 		g.P("// source: ", f.Desc.Path())
 	}
 	g.P()
-	g.PrintLeadingComments(protogen.Location{
-		SourceFile: f.Proto.GetName(),
-		Path:       []int32{fieldnum.FileDescriptorProto_Package},
-	})
-	g.P()
+
+	for _, loc := range f.Proto.GetSourceCodeInfo().GetLocation() {
+		if len(loc.Path) == 1 && loc.Path[0] == fieldnum.FileDescriptorProto_Package {
+			if s := loc.GetLeadingComments(); s != "" {
+				g.P(protogen.Comments(s))
+				g.P()
+			}
+		}
+	}
 	g.P("package ", f.GoPackageName)
 	g.P()
 
@@ -274,17 +278,17 @@ func genImport(gen *protogen.Plugin, g *protogen.GeneratedFile, f *fileInfo, imp
 
 func genEnum(gen *protogen.Plugin, g *protogen.GeneratedFile, f *fileInfo, enum *protogen.Enum) {
 	// Enum type declaration.
-	g.PrintLeadingComments(enum.Location)
 	g.Annotate(enum.GoIdent.GoName, enum.Location)
-	g.P("type ", enum.GoIdent, " int32",
+	g.P(enum.Comments.Leading,
+		"type ", enum.GoIdent, " int32",
 		deprecationComment(enum.Desc.Options().(*descriptorpb.EnumOptions).GetDeprecated()))
 
 	// Enum value constants.
 	g.P("const (")
 	for _, value := range enum.Values {
-		g.PrintLeadingComments(value.Location)
 		g.Annotate(value.GoIdent.GoName, value.Location)
-		g.P(value.GoIdent, " ", enum.GoIdent, " = ", value.Desc.Number(),
+		g.P(value.Comments.Leading,
+			value.GoIdent, " ", enum.GoIdent, " = ", value.Desc.Number(),
 			deprecationComment(value.Desc.Options().(*descriptorpb.EnumValueOptions).GetDeprecated()))
 	}
 	g.P(")")
@@ -377,15 +381,17 @@ func genMessage(gen *protogen.Plugin, g *protogen.GeneratedFile, f *fileInfo, me
 	}
 
 	// Message type declaration.
-	hasComment := g.PrintLeadingComments(message.Location)
+	leadingComments := message.Comments.Leading
 	if message.Desc.Options().(*descriptorpb.MessageOptions).GetDeprecated() {
-		if hasComment {
-			g.P("//")
+		if leadingComments != "" {
+			g.P(leadingComments, "//")
+			leadingComments = "" // avoid printing them again later
 		}
 		g.P(deprecationComment(true))
 	}
 	g.Annotate(message.GoIdent.GoName, message.Location)
-	g.P("type ", message.GoIdent, " struct {")
+	g.P(leadingComments,
+		"type ", message.GoIdent, " struct {")
 	genMessageFields(g, f, message)
 	g.P("}")
 	g.P()
@@ -446,20 +452,19 @@ func genMessageField(g *protogen.GeneratedFile, f *fileInfo, message *protogen.M
 		if oneof.Fields[0] != field {
 			return // only generate for first appearance
 		}
-		if g.PrintLeadingComments(oneof.Location) {
-			g.P("//")
+		if oneof.Comments.Leading != "" {
+			g.P(oneof.Comments.Leading, "//")
 		}
 		g.P("// Types that are valid to be assigned to ", oneof.GoName, ":")
 		for _, field := range oneof.Fields {
-			g.PrintLeadingComments(field.Location)
-			g.P("//\t*", fieldOneofType(field))
+			g.P(field.Comments.Leading,
+				"//\t*", fieldOneofType(field))
 		}
 		g.Annotate(message.GoIdent.GoName+"."+oneof.GoName, oneof.Location)
 		g.P(oneof.GoName, " ", oneofInterfaceName(oneof), " `protobuf_oneof:\"", oneof.Desc.Name(), "\"`")
 		sf.append(oneof.GoName)
 		return
 	}
-	g.PrintLeadingComments(field.Location)
 	goType, pointer := fieldGoType(g, f, field)
 	if pointer {
 		goType = "*" + goType
@@ -482,7 +487,8 @@ func genMessageField(g *protogen.GeneratedFile, f *fileInfo, message *protogen.M
 		name = "XXX_weak_" + name
 	}
 	g.Annotate(message.GoIdent.GoName+"."+name, field.Location)
-	g.P(name, " ", goType, " `", strings.Join(tags, " "), "`",
+	g.P(field.Comments.Leading,
+		name, " ", goType, " `", strings.Join(tags, " "), "`",
 		deprecationComment(field.Desc.Options().(*descriptorpb.FieldOptions).GetDeprecated()))
 	sf.append(field.GoName)
 }

compiler/protogen/protogen.go

@@ -399,7 +399,7 @@ type File struct {
 	// of "dir/foo". Appending ".pb.go" produces an output file of "dir/foo.pb.go".
 	GeneratedFilenamePrefix string
 
-	sourceInfo map[pathKey][]*descriptorpb.SourceCodeInfo_Location
+	comments map[pathKey]CommentSet
 }
 
 func newFile(gen *Plugin, p *descriptorpb.FileDescriptorProto, packageName GoPackageName, importPath GoImportPath) (*File, error) {
@@ -415,7 +415,7 @@ func newFile(gen *Plugin, p *descriptorpb.FileDescriptorProto, packageName GoPac
 		Proto:         p,
 		GoPackageName: packageName,
 		GoImportPath:  importPath,
-		sourceInfo:    make(map[pathKey][]*descriptorpb.SourceCodeInfo_Location),
+		comments:      make(map[pathKey]CommentSet),
 	}
 
 	// Determine the prefix for generated Go files.
@@ -441,8 +441,17 @@ func newFile(gen *Plugin, p *descriptorpb.FileDescriptorProto, packageName GoPac
 	f.GeneratedFilenamePrefix = prefix
 
 	for _, loc := range p.GetSourceCodeInfo().GetLocation() {
-		key := newPathKey(loc.Path)
-		f.sourceInfo[key] = append(f.sourceInfo[key], loc)
+		// Descriptors declarations are guaranteed to have unique comment sets.
+		// Other locations may not be unique, but we don't use them.
+		var leadingDetached []Comments
+		for _, s := range loc.GetLeadingDetachedComments() {
+			leadingDetached = append(leadingDetached, Comments(s))
+		}
+		f.comments[newPathKey(loc.Path)] = CommentSet{
+			LeadingDetached: leadingDetached,
+			Leading:         Comments(loc.GetLeadingComments()),
+			Trailing:        Comments(loc.GetTrailingComments()),
+		}
 	}
 	for i, mdescs := 0, desc.Messages(); i < mdescs.Len(); i++ {
 		f.Messages = append(f.Messages, newMessage(gen, f, nil, mdescs.Get(i)))
@@ -514,6 +523,7 @@ type Message struct {
 	Enums      []*Enum      // nested enum declarations
 	Extensions []*Extension // nested extension declarations
 	Location   Location     // location of this message
+	Comments   CommentSet   // comments associated with this message
 }
 
 func newMessage(gen *Plugin, f *File, parent *Message, desc protoreflect.MessageDescriptor) *Message {
@@ -527,6 +537,7 @@ func newMessage(gen *Plugin, f *File, parent *Message, desc protoreflect.Message
 		Desc:     desc,
 		GoIdent:  newGoIdent(f, desc),
 		Location: loc,
+		Comments: f.comments[newPathKey(loc.Path)],
 	}
 	gen.messagesByName[desc.FullName()] = message
 	for i, mdescs := 0, desc.Messages(); i < mdescs.Len(); i++ {
@@ -625,12 +636,13 @@ type Field struct {
 	// '{{GoName}}' and a getter method named 'Get{{GoName}}'.
 	GoName string
 
-	Parent   *Message // message in which this field is defined; nil if top-level extension
-	Oneof    *Oneof   // containing oneof; nil if not part of a oneof
-	Extendee *Message // extended message for extension fields; nil otherwise
-	Enum     *Enum    // type for enum fields; nil otherwise
-	Message  *Message // type for message or group fields; nil otherwise
-	Location Location // location of this field
+	Parent   *Message   // message in which this field is defined; nil if top-level extension
+	Oneof    *Oneof     // containing oneof; nil if not part of a oneof
+	Extendee *Message   // extended message for extension fields; nil otherwise
+	Enum     *Enum      // type for enum fields; nil otherwise
+	Message  *Message   // type for message or group fields; nil otherwise
+	Location Location   // location of this field
+	Comments CommentSet // comments associated with this field
 }
 
 func newField(gen *Plugin, f *File, message *Message, desc protoreflect.FieldDescriptor) *Field {
@@ -648,6 +660,7 @@ func newField(gen *Plugin, f *File, message *Message, desc protoreflect.FieldDes
 		GoName:   camelCase(string(desc.Name())),
 		Parent:   message,
 		Location: loc,
+		Comments: f.comments[newPathKey(loc.Path)],
 	}
 	if desc.ContainingOneof() != nil {
 		field.Oneof = message.Oneofs[desc.ContainingOneof().Index()]
@@ -695,15 +708,18 @@ type Oneof struct {
 	Parent *Message // message in which this oneof occurs
 	Fields []*Field // fields that are part of this oneof
 
-	Location Location // location of this oneof
+	Location Location   // location of this oneof
+	Comments CommentSet // comments associated with this oneof
 }
 
 func newOneof(gen *Plugin, f *File, message *Message, desc protoreflect.OneofDescriptor) *Oneof {
+	loc := message.Location.appendPath(fieldnum.DescriptorProto_OneofDecl, int32(desc.Index()))
 	return &Oneof{
 		Desc:     desc,
 		Parent:   message,
 		GoName:   camelCase(string(desc.Name())),
-		Location: message.Location.appendPath(fieldnum.DescriptorProto_OneofDecl, int32(desc.Index())),
+		Location: loc,
+		Comments: f.comments[newPathKey(loc.Path)],
 	}
 }
 
@@ -720,7 +736,8 @@ type Enum struct {
 	GoIdent GoIdent      // name of the generated Go type
 	Values  []*EnumValue // enum values
 
-	Location Location // location of this enum
+	Location Location   // location of this enum
+	Comments CommentSet // comments associated with this enum
 }
 
 func newEnum(gen *Plugin, f *File, parent *Message, desc protoreflect.EnumDescriptor) *Enum {
@@ -734,6 +751,7 @@ func newEnum(gen *Plugin, f *File, parent *Message, desc protoreflect.EnumDescri
 		Desc:     desc,
 		GoIdent:  newGoIdent(f, desc),
 		Location: loc,
+		Comments: f.comments[newPathKey(loc.Path)],
 	}
 	gen.enumsByName[desc.FullName()] = enum
 	for i, evdescs := 0, enum.Desc.Values(); i < evdescs.Len(); i++ {
@@ -748,7 +766,8 @@ type EnumValue struct {
 
 	GoIdent GoIdent // name of the generated Go type
 
-	Location Location // location of this enum value
+	Location Location   // location of this enum value
+	Comments CommentSet // comments associated with this enum value
 }
 
 func newEnumValue(gen *Plugin, f *File, message *Message, enum *Enum, desc protoreflect.EnumValueDescriptor) *EnumValue {
@@ -761,10 +780,12 @@ func newEnumValue(gen *Plugin, f *File, message *Message, enum *Enum, desc proto
 		parentIdent = message.GoIdent
 	}
 	name := parentIdent.GoName + "_" + string(desc.Name())
+	loc := enum.Location.appendPath(fieldnum.EnumDescriptorProto_Value, int32(desc.Index()))
 	return &EnumValue{
 		Desc:     desc,
 		GoIdent:  f.GoImportPath.Ident(name),
-		Location: enum.Location.appendPath(fieldnum.EnumDescriptorProto_Value, int32(desc.Index())),
+		Location: loc,
+		Comments: f.comments[newPathKey(loc.Path)],
 	}
 }
 
@@ -775,14 +796,17 @@ type Service struct {
 	GoName  string
 	Methods []*Method // service method definitions
 
-	Location Location // location of this service
+	Location Location   // location of this service
+	Comments CommentSet // comments associated with this service
 }
 
 func newService(gen *Plugin, f *File, desc protoreflect.ServiceDescriptor) *Service {
+	loc := f.location(fieldnum.FileDescriptorProto_Service, int32(desc.Index()))
 	service := &Service{
 		Desc:     desc,
 		GoName:   camelCase(string(desc.Name())),
-		Location: f.location(fieldnum.FileDescriptorProto_Service, int32(desc.Index())),
+		Location: loc,
+		Comments: f.comments[newPathKey(loc.Path)],
 	}
 	for i, mdescs := 0, desc.Methods(); i < mdescs.Len(); i++ {
 		service.Methods = append(service.Methods, newMethod(gen, f, service, mdescs.Get(i)))
@@ -799,15 +823,18 @@ type Method struct {
 	Input  *Message
 	Output *Message
 
-	Location Location // location of this method
+	Location Location   // location of this method
+	Comments CommentSet // comments associated with this method
 }
 
 func newMethod(gen *Plugin, f *File, service *Service, desc protoreflect.MethodDescriptor) *Method {
+	loc := service.Location.appendPath(fieldnum.ServiceDescriptorProto_Method, int32(desc.Index()))
 	method := &Method{
 		Desc:     desc,
 		GoName:   camelCase(string(desc.Name())),
 		Parent:   service,
-		Location: service.Location.appendPath(fieldnum.ServiceDescriptorProto_Method, int32(desc.Index())),
+		Location: loc,
+		Comments: f.comments[newPathKey(loc.Path)],
 	}
 	return method
 }
@@ -882,29 +909,6 @@ func (g *GeneratedFile) P(v ...interface{}) {
 	fmt.Fprintln(&g.buf)
 }
 
-// PrintLeadingComments writes the comment appearing before a location in
-// the .proto source to the generated file.
-//
-// It returns true if a comment was present at the location.
-func (g *GeneratedFile) PrintLeadingComments(loc Location) (hasComment bool) {
-	f := g.gen.filesByName[loc.SourceFile]
-	if f == nil {
-		return false
-	}
-	for _, infoLoc := range f.sourceInfo[newPathKey(loc.Path)] {
-		if infoLoc.LeadingComments == nil {
-			continue
-		}
-		for _, line := range strings.Split(strings.TrimSuffix(infoLoc.GetLeadingComments(), "\n"), "\n") {
-			g.buf.WriteString("//")
-			g.buf.WriteString(line)
-			g.buf.WriteString("\n")
-		}
-		return true
-	}
-	return false
-}
-
 // QualifiedGoIdent returns the string to use for a Go identifier.
 //
 // If the identifier is from a different Go package than the generated file,
@@ -1161,3 +1165,30 @@ func newPathKey(idxPath []int32) pathKey {
 	}
 	return pathKey{string(buf)}
 }
+
+// CommentSet is a set of leading and trailing comments associated
+// with a .proto descriptor declaration.
+type CommentSet struct {
+	LeadingDetached []Comments
+	Leading         Comments
+	Trailing        Comments
+}
+
+// Comments is a comments string as provided by protoc.
+type Comments string
+
+// String formats the comments by inserting // to the start of each line,
+// ensuring that there is a trailing newline.
+// An empty comment is formatted as an empty string.
+func (c Comments) String() string {
+	if c == "" {
+		return ""
+	}
+	var b []byte
+	for _, line := range strings.Split(strings.TrimSuffix(string(c), "\n"), "\n") {
+		b = append(b, "//"...)
+		b = append(b, line...)
+		b = append(b, "\n"...)
+	}
+	return string(b)
+}