x/tools/go/analysis: add Analyzer.URL and Diagnostic.URL fields

[findleyr]

This issue proposes the following additions to x/tools/go/analysis

type Analyzer struct {
  // URL holds an optional link to a web page with additional documentation for this analyzer.
  // This value will be used as the default value of Diagnostic.URL for diagnostics produced via
  // the Pass.Reportf API.
  URL string
  //...
}

type Diagnostic struct {
  // URL holds an optional link to a web page with additional documentation for this diagnostic.
  URL string
  //...
}

Background: #57903 reverted an ad-hoc URL formatted in the printf analyzer diagnostic message, as it adds unnecessary verbosity and is inconsistent with other vet errors. However, this URL was added based on evidence that some users are confused about how to fix printf formatting verb mismatches.

There is inherent tension in error messages because while we want vet errors to be as concise as possible, some users need more information to understand the error, particularly the first time they see it.

Adding a URL field to analyzers and diagnostics would allow tools to serve the user additional documentation in whatever form is most appropriate. For example, cmd/vet could have a flag that controls this information (-url?), and gopls could set the LSP codeDescription.

CC @rsc @adonovan @timothy-king @stapelberg @dominikh

[rsc]

I'd make the vet flag -url.

[findleyr]

@rsc

I'd make the vet flag -url.

Sure, updated the issue description, but this proposal is just for the go/analysis change. The change to cmd/vet seems likely to warrant much more discussion, and I don't want to bundle that discussion with the go/analysis change (which seems uncontroversial to me).

[timothy-king]

I am not a huge fan of not printing urls by default and printing only when -url is given. The default should try to be helpful instead of machine readable/backwards compliant. The json output is what folks should use for machine readable output. But I agree the cmd/vet change is somewhat separate to whether a URL field should be added.

[timothy-king]

I have seen several Analyzers that smuggle URLs into the Message field or use Category to imply a URL. There is a decent amount of demand for this information.

[findleyr]

I have seen several Analyzers that smuggle URLs into the Message field or use Category to imply a URL. There is a decent amount of demand for this information.

That reminds me of one possible counter-proposal: we could instead add a "Code" field, which could then be interpreted by a separate system to produce a URL -- this is effectively what we do for the hidden error codes in go/types.Error. However that seems unnecessarily indirect, and nothing is preventing us from also adding a code later on.

[rsc]

If we do codes then some other system has to come back and find out the code-to-URL mapping, and you have code pages and all that. I'd much rather do URLs. Unless the code is a short readable string that you prefix with "https://go.dev/e/" or something like that. But since there can be analyzers not written by the Go team that wouldn't work anyway. Reusing the URL name space makes sense here (much as it does for import paths).

I have no problem with the vet part being a separate discussion.

[findleyr]

If we do codes then some other system has to come back and find out the code-to-URL mapping, and you have code pages and all that.

Yep, I agree. URLs have a clear meaning (unlike Code), and are easy to use. Just wanted to mention the counter-proposal for the sake of completeness.

[timothy-king]

If we add a URL field what I would like to do for all of the x/tools/go/analysis/passes is to add a URL to point to a page for each analyzer. This could be a dedicated page like https://github.com/golang/tools/blob/master/gopls/doc/analyzers.md or https://staticcheck.io/docs/checks. It could also be the package documentation on pkg.go.dev. Those are good places for examples or more context like the URL links like the "https://pkg.go.dev/fmt#hdr-Printing" that kicked off this discussion.

[adonovan]

Should we add an Analyzer.URL field as well, with documentation about the analyzer and perhaps its diagnostic categories in subsections? Then failure to set the Diagnostic.URL for each call to Reportf could be treated by each driver as if the Analyzer.URL was specified (or possibly, Analyzer.URI + "#" + Diagnostic.Category). This would allow a single-line change to the analyzer source to deliver most of the benefit.

(There's obviously a bit of redundancy in the likely outcome that some users would specify for the Analyzer.URI a link to the pkg.go.dev or googlesource.com page for its own source, which contains its Doc constant declaration, but I think that's ok.)

[timothy-king]

@adonovan I don't think we will change the signature of https://pkg.go.dev/golang.org/x/tools/go/analysis#Pass.Reportf . That is a pretty good reason to go with an Analyzer.URL as this lets us derive a good default URL from:

func (pass *Pass) Reportf(pos token.Pos, format string, args ...interface{}) {

If Analyzer.URL is not given, we can give an empty url by default, so this will be backwards compatible with old Category values. (Old values might not be suitable for URLs.) No Category would just be the Analyzer.URL. Pass.Report is still fully general. Maybe we will want a Reportf variant that includes a Category now?

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group