golang / protobuf

Go support for Google's protocol buffers
BSD 3-Clause "New" or "Revised" License
9.76k stars 1.58k forks source link

--go_out generates a file that fails Golint's package comment check. #1456

Closed deeglaze closed 2 years ago

deeglaze commented 2 years ago
$ cat > test.proto <<EOF
syntax = "proto3";

// Package foo is a test package.
package foo;

option go_package = "github.com/example/foo";

message Bar {}
EOF
$ protoc --go_out=paths=source_relative:. test.proto
$ head test.pb.go
// Code generated by protoc-gen-go. DO NOT EDIT.
// versions:
//      protoc-gen-go v1.27.1
//      protoc        v3.12.4
// source: test.proto

// Package foo is a test package.

package foo

What did you expect to see?

// Code generated by protoc-gen-go. DO NOT EDIT.
// versions:
//      protoc-gen-go v1.27.1
//      protoc        v3.12.4
// source: test.proto

// Package foo is a test package.
package foo

The newline inserted by genStandaloneComments for the package causes the "Package foo" comment to be detached from the package statement, so Golint will error stating that the file should have a package comment.

The option go_package is may be more appropriate for attaching the comment to, since the proto package may be different from the Go package, but proto-gen-go doesn't pick that statement out specially.

Anything else we should know about your project / environment?

This is for a golang project straddling google3 and github, so I generate the .pb.go files internally before pushing out. It leads to golint errors every change/recompile. Minor, but an annoyance.

neild commented 2 years ago

The recommended way to include Go package-level documentation for a protobuf package is to include a doc.go file containing the package documentation.

I don't know the full history of the decision not to attach comments from the .proto package statement to the .pb.go one.

Before the rewrite of protoc-gen-go to use protogen, it placed package comments after the Go package line:

// This package contains interesting messages.
package some.protobuf.test;
package test

/*
This package contains interesting messages.
*/

I suspect there was a deliberate decision to not attach package comments from the .proto file to the .pb.go package doc comment, because protobuf files do not follow Go doc comment conventions. It's common to have several .proto files in a package, each with their own per-file documentation; including these comments in the Go doc comment would run them all together in a confusing fasion.

In addition, there's no convention in .proto files to attach package-level documentation to a package statement., meaning that it's likely that file-level documentation in a .proto file will be missed anyway.

However, I wasn't there when that choice was made, and I'm not certain. When we rewrote protoc-gen-go, we moved the package comment up to above the package statement, but deliberately chose not to attach it, both to avoid changing the behavior of the generator and for the above reasons.

deeglaze commented 2 years ago

Can we have a flag to set a package comment? I'm just trying to satisfy the linter.

neild commented 2 years ago

You can put a hand-written doc.go file in the package containing the package comment and nothing else. It doesn't need to go in a .pb.go file.

puellanivis commented 2 years ago

Yeah, same thing as neild, that comment has no reason to conform to Go documentation convention, but I also don’t seem to recall any reason why to put it ahead of the package document, rather than after. Perhaps so that it would break linting in precisely this situation? If so, it’s unfortunately that we don’t have some sort of warning about it, and how to resolve the issue with the suggested “doc.go” convention… but then it seems like this is something that could only be tested at golint time.

stapelberg commented 2 years ago

In general, you shouldn’t lint generated code. Can you exclude your .pb.go files from golint?

You’re mentioning that your project spans google3 and protobuf. In google, we don’t lint generated code and you should use a go_proto_library rule instead of importing .pb.go files.

If disabling the linter is not a possibility (why?), generate a doc.go on your end to work around the issue.

puellanivis commented 2 years ago

Indeed, golint should by default ignore any file that contains a line that starts with "// Code generated " and ends with " DO NOT EDIT." https://github.com/golang/lint/blob/master/lint.go#L118-L121

What’s possible here is that you’re using a progressive linter, that is only linting the code that is touched during a commit, (I have seen this) and since that code-generated line is the same every time you generate it, it’s being excluded from the changes to lint, and thus causing the file to be linted.

I’ve found myself quite unimpressed with the linters that the community tends to collect around, either just because it’s there, or it “has the most linter checks!”