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: add tags or codes to diagnostics #34508

Closed
stamblerre opened this issue Sep 24, 2019 · 11 comments
Closed

proposal: x/tools/go/analysis: add tags or codes to diagnostics #34508

stamblerre opened this issue Sep 24, 2019 · 11 comments
Labels
FrozenDueToAge Proposal Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@stamblerre
Copy link
Contributor

Version 3.15 of the Language Server Protocol will be adding tags to the Diagnostic. These will allow us to specify types of diagnostics, in particular unnecessary and deprecated code. To avoid our having to parse diagnostic messages for gopls, go/analysis should return these details along with diagnostics.

@gopherbot gopherbot added this to the Unreleased milestone Sep 24, 2019
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 24, 2019
@bcmills bcmills changed the title x/tools/go/analysis: proposal: add tags or codes to diagnostics proposal: x/tools/go/analysis: add tags or codes to diagnostics Sep 26, 2019
@rsc rsc added this to Incoming in Proposals (old) Dec 4, 2019
@rsc
Copy link
Contributor

rsc commented Apr 8, 2020

/cc @matloob @ianthehat

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

matloob commented Apr 8, 2020

I'm a bit wary of adding this to the analysis framework because it's specific to the LSP protocol. "unneccessary" and "deprecated" only come from specific analyses right?

If all those analyses are in tools, maybe they can populate a field of a type defined in internal/lsp so we don't need to introduce a new API? Would that work?

@ianthehat
Copy link

I was thinking about this before (we need the same thing for severity) and my conclusion was that severity should be per analyzer, not per result, which means gopls can add the tag based on the analyzer that generated the diagnostic rather than having to add it to the analysis framework. I think all analyzers can be consistent about their severity level (and if they can't it is easy to split an analyzer into a core that produces facts and multiple analyzers that use those facts to produce varying severity levels of diagnostic), and I think the same probably holds for tags.

@matloob
Copy link
Contributor

matloob commented Apr 9, 2020

Oh that's even better! Rebecca. What do you think? Is that reasonable?

@stamblerre
Copy link
Contributor Author

Sounds good to me - thanks!

@matloob
Copy link
Contributor

matloob commented Apr 10, 2020

Okay, I'll close this issue in favor of that solution then.

@matloob matloob closed this as completed Apr 10, 2020
@rsc rsc moved this from Active to Declined in Proposals (old) Apr 15, 2020
@mvdan
Copy link
Member

mvdan commented Apr 26, 2020

Could we document what Ian said in #34508 (comment)? I have a few tools which are already analyzers, and some others which I have to convert still, and this is very relevant for some of them. I vaguely remembered this conversation, but it was hard to find it (or its result) because the godoc makes no mention or hint about analyzer severity.

@matloob
Copy link
Contributor

matloob commented Apr 27, 2020

You mean to add a line to the go/analysis package documentation saying as such, or do you mean documenting it somewhere else?

@mvdan
Copy link
Member

mvdan commented Apr 27, 2020

Yeah, I did mean go/analysis - that's the first documentation page I checked, at least.

@gopherbot
Copy link

Change https://golang.org/cl/230312 mentions this issue: go/analysis: suggest to users how to associate diagnostics with severities

@matloob
Copy link
Contributor

matloob commented Apr 27, 2020

@mvdan Added a paragraph to the go/analysis docs on golang.org/cl/230312. Want to take a look and see if that works for you?

gopherbot pushed a commit to golang/tools that referenced this issue Jun 5, 2020
…ities

Add to the go/analysis docs the suggestion made by Ian Cottrell that diagnostic
severities should be classified by analyzer.

Updates golang/go#34508

Change-Id: I9a75fb1400269ece32c9ca52afbf6c7975d3e205
Reviewed-on: https://go-review.googlesource.com/c/tools/+/230312
Run-TryBot: Michael Matloob <matloob@golang.org>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Apr 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge Proposal Tools This label describes issues relating to any tools in the x/tools repository.
Projects
No open projects
Development

No branches or pull requests

6 participants