go/doc: drop //go:* comments from extracted docs

[rsc]

Today we write things like:

//go:noinline

// F does whatever it does.
func F()

instead of the more natural:

// F does whatever it does.
//go:noinline
func F()

because the latter produces:

$ go doc F

func F()
    F does whatever it does. go:noinline

Now that we have the //go: convention, let's fix go/doc to leave them out automatically,
and then we can write things the more natural way without cluttering docs.

I'm happy to do the work.

/cc @griesemer

[gopherbot]

Change https://golang.org/cl/224737 mentions this issue: go/ast: drop //directive comments from doc.Text

[rsc]

https://golang.org/cl/224737 implements the change, for reference.

[rsc]

Adding to the minutes for the first time this week, but it seems headed for a likely accept based on the reactions so far.

[benjaminjkraft]

Would this also apply to other machine-readable comments (not prefixed with go:)? We have run into similar issues with golangci-lint's //nolint comments.

[as]

Would this also apply to other machine-readable comments (not prefixed with go:)? We have run into similar issues with golangci-lint's //nolint comments.

I don't think go doc should be aware of, or handle the syntax of third-party linters. This problem might be solvable by deleting the comments and using grep -v in your CI pipeline instead to exclude specific checks you don't want.

[zikaeroh]

I don't think go doc should be aware of, or handle the syntax of third-party linters. This problem might be solvable by deleting the comments and using grep -v in your CI pipeline instead to exclude specific checks you don't want.

That's a very rudimentary way to ignore linter messages that doesn't scale. Having them be explicitly in the code is very common (tslint/eslint ignores, JetBrain's style ignores, others) and means they're version controllable, configurable per-instance, available in the editor (gopls has staticcheck built in!), and so on. I wouldn't suggest changing how a CI system checks linter messages just to fix generated documentation including unwanted comments. That'd be backwards.

One thing I will say is that comments without a space after // from my experience are usually the machine-readable ones, like Go's //go:generate, golangci-lint's //nolint, staticcheck's //lint:ignore, counterfeiter's //counterfeiter:generate, and more. Perhaps comments without a space after // should become the convention instead. That'd be a different proposal.

[benjaminjkraft]

It wouldn't have any awareness of the linter comment syntax, just that // comments with no leading space are machine-readable, which I understood to be a general convention (although I can't now find any documentation that says as much).

[rsc]

I implemented //[^ ] gets ignored, but there are comments without spaces that just omit them. There are even tests for //TODO, //NOTE, //BUG, etc. There's one Example where the output has no space between // and output.

Right now I have //line (with trailing space) or //go:.
We could replace //go: with //[a-z0-9]+:[a-z0-9].
I will try that.

Are there any objections to doing either of these options?

[zikaeroh]

I ran the following in my mod cache:

$ rg -g '*.go' -I '^//[a-z0-9]+:[a-z0-9]' | sort -u

This was the output: https://gist.github.com/zikaeroh/463b4c8bda2c602587c60d450acbc1a5

Everything there looks good to me. I don't see any false positives (besides two //0: and //1: comments, from https://github.com/denverdino/aliyungo/blob/master/dm/mail.go, which are not in a documentation comment).

I think that regex is likely to be enough. The only thing I'd consider is adding _ and - to those character sets, but when I add those and diff the results, no additional comments are added, so maybe it's not a big deal.

But, my cache is only a subset of projects.

[dmitshur]

We could replace //go: with //[a-z0-9]+:[a-z0-9].

What is the motivation to replace the "go" part with [a-z0-9]+?

[griesemer]

@dmitshur There may be directives for other commands. See the examples given here.

[benjaminjkraft]

Ideally I would have gone for ^//[^ ] or ^//[a-z0-9] or something broader, since e.g. //nolint (no colon) is also in use in some places, and that's what the cmd/compile docs say is a directive. But a grep similar to @zikaeroh's makes that seem like a bad idea: plenty of projects seem to start ordinary doc comments without a space. (A couple arbitrary examples from my mod cache: glfw gomega.) I like //[a-z0-9]+:[a-z0-9] as a compromise.

[zikaeroh]

since e.g. //nolint (no colon) is also in use in some places

Yeah, that's unfortunate, though I believe in those cases you can put the //nolint on the same line as the declaration instead, like:

// SomeFunc does something.
func SomeFunc() { //nolint
    ...
}

And I believe at least golangci-lint would accept that. Not sure past that.

and that's what the cmd/compile docs say is a directive.

It's a shame go/doc didn't omit "directive" style comments from docs from the start, but it's too late for that... I'd have preferred everyone put a space after their // and I'm really surprised there are so many without one. :slightly_frowning_face:

[rsc]

There was some discussion in response to my question in https://github.com/golang/go/issues/37974#issuecomment-611088006, but I don't see any objections.

Based on the discussion, then, this seems like a likely accept.

[rsc]

No change in consensus, so accepted.

[nd]

should the isDirective check also handle //export comments (https://golang.org/cmd/cgo/#hdr-C_references_to_Go)?

[ianlancetaylor]

@nd Would you mind opening a new issue for that? Thanks.

[nd]

Filed #38785.