[tools] internal/lsp: always send diagnostics for updated files
Ernest Romero Climent (Gerrit)
Ernest Romero Climent has uploaded this change for review.
internal/lsp: always send diagnostics for updated files
According to the LSP specification, when a file is updated, it is the
server's responsibility to re-compute diagnostics and push them to the
client, even if the diagnostics for the updated file are the same as
before the update.
Currently gopls only sends diagnostics if the computed hash is updated,
however, that only occurs if the update adds or removes diagnostics or
updates any of their positions.
Change-Id: Ic8764efb26ff948a7148cfa45cf1d73069cbc27d
---
M gopls/internal/regtest/diagnostics/diagnostics_test.go
M internal/lsp/diagnostics.go
2 files changed, 41 insertions(+), 8 deletions(-)
diff --git a/gopls/internal/regtest/diagnostics/diagnostics_test.go b/gopls/internal/regtest/diagnostics/diagnostics_test.go
index 75ba7de..8b75c4d 100644
--- a/gopls/internal/regtest/diagnostics/diagnostics_test.go
+++ b/gopls/internal/regtest/diagnostics/diagnostics_test.go
@@ -127,6 +127,22 @@
})
}
+func TestDiagnosticResendOnEditAfterLastDiagnostic(t *testing.T) {
+ Run(t, badPackage, func(t *testing.T, env *Env) {
+ env.OpenFile("b.go")
+ env.Await(env.DiagnosticAtRegexp("b.go", "a = 2"))
+
+ // Edit after the last diagnostic so no diagnostic
+ // positions change after buffer was edited.
+ at := fake.Pos{Line: 2, Column: 11}
+ env.EditBuffer("b.go", fake.Edit{Start: at, End: at, Text: " "})
+ env.Await(OnceMet(
+ env.DoneWithChange(),
+ env.AnyDiagnosticAtCurrentVersion("b.go"),
+ ))
+ })
+}
+
func TestDiagnosticClearingOnDelete_Issue37049(t *testing.T) {
Run(t, badPackage, func(t *testing.T, env *Env) {
env.OpenFile("a.go")
@@ -380,7 +396,7 @@
// completed.
OnceMet(
env.DoneWithChange(),
- NoDiagnostics("a.go"),
+ EmptyDiagnostics("a.go"),
),
)
})
@@ -1703,7 +1719,7 @@
env.Await(
OnceMet(
env.DoneWithOpen(),
- NoDiagnostics("a/a.go"),
+ EmptyDiagnostics("a/a.go"),
),
NoOutstandingWork(),
)
diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go
index 0368771..21cb52f 100644
--- a/internal/lsp/diagnostics.go
+++ b/internal/lsp/diagnostics.go
@@ -10,6 +10,7 @@
"fmt"
"os"
"path/filepath"
+ "sort"
"strings"
"sync"
@@ -87,7 +88,7 @@
ctx := snapshot.BackgroundContext()
ctx = xcontext.Detach(ctx)
s.diagnose(ctx, snapshot, false)
- s.publishDiagnostics(ctx, true, snapshot)
+ s.publishDiagnostics(ctx, []span.URI{}, true, snapshot)
}
func (s *Server) diagnoseSnapshot(snapshot source.Snapshot, changedURIs []span.URI, onDisk bool) {
@@ -105,17 +106,17 @@
// The second phase does everything, and is debounced by the configured
// delay.
s.diagnoseChangedFiles(ctx, snapshot, changedURIs, onDisk)
- s.publishDiagnostics(ctx, false, snapshot)
+ s.publishDiagnostics(ctx, changedURIs, false, snapshot)
s.debouncer.debounce(snapshot.View().Name(), snapshot.ID(), delay, func() {
s.diagnose(ctx, snapshot, false)
- s.publishDiagnostics(ctx, true, snapshot)
+ s.publishDiagnostics(ctx, changedURIs, true, snapshot)
})
return
}
// Ignore possible workspace configuration warnings in the normal flow.
s.diagnose(ctx, snapshot, false)
- s.publishDiagnostics(ctx, true, snapshot)
+ s.publishDiagnostics(ctx, changedURIs, true, snapshot)
}
func (s *Server) diagnoseChangedFiles(ctx context.Context, snapshot source.Snapshot, uris []span.URI, onDisk bool) {
@@ -414,8 +415,21 @@
}
}
+func containsURI(uris []span.URI, u span.URI) bool {
+ i := sort.Search(len(uris), func(i int) bool {
+ return u <= uris[i]
+ })
+ return i < len(uris) && uris[i] == u
+}
+
+func sortURIs(uris []span.URI) {
+ sort.Slice(uris, func(i int, j int) bool {
+ return uris[i] < uris[j]
+ })
+}
+
// publishDiagnostics collects and publishes any unpublished diagnostic reports.
-func (s *Server) publishDiagnostics(ctx context.Context, final bool, snapshot source.Snapshot) {
+func (s *Server) publishDiagnostics(ctx context.Context, changedURIs []span.URI, final bool, snapshot source.Snapshot) {
ctx, done := event.Start(ctx, "Server.publishDiagnostics", tag.Snapshot.Of(snapshot.ID()))
defer done()
s.diagnosticsMu.Lock()
@@ -426,6 +440,8 @@
log.Trace.Logf(ctx, "published %d diagnostics", published)
}()
+ sortURIs(changedURIs)
+
for uri, r := range s.diagnostics {
// Snapshot IDs are always increasing, so we use them instead of file
// versions to create the correct order for diagnostics.
@@ -461,7 +477,8 @@
}
source.SortDiagnostics(diags)
hash := hashDiagnostics(diags...)
- if hash == r.publishedHash {
+ // Make sure that we re-send diagnostics for files that have been modified.
+ if hash == r.publishedHash && !containsURI(changedURIs, uri) {
// Update snapshotID to be the latest snapshot for which this diagnostic
// hash is valid.
r.snapshotID = snapshot.ID()
To view, visit change 298853. To unsubscribe, or for help writing mail filters, visit settings.
Rebecca Stambler (Gerrit)
Attention is currently required from: Ernest Romero Climent.
1 comment:
Commit Message:
According to the LSP specification, when a file is updated, it is the
server's responsibility to re-compute diagnostics and push them to the
client, even if the diagnostics for the updated file are the same as
before the update.
Do you mind sharing where it says this in the LSP specification? From https://microsoft.github.io/language-server-protocol/specifications/specification-3-17/#textDocument_publishDiagnostics:
When a file changes it is the server’s responsibility to re-compute diagnostics and push them to the client. If the computed set is empty it has to push the empty array to clear former diagnostics. Newly pushed diagnostics always replace previously pushed diagnostics. There is no merging that happens on the client side.
This only mentions clearing old diagnostics, not maintaining the same diagnostics.
To view, visit change 298853. To unsubscribe, or for help writing mail filters, visit settings.
Ernest Romero Climent (Gerrit)
Attention is currently required from: Rebecca Stambler.
1 comment:
Commit Message:
According to the LSP specification, when a file is updated, it is the
server's responsibility to re-compute diagnostics and push them to the
client, even if the diagnostics for the updated file are the same as
before the update.
Do you mind sharing where it says this in the LSP specification? From https://microsoft.github. […]
I didn't realize that this requirement could be up for interpretation.
To prevent attributes (location lists, etc.) from getting stale, editor implementations might clear them upon receiving an update to the buffer. Thus from an LSP client's standpoint, it's quite hard to know whether to wait for the next publishDiagnostics once an update to the buffer is detected or to send the last known diagnostics.
FWIW I've checked the behavior in other LSP servers (clangd, tsserver and rls) and all re-send the diagnostics for a file, if such file is updated, whether the update changed the diagnostics or not.
To view, visit change 298853. To unsubscribe, or for help writing mail filters, visit settings.
Rebecca Stambler (Gerrit)
Attention is currently required from: Ernest Romero Climent.
1 comment:
Commit Message:
According to the LSP specification, when a file is updated, it is the
server's responsibility to re-compute diagnostics and push them to the
client, even if the diagnostics for the updated file are the same as
before the update.
FWIW I've checked the behavior in other LSP servers (clangd, tsserver and rls) and all re-send the diagnostics for a file, if such file is updated, whether the update changed the diagnostics or not.
Other servers, like Microsoft's Python language server, and the Haskell language server, do not resend unchanged diagnostics, so I'm not sure that the spec is entirely clear here. I'm happy to file an issue to clear up this ambiguity, but I think clients should be able to handle both scenarios.
editor implementations might clear them upon receiving an update to the buffer.
If an editor clears diagnostics on updates to the buffer, but then the server just sends the same diagnostics, won't this look like "flickering" diagnostics to the user?
To view, visit change 298853. To unsubscribe, or for help writing mail filters, visit settings.
Rebecca Stambler (Gerrit)
Attention is currently required from: Ernest Romero Climent.
Patch set 1:Code-Review -2
1 comment:
Commit Message:
According to the LSP specification, when a file is updated, it is the
server's responsibility to re-compute diagnostics and push them to the
client, even if the diagnostics for the updated file are the same as
before the update.
> FWIW I've checked the behavior in other LSP servers (clangd, tsserver and rls) and all re-send the […]
Got confirmation from the LSP team that not sending unchanged diagnostics is OK: https://github.com/microsoft/language-server-protocol/issues/1217.
To view, visit change 298853. To unsubscribe, or for help writing mail filters, visit settings.
Ernest Romero Climent (Gerrit)
1 comment:
Patchset:
Thanks Rebecca for clearing the ambiguity with the LSP team. I will handle it on the client side.
To view, visit change 298853. To unsubscribe, or for help writing mail filters, visit settings.
Ernest Romero Climent (Gerrit)
Ernest Romero Climent abandoned this change.
To view, visit change 298853. To unsubscribe, or for help writing mail filters, visit settings.