gopls/internal/lsp/cmd: append, don't overwrite, in PublishDiagnostics

This change causes the CLI tool's PublishDiagnostics operation
to accumulate, rather than overwrite, the slice of file diagnostics,
under the hypothesis that it is receiving multiple events and
the later ones are clobbering the earlier ones, causing golang/go#59475.

We perform a crude de-duplication in case this should result
in duplicate diagnostics. A more robust approach using
textDocument/diagnostic will be added in a follow-up.

Also, clarify the mutex's responsibility, copy (don't alias)
the diagnostics slice in the critical section, and tidy up the
surrounding code.

Updates golang/go#59475

Change-Id: Ifbb4974ef00ab7bd6547de28f052cec86462230b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/494275
Run-TryBot: Alan Donovan <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>

Author: adonovan

gopls/internal/lsp/cmd/cmd.go

@@ -406,7 +406,7 @@ type cmdClient struct {
 	diagnosticsMu   sync.Mutex
 	diagnosticsDone chan struct{}
 
-	filesMu sync.Mutex
+	filesMu sync.Mutex // guards files map and each cmdFile.diagnostics
 	files   map[span.URI]*cmdFile
 }
 
@@ -518,6 +518,11 @@ func (c *cmdClient) ApplyEdit(ctx context.Context, p *protocol.ApplyWorkspaceEdi
 }
 
 func (c *cmdClient) PublishDiagnostics(ctx context.Context, p *protocol.PublishDiagnosticsParams) error {
+	var debug = os.Getenv(DebugSuggestedFixEnvVar) == "true"
+	if debug {
+		log.Printf("PublishDiagnostics URI=%v Diagnostics=%v", p.URI, p.Diagnostics)
+	}
+
 	if p.URI == "gopls://diagnostics-done" {
 		close(c.diagnosticsDone)
 	}
@@ -530,7 +535,24 @@ func (c *cmdClient) PublishDiagnostics(ctx context.Context, p *protocol.PublishD
 	defer c.filesMu.Unlock()
 
 	file := c.getFile(ctx, fileURI(p.URI))
-	file.diagnostics = p.Diagnostics
+	file.diagnostics = append(file.diagnostics, p.Diagnostics...)
+
+	// Perform a crude in-place deduplication.
+	// TODO(golang/go#60122): replace the ad-hoc gopls/diagnoseFiles
+	// non-standard request with support for textDocument/diagnostic,
+	// so that we don't need to do this de-duplication.
+	type key [5]interface{}
+	seen := make(map[key]bool)
+	out := file.diagnostics[:0]
+	for _, d := range file.diagnostics {
+		k := key{d.Range, d.Severity, d.Code, d.Source, d.Message}
+		if !seen[k] {
+			seen[k] = true
+			out = append(out, d)
+		}
+	}
+	file.diagnostics = out
+
 	return nil
 }
 

gopls/internal/lsp/cmd/suggested_fix.go

@@ -67,39 +67,38 @@ func (s *suggestedFix) Run(ctx context.Context, args ...string) error {
 	if err != nil {
 		return err
 	}
+	rng, err := file.mapper.SpanRange(from)
+	if err != nil {
+		return err
+	}
 
+	// Get diagnostics.
 	if err := conn.diagnoseFiles(ctx, []span.URI{uri}); err != nil {
 		return err
 	}
+	diagnostics := []protocol.Diagnostic{} // LSP wants non-nil slice
 	conn.Client.filesMu.Lock()
-	defer conn.Client.filesMu.Unlock()
+	diagnostics = append(diagnostics, file.diagnostics...)
+	conn.Client.filesMu.Unlock()
+	if debug {
+		log.Printf("file diagnostics: %#v", diagnostics)
+	}
 
+	// Request code actions
 	codeActionKinds := []protocol.CodeActionKind{protocol.QuickFix}
 	if len(args) > 1 {
 		codeActionKinds = []protocol.CodeActionKind{}
 		for _, k := range args[1:] {
 			codeActionKinds = append(codeActionKinds, protocol.CodeActionKind(k))
 		}
 	}
-
-	rng, err := file.mapper.SpanRange(from)
-	if err != nil {
-		return err
-	}
-	if file.diagnostics == nil {
-		// LSP requires a slice, not a nil.
-		file.diagnostics = []protocol.Diagnostic{}
-	}
-	if debug {
-		log.Printf("file diagnostics: %#v", file.diagnostics)
-	}
 	p := protocol.CodeActionParams{
 		TextDocument: protocol.TextDocumentIdentifier{
 			URI: protocol.URIFromSpanURI(uri),
 		},
 		Context: protocol.CodeActionContext{
 			Only:        codeActionKinds,
-			Diagnostics: file.diagnostics,
+			Diagnostics: diagnostics,
 		},
 		Range: rng,
 	}
@@ -111,6 +110,7 @@ func (s *suggestedFix) Run(ctx context.Context, args ...string) error {
 		log.Printf("code actions: %#v", actions)
 	}
 
+	// Gather edits from matching code actions.
 	var edits []protocol.TextEdit
 	for _, a := range actions {
 		if a.Command != nil {