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

x/tools/gopls: resend diagnostics for changed files #54983

Closed
findleyr opened this issue Sep 9, 2022 · 2 comments
Closed

x/tools/gopls: resend diagnostics for changed files #54983

findleyr opened this issue Sep 9, 2022 · 2 comments
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@findleyr
Copy link
Contributor

findleyr commented Sep 9, 2022

In #50267, I added a (hidden) chattyDiagnostics option to gopls that publishes diagnostics on each change to a file, even if the diagnostics have not changed.

In practice, this doesn't seem to matter for VS Code has eliminated some cases where certain vim and emacs LSP clients get confused.

For example, from empirical observation coc.nvim seems to be subject to the following race. In pseudo-LSP (-> is client->server, <- is server->client):

1. -> didOpen(a.go)
2. <- publishDiagnostics(a.go, version 1, ["something bad"])
3. -> didChange(a.go) // fixes the diagnostic, creates version 2
4. <- publishDiagnostics(a.go, version 2, [])
5. -> didChange(a.go) // add whitespace, creates version 3
// gopls publishes nothing, because diagnostics didn't change

If the client observes (4) and (5) in the opposite order, it skips the diagnostics from 4 as they relate to a stale version. Then gopls never publishes another set of diagnostics (because it thinks the last published diagnostics are valid), and the client still shows "something bad".

Simply always publishing diagnostics after changes has fixed this problem, so we should make this the default behavior.

@findleyr findleyr added this to the gopls/v0.10.0 milestone Sep 9, 2022
@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Sep 9, 2022
@findleyr
Copy link
Contributor Author

findleyr commented Sep 9, 2022

I'll add:

  • an additional diagnostics notification following file changes does not increase LSP traffic significantly; there is already a lot of chatter after changes (e.g. new requests for codeLens)
  • our current behavior is probably incorrect according to the spec: "When a file changes it is the server’s responsibility to re-compute diagnostics and push them to the client." (spec)

@gopherbot
Copy link

Change https://go.dev/cl/429935 mentions this issue: gopls/internal/lsp/source: make "chatty" diagnostics the default

@golang golang locked and limited conversation to collaborators Sep 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

2 participants