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

proposal: x/tools/go/analysis: new godoc analyzer #63929

Open
Antonboom opened this issue Nov 3, 2023 · 4 comments
Open

proposal: x/tools/go/analysis: new godoc analyzer #63929

Antonboom opened this issue Nov 3, 2023 · 4 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) Proposal Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@Antonboom
Copy link

Antonboom commented Nov 3, 2023

Hello!

I need to know if the Go team is interested in having Go Doc checker in x/tools or if we implement it in a third party module.
Go Doc comments is common place for issues (especially after recent HTML support) and I thought maybe Gopls interested in it.

Example of checks:

https://go.dev/doc/comment#packages

Before:

// path implements utility routines for manipulating slash-separated paths.

After:

// Package path implements utility routines for manipulating slash-separated paths.

Or typo:

// Package path implements utility routines for manipulating slash-separated paths.
package patch

https://go.dev/doc/comment#links

Before:

// Package json implements encoding and decoding of JSON as defined in
// https://tools.ietf.org/html/rfc7159. The mapping between JSON and Go values is described
// in the documentation for the Marshal and Unmarshal functions.
//
// For an introduction to this package, see the article
// https://golang.org/doc/articles/json_and_go.html
package json

After:

// Package json implements encoding and decoding of JSON as defined in
// [RFC 7159]. The mapping between JSON and Go values is described
// in the documentation for the Marshal and Unmarshal functions.
//
// For an introduction to this package, see the article
// “[JSON and Go].”
//
// [RFC 7159]: https://tools.ietf.org/html/rfc7159
// [JSON and Go]: https://golang.org/doc/articles/json_and_go.html
package json

(+ check URL formatting)

And so on and so forth.


Related:


P.S. Based on golangci/golangci-lint#4147
(cc) @500poundbear

@gopherbot gopherbot added this to the Proposal milestone Nov 3, 2023
@seankhliao seankhliao added Tools This label describes issues relating to any tools in the x/tools repository. Analysis Issues related to static analysis (vet, x/tools/go/analysis) labels Nov 15, 2023
@timothy-king
Copy link
Contributor

timothy-king commented Nov 21, 2023

Checks that enforce comments to be correct "Doc Comments" seem plausible as a cmd/vet (or gopls) check IMO. If we have a strong reason to suspect that a comment is a doc comment and it is a 'buggy' comment, that will cause an incorrect rendering of the documentation for the package. I think this is on the right side of the vet correctness criteria. (In this case it is the execution of the documentation programs on the package.)

cmd/vet should not be too opinionated about non-"Doc Comments". So I think a lot of these will hinge on having enough context to decide if something is intended to be a "Doc Comment". I don't think there is sufficient context from just this:

// path implements ...

I do think there is sufficient context once this is the comment above a package declaration and the first word matches the package name:

// path implements ...
package path // warning

I also think there is sufficient context to warn on that the word after "Package" does not match the package name:

// Package path implements utility routines for manipulating slash-separated paths.
package patch // warning

From https://go.dev/doc/comment#links :

Plain text that is recognized as a URL is automatically linked in HTML renderings.

IIUC this correctly the links given are are style issues not correctness. That is not to say they are not valuable checks. Just not a fit for vet (IMO).

Or typo:

I am not really sure what other typos you have in mind. Regardless, it will need to be typos according to some dictionary (system dictionary, then personal dictionaries, etc.) I am not really sure this a good match for cmd/vet, which tends towards zero config. This may be okay for gopls. @findleyr ?

@seankhliao
Copy link
Member

I think I already see the first one in gopls via staticcheck's ST1000. @dominikh placed this as a style check, maybe not strong enough to fail a vet check for?

@Antonboom
Copy link
Author

Antonboom commented Dec 8, 2023

If we have a strong reason to suspect that a comment is a doc comment and it is a 'buggy' comment, that will cause an incorrect rendering of the documentation for the package

Exactly! And this is true for links and other HTML.

I am not really sure what other typos you have in mind.

E.g. the common error – do not change documentation after function name changing

// WasClosed reports whether the connection was closed.
func (c *conn) Closed() bool

Of course there is some analysis of godoc checker's scope needed.
I thought it is the next step after decision "yes, the nice idea, we accept it".

maybe not strong enough to fail a vet check for

I'm not sure that go vet should rely on the opinions of third-party linters. Moreover, @timothy-king explained above why this is not about style. Added ST1000 to "Related" section, thanks!

@adonovan
Copy link
Member

This proposal overlaps with #57963, which is specifically about broken doc links.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) Proposal Tools This label describes issues relating to any tools in the x/tools repository.
Projects
Status: Incoming
Development

No branches or pull requests

5 participants