Skip to content

Commit

Permalink
cmd/protoc-gen-go: improve generation of comments
Browse files Browse the repository at this point in the history 
The following improvements were made:
* All standalone comments above the "syntax" marker are preserved
similar to Java and some other generators.
* All standalone comments above the "package" marker are preserved
to be consistent with our former behavior.
* Leading comments are now generated for enums and extension fields.
* Single-line trailing comments are now generated for
enum values, message fields, and extension fields.
* The leading comments for each field that is part of a oneof are now
generated with the wrapper types rather than being shoved into the
comment for the oneof itself in an unreadable way.
* The deprecation marker is always generated as being above the declaration
rather than sometimes being an inlined comment.
* The deprecation marker is now properly generated for weak field setters.

Updates golang/protobuf#666

Change-Id: I7fd832dd4f86d15bfff70d7c22c6ba4934c05fcf
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/189238
Reviewed-by: Damien Neil <dneil@google.com>
  • Loading branch information
@dsnet
dsnet committed Aug 7, 2019
1 parent 70fdd5d commit 8d5e6d6
Show file tree
Hide file tree
Showing 83 changed files with 5,309 additions and 3,853 deletions.
4 changes: 4 additions & 0 deletions cmd/protoc-gen-go-grpc/testdata/grpc/deprecation.pb.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions cmd/protoc-gen-go-grpc/testdata/grpc/grpc.pb.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

160 changes: 105 additions & 55 deletions cmd/protoc-gen-go/internal_gengo/main.go
View file
Expand Up @@ -144,6 +144,8 @@ func GenerateFile(gen *protogen.Plugin, file *protogen.File) *protogen.Generated
}
}

genStandaloneComments(g, f, fieldnum.FileDescriptorProto_Syntax)

g.P("// Code generated by protoc-gen-go. DO NOT EDIT.")
if f.Proto.GetOptions().GetDeprecated() {
g.P("// ", f.Desc.Path(), " is a deprecated file.")
Expand All @@ -152,14 +154,8 @@ func GenerateFile(gen *protogen.Plugin, file *protogen.File) *protogen.Generated
}
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()
}
}
}
genStandaloneComments(g, f, fieldnum.FileDescriptorProto_Package)

g.P("package ", f.GoPackageName)
g.P()

Expand Down Expand Up @@ -196,6 +192,23 @@ func walkMessages(messages []*protogen.Message, f func(*protogen.Message)) {
}
}

// genStandaloneComments prints all leading comments for a FileDescriptorProto
// location identified by the field number n.
func genStandaloneComments(g *protogen.GeneratedFile, f *fileInfo, n int32) {
for _, loc := range f.Proto.GetSourceCodeInfo().GetLocation() {
if len(loc.Path) == 1 && loc.Path[0] == n {
for _, s := range loc.GetLeadingDetachedComments() {
g.P(protogen.Comments(s))
g.P()
}
if s := loc.GetLeadingComments(); s != "" {
g.P(protogen.Comments(s))
g.P()
}
}
}
}

