Add tags to LSP diagnostics #4311
Conversation
Also removes the diagnostic.Data from the new diagnostics, which AFAICS is unused; we can save ourselves the json marshaling. As a part of this, added a first pass at adding tests for diagnostics; we don't currently have any. These are a bit different because we haven't implemented [PullDiagnostics][1] (we can add this, but we'd need a custom implementation, or wait until [go.lsp.dev upgrades to 3.17][2]). Instead, we intercept the notifications from the server. Lastly, since we don't actually want the tests to wait around, adds synctest and only runs the tests on Go 1.25 for now (1.26 should be released next month and we'll be able to upgrade to 1.25 as our minimum). This is in preparation to eventually landing relatedInformation (other spans) in diagnostics, from bufbuild/protocompile#659. Also pulls in the latest protocompile@main to bring in the new deprecated tag. [1]: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_pullDiagnostics [2]: go-language-server/protocol#52
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
| stream, | ||
| queryExecutor, | ||
| ) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
just simplified; Serve will start running in the background, so we don't need the extra goroutine.
| // We serialize the bytes into a string before providing the structure to diagnostic.Data | ||
| // because diagnostic.Data is an interface{}, which is treated as a JSON "any", which | ||
| // will not cleanly deserialize. | ||
| diagnostic.Data = string(bytes) |
There was a problem hiding this comment.
we aren't using this data; it's just included so it can be passed back to us in a subsequent codeAction (ref: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#diagnostic). I think we can leave it off, for now.
| if annotation.Type() == "IMPORT_USED" { | ||
| diagnostic.Tags = []protocol.DiagnosticTag{ | ||
| protocol.DiagnosticTagUnnecessary, | ||
| } | ||
| } |
There was a problem hiding this comment.
this is the old diagnostic from the lint server that we're currently using; the const is here (unexported); if it changes out from under us, the test will catch it, but I suspect we're not going to change anything in that package in the near future.
| // TODO: Use Tags with a protocol.CompletionItemTagDeprecated if the client supports tags. | ||
| Deprecated: isDeprecated, | ||
| Deprecated: isDeprecated, | ||
| Tags: []protocol.SymbolTag{ | ||
| protocol.SymbolTagDeprecated, | ||
| }, |
There was a problem hiding this comment.
I still think we ought to have a more generalized idea of client capabilities, but it doesn't seem like a bad idea to just populate both of these values; figured I'd nix the TODO.
|
|
||
| package diagnostics.v1; | ||
|
|
||
| import "google/protobuf/timestamp.proto"; |
There was a problem hiding this comment.
rendered in grey, from the "unnecessary" tag:
Also removes the diagnostic.Data from the new diagnostics, which AFAICS is unused; we can save ourselves the json marshaling.
As a part of this, added a first pass at adding tests for diagnostics; we don't currently have any. These are a bit different because we haven't implemented PullDiagnostics (we can add this, but we'd need a custom implementation, or wait until go.lsp.dev upgrades to 3.17). Instead, we intercept the notifications from the server. Lastly, since we don't actually want the tests to wait around, adds synctest and only runs the tests on Go 1.25 for now (1.26 should be released next month and we'll be able to upgrade to 1.25 as our minimum).
This is in preparation to eventually landing relatedInformation (other spans) in diagnostics, from bufbuild/protocompile#659.
Also pulls in the latest protocompile@main to bring in the new deprecated tag.