From 97cd989a767277f2e36b1f79cc635b38fdc98ada Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Fri, 10 Jan 2020 12:29:37 -0500 Subject: [PATCH] internal/lsp: remove boolean for publishEmpty in diagnostics This parameter was added to avoid sending a slew of empty diagnostics for the initial workspace load. It's actually not needed anymore, as we can just detect if we have previously sent diagnostics for the given file, and if not we shouldn't send an empty diagnostic. This is true for all cases, not just the initial workspace load. Change-Id: I34a323bc0c0df79edd39630cd25577238b256266 Reviewed-on: https://go-review.googlesource.com/c/tools/+/214287 Run-TryBot: Rebecca Stambler TryBot-Result: Gobot Gobot Reviewed-by: Heschi Kreinick --- internal/lsp/cmd/check.go | 3 +++ internal/lsp/diagnostics.go | 44 +++++++++++++------------------------ internal/lsp/lsp_test.go | 4 ++-- internal/lsp/server.go | 23 ++++++++++++++++++- 4 files changed, 42 insertions(+), 32 deletions(-) diff --git a/internal/lsp/cmd/check.go b/internal/lsp/cmd/check.go index 384a3239b5..33310535f1 100644 --- a/internal/lsp/cmd/check.go +++ b/internal/lsp/cmd/check.go @@ -59,6 +59,9 @@ func (c *check) Run(ctx context.Context, args ...string) error { if err := conn.diagnoseFiles(ctx, uris); err != nil { return err } + conn.Client.filesMu.Lock() + defer conn.Client.filesMu.Unlock() + for _, file := range checking { for _, d := range file.diagnostics { spn, err := file.mapper.RangeSpan(d.Range) diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index c3ce41cb30..2bcf640b09 100644 --- a/internal/lsp/diagnostics.go +++ b/internal/lsp/diagnostics.go @@ -32,7 +32,7 @@ func (s *Server) diagnoseSnapshot(ctx context.Context, snapshot source.Snapshot) log.Error(ctx, "diagnoseSnapshot: no diagnostics", err, telemetry.Package.Of(ph.ID())) return } - s.publishReports(ctx, snapshot, reports, false, false) + s.publishReports(ctx, snapshot, reports, false) }(ph) } // Run diagnostics on the go.mod file. @@ -60,8 +60,7 @@ func (s *Server) diagnoseFile(snapshot source.Snapshot, fh source.FileHandle) { } return } - // Publish empty diagnostics for files. - s.publishReports(ctx, snapshot, reports, true, true) + s.publishReports(ctx, snapshot, reports, true) } func (s *Server) diagnoseModfile(snapshot source.Snapshot) { @@ -76,11 +75,10 @@ func (s *Server) diagnoseModfile(snapshot source.Snapshot) { } return } - // Publish empty diagnostics for files. - s.publishReports(ctx, snapshot, reports, true, true) + s.publishReports(ctx, snapshot, reports, false) } -func (s *Server) publishReports(ctx context.Context, snapshot source.Snapshot, reports map[source.FileIdentity][]source.Diagnostic, publishEmpty, withAnalysis bool) { +func (s *Server) publishReports(ctx context.Context, snapshot source.Snapshot, reports map[source.FileIdentity][]source.Diagnostic, withAnalysis bool) { // Check for context cancellation before publishing diagnostics. if ctx.Err() != nil { return @@ -104,34 +102,22 @@ func (s *Server) publishReports(ctx context.Context, snapshot source.Snapshot, r withAnalysis: withAnalysis, snapshotID: snapshot.ID(), } - delivered, ok := s.delivered[fileID.URI] + // We use the zero values if this is an unknown file. + delivered := s.delivered[fileID.URI] - // If diagnostics are empty and not previously delivered, - // only send them if we are publishing empty diagnostics. - if !ok && len(diagnostics) == 0 && !publishEmpty { - // Update the delivered map to cache the diagnostics. + // Reuse cached diagnostics and update the delivered map. + if fileID.Version >= delivered.version && equalDiagnostics(delivered.sorted, diagnostics) { s.delivered[fileID.URI] = toSend continue } - // Reuse equivalent cached diagnostics for subsequent file versions (if known), - // or identical files (if versions are not known). - if ok { - geqVersion := fileID.Version >= delivered.version && delivered.version > 0 - noVersions := (fileID.Version == 0 && delivered.version == 0) && delivered.identifier == fileID.Identifier - if (geqVersion || noVersions) && equalDiagnostics(delivered.sorted, toSend.sorted) { - // Update the delivered map even if we reuse cached diagnostics. - s.delivered[fileID.URI] = toSend - continue - } - // If we've already delivered diagnostics with analyses for this file, for this snapshot, - // at this version, do not send diagnostics without analyses. - if delivered.snapshotID == toSend.snapshotID && delivered.version == toSend.version && - delivered.withAnalysis && !toSend.withAnalysis { - continue - } + // If we've already delivered diagnostics with analyses for this file, for this snapshot, + // at this version, do not send diagnostics without analyses. + if delivered.snapshotID == toSend.snapshotID && delivered.version == toSend.version && + delivered.withAnalysis && !toSend.withAnalysis { + continue } if err := s.client.PublishDiagnostics(ctx, &protocol.PublishDiagnosticsParams{ - Diagnostics: toProtocolDiagnostics(ctx, diagnostics), + Diagnostics: toProtocolDiagnostics(diagnostics), URI: protocol.NewURI(fileID.URI), Version: fileID.Version, }); err != nil { @@ -157,7 +143,7 @@ func equalDiagnostics(a, b []source.Diagnostic) bool { return true } -func toProtocolDiagnostics(ctx context.Context, diagnostics []source.Diagnostic) []protocol.Diagnostic { +func toProtocolDiagnostics(diagnostics []source.Diagnostic) []protocol.Diagnostic { reports := []protocol.Diagnostic{} for _, diag := range diagnostics { related := make([]protocol.DiagnosticRelatedInformation, 0, len(diag.Related)) diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go index f2b5008a94..060cf584c1 100644 --- a/internal/lsp/lsp_test.go +++ b/internal/lsp/lsp_test.go @@ -357,7 +357,7 @@ func (r *runner) SuggestedFix(t *testing.T, spn span.Span) { }, Context: protocol.CodeActionContext{ Only: []protocol.CodeActionKind{protocol.QuickFix}, - Diagnostics: toProtocolDiagnostics(r.ctx, diagnostics[fileID]), + Diagnostics: toProtocolDiagnostics(diagnostics[fileID]), }, }) if err != nil { @@ -979,7 +979,7 @@ func TestModfileSuggestedFixes(t *testing.T) { }, Context: protocol.CodeActionContext{ Only: []protocol.CodeActionKind{protocol.SourceOrganizeImports}, - Diagnostics: toProtocolDiagnostics(ctx, diags), + Diagnostics: toProtocolDiagnostics(diags), }, }) if err != nil { diff --git a/internal/lsp/server.go b/internal/lsp/server.go index 417efe7ce8..d279012b7a 100644 --- a/internal/lsp/server.go +++ b/internal/lsp/server.go @@ -15,6 +15,7 @@ import ( "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/lsp/source" "golang.org/x/tools/internal/span" + errors "golang.org/x/xerrors" ) // NewClientServer @@ -299,7 +300,27 @@ func (s *Server) NonstandardRequest(ctx context.Context, method string, params i if err != nil { return nil, err } - s.diagnoseFile(view.Snapshot(), view.Session().GetFile(span.URI(uri))) + snapshot := view.Snapshot() + fh, err := snapshot.GetFile(uri) + if err != nil { + return nil, err + } + reports, _, err := source.FileDiagnostics(ctx, view.Snapshot(), fh, true, nil) + if err != nil { + return nil, err + } + fileID := fh.Identity() + diagnostics, ok := reports[fileID] + if !ok { + return nil, errors.Errorf("no diagnostics for %s", uri) + } + if err := s.client.PublishDiagnostics(ctx, &protocol.PublishDiagnosticsParams{ + URI: protocol.NewURI(uri), + Diagnostics: toProtocolDiagnostics(diagnostics), + Version: fileID.Version, + }); err != nil { + return nil, err + } } if err := s.client.PublishDiagnostics(ctx, &protocol.PublishDiagnosticsParams{ URI: "gopls://diagnostics-done",