func genImport(gen *protogen.Plugin, g *protogen.GeneratedFile, f *fileInfo, imp protoreflect.FileImport) {
impFile, ok := gen.FileByName(imp.Path())
if !ok {
Expand Down Expand Up @@ -249,7 +262,7 @@ func genImport(gen *protogen.Plugin, g *protogen.GeneratedFile, f *fileInfo, imp
}
g.P(tok, " ", name, " = ", impFile.GoImportPath.Ident(name))
}
g.P("// Symbols defined in public import of ", imp.Path())
g.P("// Symbols defined in public import of ", imp.Path(), ".")
g.P()
for _, decl := range astFile.Decls {
switch decl := decl.(type) {
Expand Down Expand Up @@ -279,17 +292,20 @@ 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.Annotate(enum.GoIdent.GoName, enum.Location)
g.P(enum.Comments.Leading,
"type ", enum.GoIdent, " int32",
deprecationComment(enum.Desc.Options().(*descriptorpb.EnumOptions).GetDeprecated()))
leadingComments := appendDeprecationSuffix(enum.Comments.Leading,
enum.Desc.Options().(*descriptorpb.EnumOptions).GetDeprecated())
g.P(leadingComments,
"type ", enum.GoIdent, " int32")

// Enum value constants.
g.P("const (")
for _, value := range enum.Values {
g.Annotate(value.GoIdent.GoName, value.Location)
g.P(value.Comments.Leading,
leadingComments := appendDeprecationSuffix(value.Comments.Leading,
value.Desc.Options().(*descriptorpb.EnumValueOptions).GetDeprecated())
g.P(leadingComments,
value.GoIdent, " ", enum.GoIdent, " = ", value.Desc.Number(),
deprecationComment(value.Desc.Options().(*descriptorpb.EnumValueOptions).GetDeprecated()))
trailingComment(value.Comments.Trailing))
}
g.P(")")
g.P()
Expand Down Expand Up @@ -381,15 +397,9 @@ func genMessage(gen *protogen.Plugin, g *protogen.GeneratedFile, f *fileInfo, me
}

// Message type declaration.
leadingComments := message.Comments.Leading
if message.Desc.Options().(*descriptorpb.MessageOptions).GetDeprecated() {
if leadingComments != "" {
g.P(leadingComments, "//")
leadingComments = "" // avoid printing them again later
}
g.P(deprecationComment(true))
}
g.Annotate(message.GoIdent.GoName, message.Location)
leadingComments := appendDeprecationSuffix(message.Comments.Leading,
message.Desc.Options().(*descriptorpb.MessageOptions).GetDeprecated())
g.P(leadingComments,
"type ", message.GoIdent, " struct {")
genMessageFields(g, f, message)
Expand Down Expand Up @@ -441,6 +451,9 @@ func genMessageInternalFields(g *protogen.GeneratedFile, message *protogen.Messa
sf.append("extensionFields")
}
}
if sf.count > 0 {
g.P()
}
}

func genMessageField(g *protogen.GeneratedFile, f *fileInfo, message *protogen.Message, field *protogen.Field, sf *structFields) {
Expand All @@ -452,16 +465,19 @@ func genMessageField(g *protogen.GeneratedFile, f *fileInfo, message *protogen.M
if oneof.Fields[0] != field {
return // only generate for first appearance
}
if oneof.Comments.Leading != "" {
g.P(oneof.Comments.Leading, "//")

g.Annotate(message.GoIdent.GoName+"."+oneof.GoName, oneof.Location)
leadingComments := oneof.Comments.Leading
if leadingComments != "" {
leadingComments += "\n"
}
g.P("// Types that are valid to be assigned to ", oneof.GoName, ":")
ss := []string{fmt.Sprintf(" Types that are assignable to %s:\n", oneof.GoName)}
for _, field := range oneof.Fields {
g.P(field.Comments.Leading,
"//\t*", fieldOneofType(field))
ss = append(ss, "\t*"+fieldOneofType(field).GoName+"\n")
}
g.Annotate(message.GoIdent.GoName+"."+oneof.GoName, oneof.Location)
g.P(oneof.GoName, " ", oneofInterfaceName(oneof), " `protobuf_oneof:\"", oneof.Desc.Name(), "\"`")
leadingComments += protogen.Comments(strings.Join(ss, ""))
g.P(leadingComments,
oneof.GoName, " ", oneofInterfaceName(oneof), " `protobuf_oneof:\"", oneof.Desc.Name(), "\"`")
sf.append(oneof.GoName)
return
}
Expand All @@ -487,9 +503,11 @@ func genMessageField(g *protogen.GeneratedFile, f *fileInfo, message *protogen.M
name = "XXX_weak_" + name
}
g.Annotate(message.GoIdent.GoName+"."+name, field.Location)
g.P(field.Comments.Leading,
leadingComments := appendDeprecationSuffix(field.Comments.Leading,
field.Desc.Options().(*descriptorpb.FieldOptions).GetDeprecated())
g.P(leadingComments,
name, " ", goType, " `", strings.Join(tags, " "), "`",
deprecationComment(field.Desc.Options().(*descriptorpb.FieldOptions).GetDeprecated()))
trailingComment(field.Comments.Trailing))
sf.append(field.GoName)
}

Expand Down Expand Up @@ -628,13 +646,12 @@ func genMessageGetterMethods(gen *protogen.Plugin, g *protogen.GeneratedFile, f
// Getter for message field.
goType, pointer := fieldGoType(g, f, field)
defaultValue := fieldDefaultValue(g, message, field)
if field.Desc.Options().(*descriptorpb.FieldOptions).GetDeprecated() {
g.P(deprecationComment(true))
}
g.Annotate(message.GoIdent.GoName+".Get"+field.GoName, field.Location)
leadingComments := appendDeprecationSuffix("",
field.Desc.Options().(*descriptorpb.FieldOptions).GetDeprecated())
switch {
case field.Desc.IsWeak():
g.P("func (x *", message.GoIdent, ") Get", field.GoName, "() ", protoifacePackage.Ident("MessageV1"), "{")
g.P(leadingComments, "func (x *", message.GoIdent, ") Get", field.GoName, "() ", protoifacePackage.Ident("MessageV1"), "{")
g.P("if x != nil {")
g.P("v := x.XXX_weak[", field.Desc.Number(), "]")
g.P("_ = x.XXX_weak_" + field.GoName) // for field tracking
Expand All @@ -645,14 +662,14 @@ func genMessageGetterMethods(gen *protogen.Plugin, g *protogen.GeneratedFile, f
g.P("return ", protoimplPackage.Ident("X"), ".WeakNil(", strconv.Quote(string(field.Message.Desc.FullName())), ")")
g.P("}")
case field.Oneof != nil:
g.P("func (x *", message.GoIdent, ") Get", field.GoName, "() ", goType, " {")
g.P(leadingComments, "func (x *", message.GoIdent, ") Get", field.GoName, "() ", goType, " {")
g.P("if x, ok := x.Get", field.Oneof.GoName, "().(*", fieldOneofType(field), "); ok {")
g.P("return x.", field.GoName)
g.P("}")
g.P("return ", defaultValue)
g.P("}")
default:
g.P("func (x *", message.GoIdent, ") Get", field.GoName, "() ", goType, " {")
g.P(leadingComments, "func (x *", message.GoIdent, ") Get", field.GoName, "() ", goType, " {")
if field.Desc.Syntax() == protoreflect.Proto3 || defaultValue == "nil" {
g.P("if x != nil {")
} else {
Expand All @@ -675,7 +692,9 @@ func genMessageSetterMethods(gen *protogen.Plugin, g *protogen.GeneratedFile, f
for _, field := range message.Fields {
if field.Desc.IsWeak() {
g.Annotate(message.GoIdent.GoName+".Set"+field.GoName, field.Location)
g.P("func (x *", message.GoIdent, ") Set", field.GoName, "(v ", protoifacePackage.Ident("MessageV1"), ") {")
leadingComments := appendDeprecationSuffix("",
field.Desc.Options().(*descriptorpb.FieldOptions).GetDeprecated())
g.P(leadingComments, "func (x *", message.GoIdent, ") Set", field.GoName, "(v ", protoifacePackage.Ident("MessageV1"), ") {")
g.P("if x.XXX_weak == nil {")
g.P("x.XXX_weak = make(", protoimplPackage.Ident("WeakFields"), ")")
g.P("}")
Expand Down Expand Up @@ -801,6 +820,7 @@ func genExtensions(gen *protogen.Plugin, g *protogen.GeneratedFile, f *fileInfo)
g.P("},")
}
g.P("}")
g.P()

// Group extensions by the target message.
var orderedTargets []protogen.GoIdent
Expand All @@ -818,17 +838,27 @@ func genExtensions(gen *protogen.Plugin, g *protogen.GeneratedFile, f *fileInfo)
g.P("// Extension fields to ", target, ".")
g.P("var (")
for _, extension := range allExtensionsByTarget[target] {
ed := extension.Desc
typeName := ed.Kind().String()
switch ed.Kind() {
xd := extension.Desc
typeName := xd.Kind().String()
switch xd.Kind() {
case protoreflect.EnumKind:
typeName = string(ed.Enum().FullName())
typeName = string(xd.Enum().FullName())
case protoreflect.MessageKind, protoreflect.GroupKind:
typeName = string(ed.Message().FullName())
typeName = string(xd.Message().FullName())
}
fieldName := string(xd.Name())

leadingComments := extension.Comments.Leading
if leadingComments != "" {
leadingComments += "\n"
}
fieldName := string(ed.Name())
g.P("// ", ed.Cardinality().String(), " ", typeName, " ", fieldName, " = ", ed.Number(), ";")
g.P(extensionVar(f.File, extension), " = &", extDescsVarName(f), "[", allExtensionsByPtr[extension], "]")
leadingComments += protogen.Comments(fmt.Sprintf(" %v %v %v = %v;\n",
xd.Cardinality(), typeName, fieldName, xd.Number()))
leadingComments = appendDeprecationSuffix(leadingComments,
extension.Desc.Options().(*descriptorpb.FieldOptions).GetDeprecated())
g.P(leadingComments,
extensionVar(f.File, extension), " = &", extDescsVarName(f), "[", allExtensionsByPtr[extension], "]",
trailingComment(extension.Comments.Trailing))
}
g.P(")")
g.P()
Expand All @@ -845,14 +875,6 @@ func extensionVar(f *protogen.File, extension *protogen.Extension) protogen.GoId
return f.GoImportPath.Ident(name)
}

// deprecationComment returns a standard deprecation comment if deprecated is true.
func deprecationComment(deprecated bool) string {
if !deprecated {
return ""
}
return "// Deprecated: Do not use."
}

// genOneofWrapperTypes generates the oneof wrapper types and
// associates the types with the parent message type.
func genOneofWrapperTypes(gen *protogen.Plugin, g *protogen.GeneratedFile, f *fileInfo, message *protogen.Message) {
Expand All @@ -871,7 +893,11 @@ func genOneofWrapperTypes(gen *protogen.Plugin, g *protogen.GeneratedFile, f *fi
tags := []string{
fmt.Sprintf("protobuf:%q", fieldProtobufTag(field)),
}
g.P(field.GoName, " ", goType, " `", strings.Join(tags, " "), "`")
leadingComments := appendDeprecationSuffix(field.Comments.Leading,
field.Desc.Options().(*descriptorpb.FieldOptions).GetDeprecated())
g.P(leadingComments,
field.GoName, " ", goType, " `", strings.Join(tags, " "), "`",
trailingComment(field.Comments.Trailing))
g.P("}")
g.P()
}
Expand Down Expand Up @@ -919,3 +945,27 @@ Loop:
return ident
}
}

// appendDeprecationSuffix optionally appends a deprecation notice as a suffix.
func appendDeprecationSuffix(prefix protogen.Comments, deprecated bool) protogen.Comments {
if !deprecated {
return prefix
}
if prefix != "" {
prefix += "\n"
}
return prefix + " Deprecated: Do not use.\n"
}

// trailingComment is like protogen.Comments, but lacks a trailing newline.
type trailingComment protogen.Comments

func (c trailingComment) String() string {
s := strings.TrimSuffix(protogen.Comments(c).String(), "\n")
if strings.Contains(s, "\n") {
// We don't support multi-lined trailing comments as it is unclear
// how to best render them in the generated code.
return ""
}
return s
}
11 changes: 8 additions & 3 deletions cmd/protoc-gen-go/testdata/annotations/annotations.pb.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion cmd/protoc-gen-go/testdata/annotations/annotations.pb.go.meta
View file
@@ -1 +1 @@
annotation:{path:5 path:0 source_file:"annotations/annotations.proto" begin:590 end:609} annotation:{path:5 path:0 path:2 path:0 source_file:"annotations/annotations.proto" begin:626 end:673} annotation:{path:4 path:0 source_file:"annotations/annotations.proto" begin:1953 end:1975} annotation:{path:4 path:0 path:2 path:0 source_file:"annotations/annotations.proto" begin:2119 end:2139} annotation:{path:4 path:0 path:2 path:0 source_file:"annotations/annotations.proto" begin:3033 end:3056}
annotation:{path:5 path:0 source_file:"annotations/annotations.proto" begin:750 end:769} annotation:{path:5 path:0 path:2 path:0 source_file:"annotations/annotations.proto" begin:786 end:833} annotation:{path:4 path:0 source_file:"annotations/annotations.proto" begin:2113 end:2135} annotation:{path:4 path:0 path:2 path:0 source_file:"annotations/annotations.proto" begin:2259 end:2279} annotation:{path:4 path:0 path:2 path:0 source_file:"annotations/annotations.proto" begin:3173 end:3196}

0 comments on commit 8d5e6d6

Please sign in to comment.