internal/lsp: fix flickering analysis diagnostics
Also, change withAnalysis to includesAnalysis to make the name a bit
more accurate.
Fixes golang/go#42363
Change-Id: If50f7d36b953e6627ed9ba049357a2b86dc3f676
Reviewed-on: https://go-review.googlesource.com/c/tools/+/267817
Trust: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go
index 92f2e46..f878108 100644
--- a/internal/lsp/diagnostics.go
+++ b/internal/lsp/diagnostics.go
@@ -26,8 +26,8 @@
// idWithAnalysis is used to track if the diagnostics for a given file were
// computed with analyses.
type idWithAnalysis struct {
- id source.VersionedFileIdentity
- withAnalysis bool
+ id source.VersionedFileIdentity
+ includeAnalysis bool
}
// A reportSet collects diagnostics for publication, sorting them by file and
@@ -38,15 +38,15 @@
reports map[idWithAnalysis]map[string]*source.Diagnostic
}
-func (s *reportSet) add(id source.VersionedFileIdentity, withAnalysis bool, diags ...*source.Diagnostic) {
+func (s *reportSet) add(id source.VersionedFileIdentity, includeAnalysis bool, diags ...*source.Diagnostic) {
s.mu.Lock()
defer s.mu.Unlock()
if s.reports == nil {
s.reports = make(map[idWithAnalysis]map[string]*source.Diagnostic)
}
key := idWithAnalysis{
- id: id,
- withAnalysis: withAnalysis,
+ id: id,
+ includeAnalysis: includeAnalysis,
}
if _, ok := s.reports[key]; !ok {
s.reports[key] = map[string]*source.Diagnostic{}
@@ -79,7 +79,7 @@
// If a view has been created or the configuration changed, warn the user.
s.client.ShowMessage(ctx, shows)
}
- s.publishReports(ctx, snapshot, reports)
+ s.publishReports(ctx, snapshot, reports, false)
}
func (s *Server) diagnoseSnapshot(snapshot source.Snapshot, changedURIs []span.URI) {
@@ -99,17 +99,17 @@
event.Error(ctx, "diagnosing changed files", err)
}
}
- s.publishReports(ctx, snapshot, reports)
+ s.publishReports(ctx, snapshot, reports, true)
s.debouncer.debounce(snapshot.View().Name(), snapshot.ID(), delay, func() {
reports, _ := s.diagnose(ctx, snapshot, false)
- s.publishReports(ctx, snapshot, reports)
+ s.publishReports(ctx, snapshot, reports, false)
})
return
}
// Ignore possible workspace configuration warnings in the normal flow.
reports, _ := s.diagnose(ctx, snapshot, false)
- s.publishReports(ctx, snapshot, reports)
+ s.publishReports(ctx, snapshot, reports, false)
}
func (s *Server) diagnoseChangedFiles(ctx context.Context, snapshot source.Snapshot, uris []span.URI) (*reportSet, error) {
@@ -215,11 +215,11 @@
go func(pkg source.Package) {
defer wg.Done()
- withAnalysis := alwaysAnalyze // only run analyses for packages with open files
- var gcDetailsDir span.URI // find the package's optimization details, if available
+ includeAnalysis := alwaysAnalyze // only run analyses for packages with open files
+ var gcDetailsDir span.URI // find the package's optimization details, if available
for _, pgf := range pkg.CompiledGoFiles() {
if snapshot.IsOpen(pgf.URI) {
- withAnalysis = true
+ includeAnalysis = true
}
if gcDetailsDir == "" {
dirURI := span.URIFromPath(filepath.Dir(pgf.URI.Filename()))
@@ -232,7 +232,7 @@
}
}
- pkgReports, warn, err := source.Diagnostics(ctx, snapshot, pkg, withAnalysis)
+ pkgReports, warn, err := source.Diagnostics(ctx, snapshot, pkg, includeAnalysis)
// Check if might want to warn the user about their build configuration.
// Our caller decides whether to send the message.
@@ -251,7 +251,7 @@
// Add all reports to the global map, checking for duplicates.
for id, diags := range pkgReports {
- reports.add(id, withAnalysis, diags...)
+ reports.add(id, includeAnalysis, diags...)
}
// If gc optimization details are available, add them to the
// diagnostic reports.
@@ -261,7 +261,7 @@
event.Error(ctx, "warning: gc details", err, tag.Snapshot.Of(snapshot.ID()))
}
for id, diags := range gcReports {
- reports.add(id, withAnalysis, diags...)
+ reports.add(id, includeAnalysis, diags...)
}
}
}(pkg)
@@ -274,10 +274,10 @@
// Check if we already have diagnostic reports for the given file,
// meaning that we have already seen its package.
var seen bool
- for _, withAnalysis := range []bool{true, false} {
+ for _, includeAnalysis := range []bool{true, false} {
_, ok := reports.reports[idWithAnalysis{
- id: o.VersionedFileIdentity(),
- withAnalysis: withAnalysis,
+ id: o.VersionedFileIdentity(),
+ includeAnalysis: includeAnalysis,
}]
seen = seen || ok
}
@@ -349,7 +349,7 @@
return nil
}
-func (s *Server) publishReports(ctx context.Context, snapshot source.Snapshot, reports *reportSet) {
+func (s *Server) publishReports(ctx context.Context, snapshot source.Snapshot, reports *reportSet, isFirstPass bool) {
// Check for context cancellation before publishing diagnostics.
if ctx.Err() != nil || reports == nil {
return
@@ -370,10 +370,10 @@
}
source.SortDiagnostics(diagnostics)
toSend := sentDiagnostics{
- id: key.id,
- sorted: diagnostics,
- withAnalysis: key.withAnalysis,
- snapshotID: snapshot.ID(),
+ id: key.id,
+ sorted: diagnostics,
+ includeAnalysis: key.includeAnalysis,
+ snapshotID: snapshot.ID(),
}
// We use the zero values if this is an unknown file.
@@ -382,10 +382,11 @@
// Snapshot IDs are always increasing, so we use them instead of file
// versions to create the correct order for diagnostics.
- // If we've already delivered diagnostics for a future snapshot for this file,
- // do not deliver them.
+ // If we've already delivered diagnostics for a future snapshot for
+ // this file, do not deliver them.
if delivered.snapshotID > toSend.snapshotID {
- // Do not update the delivered map since it already contains newer diagnostics.
+ // Do not update the delivered map since it already contains newer
+ // diagnostics.
continue
}
@@ -399,10 +400,18 @@
// If we've already delivered diagnostics for this file, at this
// snapshot, with analyses, do not send diagnostics without analyses.
if delivered.snapshotID == toSend.snapshotID && delivered.id == toSend.id &&
- delivered.withAnalysis && !toSend.withAnalysis {
+ delivered.includeAnalysis && !toSend.includeAnalysis {
// Do not update the delivered map since it already contains better diagnostics.
continue
}
+
+ // If we've previously delivered non-empty diagnostics and this is a
+ // first diagnostic pass, wait for a subsequent pass to complete before
+ // sending empty diagnostics to avoid flickering diagnostics.
+ if isFirstPass && delivered.includeAnalysis && !toSend.includeAnalysis && len(toSend.sorted) == 0 {
+ continue
+ }
+
if err := s.client.PublishDiagnostics(ctx, &protocol.PublishDiagnosticsParams{
Diagnostics: toProtocolDiagnostics(diagnostics),
URI: protocol.URIFromSpanURI(key.id.URI),
diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go
index aa73f6c..0303491 100644
--- a/internal/lsp/lsp_test.go
+++ b/internal/lsp/lsp_test.go
@@ -1159,7 +1159,7 @@
// Always run diagnostics with analysis.
reports, _ := r.server.diagnose(r.ctx, snapshot, true)
- r.server.publishReports(r.ctx, snapshot, reports)
+ r.server.publishReports(r.ctx, snapshot, reports, false)
for uri, sent := range r.server.delivered {
var diagnostics []*source.Diagnostic
for _, d := range sent.sorted {
diff --git a/internal/lsp/server.go b/internal/lsp/server.go
index 058ffbc..c2f5004 100644
--- a/internal/lsp/server.go
+++ b/internal/lsp/server.go
@@ -107,10 +107,10 @@
// sentDiagnostics is used to cache diagnostics that have been sent for a given file.
type sentDiagnostics struct {
- id source.VersionedFileIdentity
- sorted []*source.Diagnostic
- withAnalysis bool
- snapshotID uint64
+ id source.VersionedFileIdentity
+ sorted []*source.Diagnostic
+ includeAnalysis bool
+ snapshotID uint64
}
func (s *Server) workDoneProgressCancel(ctx context.Context, params *protocol.WorkDoneProgressCancelParams) error {
