I'd make the vet flag -url.

@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).

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.

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.

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.

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.

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.

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.

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

@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?

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

@adonovan @timothy-king Analyzer.URL makes a lot of sense to me -- nice. Added to the proposal.

This seems simple and helpful. Have all concerns about this proposal been addressed?

Any thoughts on the subsections idea? In other words, should we specify Analyzer URL something like this:

type Analyzer struct {
  // URL specifies the location of documentation for this analyzer.
  URL string
}

type Diagnostic struct {
  // URL specifies the location of documentation for this diagnostic.
  // It is optional. If empty, the driver will instead use the Analyzer.URL,
  // and if the Diagnostic's Category field is non-empty, it will be used as
  // the URL fragment, replacing any already present.
  URL string
}

This allows the Analyzer's HTML page to document various categories of errors, each in its own section, without requiring analyzers to set this field.

Nitpick: I think the documentation for Diagnostic should just be something like // URL specifies the location of documentation for this diagnostic. (Optional). I think the documentation for Reportf [and similar] should explain how they populate the field. The fields themselves can be flexible.

Nitpick: I think the documentation for Diagnostic should just be something like // URL specifies the location of documentation for this diagnostic. (Optional). I think the documentation for Reportf [and similar] should explain how they populate the field. The fields themselves can be flexible.

Reportf cannot implement the computation I described in the comment above because it doesn't have access to the diagnostic Category. Nor should it: it's just a convenience function that makes it easy to set the common fields; it is not really part of the contract between driver and analyzer. The point of putting this interpretation of the empty URL in the contract is that every analyzer would benefit, even those that use their own function instead of Reportf, without the need for invasive changes.

Hard-coding that if both URLs are set the Diagnostic.URL is a fragment seems fragile.
We could instead say that Diagnostic.URL is interpreted relative to Analyzer.URL when the first is a relative URL and the second is set (using https://pkg.go.dev/net/url#URL.ResolveReference).

Hard-coding that if both URLs are set the Diagnostic.URL is a fragment seems fragile.

I was suggesting that the Category should be interpreted as a fragment if the Diagnostic URL is empty.

I think that once the Diagnostic.URL is set it should not be further related to any other value. This is a value we will serialize and send to other places. In this spirit, pass.Report can just report the Diagnostic as given.

I still think the documentation should just be on Reportf for how it is populated. Maybe we need to make it easier to include a Category? Either a new special verb for Reportf or a new function.

I think that once the Diagnostic.URL is set it should not be further related to any other value. This is a value we will serialize and send to other places. In this spirit, pass.Report can just report the Diagnostic as given.

The Diagnostic is always interpreted by the driver before it can be serialized. For example, it contains token.Pos values, which only make sense w.r.t. a FileSet that is not part of the Diagnostic itself. So there's precedent for interpreting the Category field in the way I proposed.

I still think the documentation should just be on Reportf for how it is populated. Maybe we need to make it easier to include a Category? Either a new special verb for Reportf or a new function.

We can always add new Reportf-like convenience functions, but the point of my fragment proposal was to ensure that existing programs all do something sensible with the URL field, whether they use Reportf or construct Diagnostics directly. For programs that use Reportf, our only choice is to say that an empty Diagnostics.URL field should be interpreted as if equal to Analyzer.URL. For programs that construct Diagnostics directly (e.g. to set the Category field), the fragment allows them to provide a single page that documents each kind of finding in a different section.

Personally I'm not convinced we need more Reportf-like functions because it's so easy for users to do it themselves.

P.S. Category was conceived as something suitable for a URL fragment:

// An Analyzer may return a variety of diagnostics; the optional Category,
// which should be a constant, may be used to classify them.
// It is primarily intended to make it easy to look up documentation.

nit: I think the comment for Category need update and be more explicit about the use as uri fragment - so people shouldn't use strings that aren't acceptable as URL fragments (e.g. space) unless go/analysis package is doing extra encoding.

@adonovan, you mean something like this?

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

type Diagnostic struct {
  // URL holds an optional link to a web page with additional documentation for this diagnostic.
  // If omitted, documentation should be provided at `Analyzer.URL + "#" + Diagnostic.Category`.
  URL string
  //...
}

That makes sense to me.

Why overload Category this way? Why not just let Diagnostic.URL be set to "#foo"?

Why overload Category this way? Why not just let Diagnostic.URL be set to "#foo"?

So that existing Analyzers that set the Category field (but not the new URL field) work without modification.

It sounds like we are at:

• Analyzer.URL is the URL for the overall analyzer
• Diagnostic.URL is the URL for this diagnostic, relative to Analyzer.URL (using https://pkg.go.dev/net/url#URL.ResolveReference).
• If unset, Diagnostic.URL defaults to "#"+Diagnostic.Category

Do I have that right?

Have all remaining concerns been addressed?

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

Change https://go.dev/cl/474935 mentions this issue: go/analysis: ExtractDoc unifies Analyzer.Doc and package doc comment

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

Change https://go.dev/cl/476615 mentions this issue: go/analysis: add Analyser.URL and Diagnostic.URI fields

Change https://go.dev/cl/485435 mentions this issue: go/analysis/checker/internal: resolve Diagnostic.URLs