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 <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>

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)

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)
-	s.publishReports(ctx, snapshot, reports, true, 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, false)
-	s.publishReports(ctx, snapshot, reports, true, true)
 }
 
-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,
+		// Reuse cached diagnostics and update the delivered map.
-		// only send them if we are publishing empty diagnostics.
+		if fileID.Version >= delivered.version && equalDiagnostics(delivered.sorted, diagnostics) {
-		if !ok && len(diagnostics) == 0 && !publishEmpty {
-			// Update the delivered map to cache the diagnostics.
 			s.delivered[fileID.URI] = toSend
 			continue
 		}
-		// Reuse equivalent cached diagnostics for subsequent file versions (if known),
+		// If we've already delivered diagnostics with analyses for this file, for this snapshot,
-		// or identical files (if versions are not known).
+		// at this version, do not send diagnostics without analyses.
-		if ok {
+		if delivered.snapshotID == toSend.snapshotID && delivered.version == toSend.version &&
-			geqVersion := fileID.Version >= delivered.version && delivered.version > 0
+			delivered.withAnalysis && !toSend.withAnalysis {
-			noVersions := (fileID.Version == 0 && delivered.version == 0) && delivered.identifier == fileID.Identifier
+			continue
-			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 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))

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 {

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",