Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
rsc opened this issue Mar 20, 2020 · 18 comments
Closed

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

rsc opened this issue Mar 20, 2020 · 18 comments

Comments

@rsc
Copy link
Contributor

rsc commented Mar 20, 2020

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

@rsc rsc added this to the Proposal milestone Mar 20, 2020
@rsc rsc added this to Incoming in Proposals (old) Mar 20, 2020
@gopherbot
Copy link

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

@rsc
Copy link
Contributor Author

rsc commented Mar 21, 2020

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

@rsc rsc moved this from Incoming to Active in Proposals (old) Apr 1, 2020
@rsc
Copy link
Contributor Author

rsc commented Apr 1, 2020

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
Copy link

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
Copy link
Contributor

as commented Apr 1, 2020

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
Copy link
Contributor

zikaeroh commented Apr 1, 2020

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
Copy link

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
Copy link
Contributor Author

rsc commented Apr 8, 2020

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
Copy link
Contributor

zikaeroh commented Apr 8, 2020

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
Copy link
Contributor

dmitshur commented Apr 8, 2020

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
Copy link
Contributor

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

@benjaminjkraft
Copy link

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
Copy link
Contributor

zikaeroh commented Apr 9, 2020

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. 🙁

@rsc
Copy link
Contributor Author

rsc commented Apr 15, 2020

There was some discussion in response to my question in #37974 (comment), but I don't see any objections.

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

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Apr 15, 2020
@rsc
Copy link
Contributor Author

rsc commented Apr 22, 2020

No change in consensus, so accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Apr 22, 2020
@rsc rsc changed the title proposal: go/doc: drop //go:* comments from extracted docs go/doc: drop //go:* comments from extracted docs Apr 22, 2020
@rsc rsc modified the milestones: Proposal, Go1.15 Apr 22, 2020
@nd
Copy link
Contributor

nd commented Apr 30, 2020

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

@ianlancetaylor
Copy link
Contributor

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

@nd
Copy link
Contributor

nd commented May 1, 2020

Filed #38785.

xujianhai666 pushed a commit to xujianhai666/go-1 that referenced this issue May 21, 2020
This allows writing

	// F does a thing.
	//go:noinline
	func F()

without the //go:noinline or other directive (such as //line)
ending up looking like extra words in the doc comment.

Fixes golang#37974.

Change-Id: Ic738d72802cc2fa448f7633915e7126d2f76d8ca
Reviewed-on: https://go-review.googlesource.com/c/go/+/224737
Reviewed-by: Robert Griesemer <gri@golang.org>
sykesm added a commit to sykesm/fabric that referenced this issue Aug 18, 2020
It's been a long time coming but the ast package is now treating
directives as a special case and removing them from comment groups.

See golang/go#37974

Signed-off-by: Matthew Sykes <sykesmat@us.ibm.com>
caod123 pushed a commit to hyperledger/fabric that referenced this issue Aug 18, 2020
It's been a long time coming but the ast package is now treating
directives as a special case and removing them from comment groups.

See golang/go#37974

Signed-off-by: Matthew Sykes <sykesmat@us.ibm.com>
sykesm added a commit to sykesm/fabric that referenced this issue Feb 10, 2021
It's been a long time coming but the ast package is now treating
directives as a special case and removing them from comment groups.

See golang/go#37974

Signed-off-by: Matthew Sykes <sykesmat@us.ibm.com>
jyellick pushed a commit to hyperledger/fabric that referenced this issue Feb 15, 2021
It's been a long time coming but the ast package is now treating
directives as a special case and removing them from comment groups.

See golang/go#37974

Signed-off-by: Matthew Sykes <sykesmat@us.ibm.com>
@golang golang locked and limited conversation to collaborators May 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

9 participants