From 56eb7d2c19ba6b05ace11fab7ac8d090edd31eed Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Thu, 21 Nov 2019 01:24:43 -0500 Subject: [PATCH] internal/lsp: don't resend diagnostics if they are unchanged Cache delivered diagnostics on the server so that we can determine if they should be resent. To be careful about this, we only reuse cached diagnostics if they are for a greater version, or if we don't know the file's version and it is unchanged. Fixes golang/go#32443 Change-Id: I4ba22d85e5b21a8ad6cc62f74cd83c07d3c220cf Reviewed-on: https://go-review.googlesource.com/c/tools/+/208261 Run-TryBot: Rebecca Stambler TryBot-Result: Gobot Gobot Reviewed-by: Heschi Kreinick --- internal/lsp/diagnostics.go | 39 +++++++++++++++++++++++++++++++ internal/lsp/lsp_test.go | 3 ++- internal/lsp/server.go | 20 +++++++++++++--- internal/lsp/source/util.go | 20 ++++++++++++++++ internal/lsp/tests/diagnostics.go | 25 ++------------------ 5 files changed, 80 insertions(+), 27 deletions(-) diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index 7a709da1a5..22f056e4a4 100644 --- a/internal/lsp/diagnostics.go +++ b/internal/lsp/diagnostics.go @@ -90,6 +90,9 @@ func (s *Server) publishReports(ctx context.Context, reports map[source.FileIden return } + s.deliveredMu.Lock() + defer s.deliveredMu.Unlock() + for fileID, diagnostics := range reports { // Don't deliver diagnostics if the context has already been canceled. if ctx.Err() != nil { @@ -99,6 +102,26 @@ func (s *Server) publishReports(ctx context.Context, reports map[source.FileIden if len(diagnostics) == 0 && !publishEmpty { continue } + // Pre-sort diagnostics to avoid extra work when we compare them. + source.SortDiagnostics(diagnostics) + toSend := sentDiagnostics{ + version: fileID.Version, + identifier: fileID.Identifier, + sorted: diagnostics, + } + + if delivered, ok := s.delivered[fileID.URI]; ok { + // We only reuse cached diagnostics in two cases: + // 1. This file is at a greater version than that of the previously sent diagnostics. + // 2. There are no known versions for the file. + greaterVersion := fileID.Version > delivered.version && delivered.version > 0 + noVersions := (fileID.Version == 0 && delivered.version == 0) && delivered.identifier == fileID.Identifier + if (greaterVersion || noVersions) && equalDiagnostics(delivered.sorted, diagnostics) { + // Update the delivered map even if we reuse cached diagnostics. + s.delivered[fileID.URI] = toSend + continue + } + } if err := s.client.PublishDiagnostics(ctx, &protocol.PublishDiagnosticsParams{ Diagnostics: toProtocolDiagnostics(ctx, diagnostics), URI: protocol.NewURI(fileID.URI), @@ -107,9 +130,25 @@ func (s *Server) publishReports(ctx context.Context, reports map[source.FileIden log.Error(ctx, "failed to deliver diagnostic", err, telemetry.File) continue } + // Update the delivered map. + s.delivered[fileID.URI] = toSend } } +// equalDiagnostics returns true if the 2 lists of diagnostics are equal. +// It assumes that both a and b are already sorted. +func equalDiagnostics(a, b []source.Diagnostic) bool { + if len(a) != len(b) { + return false + } + for i := 0; i < len(a); i++ { + if source.CompareDiagnostic(a[i], b[i]) != 0 { + return false + } + } + return true +} + func toProtocolDiagnostics(ctx context.Context, diagnostics []source.Diagnostic) []protocol.Diagnostic { reports := []protocol.Diagnostic{} for _, diag := range diagnostics { diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go index 3ff4642d80..664279e6c9 100644 --- a/internal/lsp/lsp_test.go +++ b/internal/lsp/lsp_test.go @@ -73,7 +73,8 @@ func testLSP(t *testing.T, exporter packagestest.Exporter) { } r := &runner{ server: &Server{ - session: session, + session: session, + delivered: map[span.URI]sentDiagnostics{}, }, data: data, ctx: ctx, diff --git a/internal/lsp/server.go b/internal/lsp/server.go index e388f71b05..e500b48212 100644 --- a/internal/lsp/server.go +++ b/internal/lsp/server.go @@ -21,15 +21,18 @@ import ( func NewClientServer(ctx context.Context, cache source.Cache, client protocol.Client) (context.Context, *Server) { ctx = protocol.WithClient(ctx, client) return ctx, &Server{ - client: client, - session: cache.NewSession(ctx), + client: client, + session: cache.NewSession(ctx), + delivered: make(map[span.URI]sentDiagnostics), } } // NewServer starts an LSP server on the supplied stream, and waits until the // stream is closed. func NewServer(ctx context.Context, cache source.Cache, stream jsonrpc2.Stream) (context.Context, *Server) { - s := &Server{} + s := &Server{ + delivered: make(map[span.URI]sentDiagnostics), + } ctx, s.Conn, s.client = protocol.NewServer(ctx, stream, s) s.session = cache.NewSession(ctx) return ctx, s @@ -85,6 +88,17 @@ type Server struct { // folders is only valid between initialize and initialized, and holds the // set of folders to build views for when we are ready pendingFolders []protocol.WorkspaceFolder + + // delivered is a cache of the diagnostics that the server has sent. + deliveredMu sync.Mutex + delivered map[span.URI]sentDiagnostics +} + +// sentDiagnostics is used to cache diagnostics that have been sent for a given file. +type sentDiagnostics struct { + version float64 + identifier string + sorted []source.Diagnostic } // General diff --git a/internal/lsp/source/util.go b/internal/lsp/source/util.go index a8d7ad21b7..f9b321f50f 100644 --- a/internal/lsp/source/util.go +++ b/internal/lsp/source/util.go @@ -13,6 +13,7 @@ import ( "go/types" "path/filepath" "regexp" + "sort" "strings" "golang.org/x/tools/internal/lsp/protocol" @@ -590,3 +591,22 @@ func formatFunction(params []string, results []string, writeResultParens bool) s return detail.String() } + +func SortDiagnostics(d []Diagnostic) { + sort.Slice(d, func(i int, j int) bool { + return CompareDiagnostic(d[i], d[j]) < 0 + }) +} + +func CompareDiagnostic(a, b Diagnostic) int { + if r := protocol.CompareRange(a.Range, b.Range); r != 0 { + return r + } + if a.Message < b.Message { + return -1 + } + if a.Message == b.Message { + return 0 + } + return 1 +} diff --git a/internal/lsp/tests/diagnostics.go b/internal/lsp/tests/diagnostics.go index 6b0c1cf1c5..192cde40f5 100644 --- a/internal/lsp/tests/diagnostics.go +++ b/internal/lsp/tests/diagnostics.go @@ -3,7 +3,6 @@ package tests import ( "bytes" "fmt" - "sort" "strings" "golang.org/x/tools/internal/lsp/protocol" @@ -14,8 +13,8 @@ import ( // DiffDiagnostics prints the diff between expected and actual diagnostics test // results. func DiffDiagnostics(uri span.URI, want, got []source.Diagnostic) string { - sortDiagnostics(want) - sortDiagnostics(got) + source.SortDiagnostics(want) + source.SortDiagnostics(got) if len(got) != len(want) { return summarizeDiagnostics(-1, uri, want, got, "different lengths got %v want %v", len(got), len(want)) @@ -47,26 +46,6 @@ func DiffDiagnostics(uri span.URI, want, got []source.Diagnostic) string { return "" } -func sortDiagnostics(d []source.Diagnostic) { - sort.Slice(d, func(i int, j int) bool { - return compareDiagnostic(d[i], d[j]) < 0 - }) -} - -func compareDiagnostic(a, b source.Diagnostic) int { - if r := protocol.CompareRange(a.Range, b.Range); r != 0 { - return r - } - if a.Message < b.Message { - return -1 - } - if a.Message == b.Message { - return 0 - } else { - return 1 - } -} - func summarizeDiagnostics(i int, uri span.URI, want []source.Diagnostic, got []source.Diagnostic, reason string, args ...interface{}) string { msg := &bytes.Buffer{} fmt.Fprint(msg, "diagnostics failed")