internal/lsp: don't publish duplicate diagnostics in one report

CL 242579 changed the mechanism of reporting diagnostics so that we now
combine reports from multiple sources. Previously, we overwrote existing
diagnostics if new ones were reported. This was fine because
source.Diagnostics generated all of the diagnostics for Go files, and
mod.Diagnostics generated all of the diagnostics for go.mod files.

Now, we combine diagnostics from both sources -- mod.Diagnostics can
generate reports for Go files. We may get duplicate reports for packages
with test variants, so now, we check for those and dedupe.

Change-Id: I42e98079b4eead380058dd029a3a0c72a1796ebb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/243778
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>

internal/lsp/diagnostics.go

@@ -6,6 +6,7 @@ package lsp
 
 import (
 	"context"
+	"crypto/sha1"
 	"fmt"
 	"strings"
 	"sync"
@@ -19,7 +20,9 @@ import (
 	"golang.org/x/xerrors"
 )
 
-type diagnosticKey struct {
+// idWithAnalysis is used to track if the diagnostics for a given file were
+// computed with analyses.
+type idWithAnalysis struct {
 	id           source.FileIdentity
 	withAnalysis bool
 }
@@ -45,7 +48,7 @@ func (s *Server) diagnoseSnapshot(snapshot source.Snapshot) {
 
 // diagnose is a helper function for running diagnostics with a given context.
 // Do not call it directly.
-func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, alwaysAnalyze bool) (map[diagnosticKey][]*source.Diagnostic, *protocol.ShowMessageParams) {
+func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, alwaysAnalyze bool) (map[idWithAnalysis]map[string]*source.Diagnostic, *protocol.ShowMessageParams) {
 	ctx, done := event.Start(ctx, "lsp:background-worker")
 	defer done()
 
@@ -57,28 +60,32 @@ func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, alwaysA
 	}
 	defer func() { <-s.diagnosticsSema }()
 
-	allReports := make(map[diagnosticKey][]*source.Diagnostic)
 	var reportsMu sync.Mutex
-	var wg sync.WaitGroup
+	reports := map[idWithAnalysis]map[string]*source.Diagnostic{}
 
-	// Diagnose the go.mod file.
-	reports, modErr := mod.Diagnostics(ctx, snapshot)
+	// First, diagnose the go.mod file.
+	modReports, modErr := mod.Diagnostics(ctx, snapshot)
 	if ctx.Err() != nil {
 		return nil, nil
 	}
 	if modErr != nil {
 		event.Error(ctx, "warning: diagnose go.mod", modErr, tag.Directory.Of(snapshot.View().Folder().Filename()))
 	}
-	for id, diags := range reports {
+	for id, diags := range modReports {
 		if id.URI == "" {
 			event.Error(ctx, "missing URI for module diagnostics", fmt.Errorf("empty URI"), tag.Directory.Of(snapshot.View().Folder().Filename()))
 			continue
 		}
-		key := diagnosticKey{
+		key := idWithAnalysis{
 			id:           id,
 			withAnalysis: true, // treat go.mod diagnostics like analyses
 		}
-		allReports[key] = diags
+		if _, ok := reports[key]; !ok {
+			reports[key] = map[string]*source.Diagnostic{}
+		}
+		for _, d := range diags {
+			reports[key][diagnosticKey(d)] = d
+		}
 	}
 
 	// Diagnose all of the packages in the workspace.
@@ -99,23 +106,30 @@ If you believe this is a mistake, please file an issue: https://github.com/golan
 		}
 		return nil, nil
 	}
-	var shows *protocol.ShowMessageParams
+	var (
+		showMsg *protocol.ShowMessageParams
+		wg      sync.WaitGroup
+	)
 	for _, ph := range wsPackages {
 		wg.Add(1)
 		go func(ph source.PackageHandle) {
 			defer wg.Done()
+
 			// Only run analyses for packages with open files.
-			withAnalyses := alwaysAnalyze
+			withAnalysis := alwaysAnalyze
 			for _, pgh := range ph.CompiledGoFiles() {
 				if snapshot.IsOpen(pgh.File().URI()) {
-					withAnalyses = true
+					withAnalysis = true
+					break
 				}
 			}
-			reports, warn, err := source.Diagnostics(ctx, snapshot, ph, withAnalyses)
+
+			pkgReports, warn, err := source.Diagnostics(ctx, snapshot, ph, withAnalysis)
+
 			// Check if might want to warn the user about their build configuration.
 			// Our caller decides whether to send the message.
 			if warn && !snapshot.View().ValidBuildConfiguration() {
-				shows = &protocol.ShowMessageParams{
+				showMsg = &protocol.ShowMessageParams{
 					Type:    protocol.Warning,
 					Message: `You are neither in a module nor in your GOPATH. If you are using modules, please open your editor at the directory containing the go.mod. If you believe this warning is incorrect, please file an issue: https://github.com/golang/go/issues/new.`,
 				}
@@ -124,22 +138,44 @@ If you believe this is a mistake, please file an issue: https://github.com/golan
 				event.Error(ctx, "warning: diagnose package", err, tag.Snapshot.Of(snapshot.ID()), tag.Package.Of(ph.ID()))
 				return
 			}
+
+			// Add all reports to the global map, checking for duplciates.
 			reportsMu.Lock()
-			for id, diags := range reports {
-				key := diagnosticKey{
+			for id, diags := range pkgReports {
+				key := idWithAnalysis{
 					id:           id,
-					withAnalysis: withAnalyses,
+					withAnalysis: withAnalysis,
+				}
+				if _, ok := reports[key]; !ok {
+					reports[key] = map[string]*source.Diagnostic{}
+				}
+				for _, d := range diags {
+					reports[key][diagnosticKey(d)] = d
 				}
-				allReports[key] = append(allReports[key], diags...)
 			}
 			reportsMu.Unlock()
 		}(ph)
 	}
 	wg.Wait()
-	return allReports, shows
+	return reports, showMsg
 }
 
-func (s *Server) publishReports(ctx context.Context, snapshot source.Snapshot, reports map[diagnosticKey][]*source.Diagnostic) {
+// diagnosticKey creates a unique identifier for a given diagnostic, since we
+// cannot use source.Diagnostics as map keys. This is used to de-duplicate
+// diagnostics.
+func diagnosticKey(d *source.Diagnostic) string {
+	var tags, related string
+	for _, t := range d.Tags {
+		tags += fmt.Sprintf("%s", t)
+	}
+	for _, r := range d.Related {
+		related += fmt.Sprintf("%s%s%s", r.URI, r.Message, r.Range)
+	}
+	key := fmt.Sprintf("%s%s%s%s%s%s", d.Message, d.Range, d.Severity, d.Source, tags, related)
+	return fmt.Sprintf("%x", sha1.Sum([]byte(key)))
+}
+
+func (s *Server) publishReports(ctx context.Context, snapshot source.Snapshot, reports map[idWithAnalysis]map[string]*source.Diagnostic) {
 	// Check for context cancellation before publishing diagnostics.
 	if ctx.Err() != nil {
 		return
@@ -148,12 +184,16 @@ func (s *Server) publishReports(ctx context.Context, snapshot source.Snapshot, r
 	s.deliveredMu.Lock()
 	defer s.deliveredMu.Unlock()
 
-	for key, diagnostics := range reports {
+	for key, diagnosticsMap := range reports {
 		// Don't deliver diagnostics if the context has already been canceled.
 		if ctx.Err() != nil {
 			break
 		}
 		// Pre-sort diagnostics to avoid extra work when we compare them.
+		var diagnostics []*source.Diagnostic
+		for _, d := range diagnosticsMap {
+			diagnostics = append(diagnostics, d)
+		}
 		source.SortDiagnostics(diagnostics)
 		toSend := sentDiagnostics{
 			version:      key.id.Version,
@@ -295,8 +335,8 @@ See https://github.com/golang/go/issues/39164 for more detail on this issue.`,
 		if err != nil {
 			return false
 		}
-		s.publishReports(ctx, snapshot, map[diagnosticKey][]*source.Diagnostic{
-			{id: fh.Identity()}: {diag},
+		s.publishReports(ctx, snapshot, map[idWithAnalysis]map[string]*source.Diagnostic{
+			{id: fh.Identity()}: {diagnosticKey(diag): diag},
 		})
 		return true
 	}

internal/lsp/lsp_test.go

@@ -38,7 +38,7 @@ func TestLSP(t *testing.T) {
 type runner struct {
 	server      *Server
 	data        *tests.Data
-	diagnostics map[span.URI][]*source.Diagnostic
+	diagnostics map[span.URI]map[string]*source.Diagnostic
 	ctx         context.Context
 }
 
@@ -118,7 +118,7 @@ func (r *runner) CodeLens(t *testing.T, uri span.URI, want []protocol.CodeLens)
 func (r *runner) Diagnostics(t *testing.T, uri span.URI, want []*source.Diagnostic) {
 	// Get the diagnostics for this view if we have not done it before.
 	if r.diagnostics == nil {
-		r.diagnostics = make(map[span.URI][]*source.Diagnostic)
+		r.diagnostics = make(map[span.URI]map[string]*source.Diagnostic)
 		v := r.server.session.View(r.data.Config.Dir)
 		// Always run diagnostics with analysis.
 		reports, _ := r.server.diagnose(r.ctx, v.Snapshot(), true)
@@ -126,7 +126,10 @@ func (r *runner) Diagnostics(t *testing.T, uri span.URI, want []*source.Diagnost
 			r.diagnostics[key.id.URI] = diags
 		}
 	}
-	got := r.diagnostics[uri]
+	var got []*source.Diagnostic
+	for _, d := range r.diagnostics[uri] {
+		got = append(got, d)
+	}
 	// A special case to test that there are no diagnostics for a file.
 	if len(want) == 1 && want[0].Source == "no_diagnostics" {
 		if len(got) != 0 {
@@ -380,7 +383,7 @@ func (r *runner) SuggestedFix(t *testing.T, spn span.Span, actionKinds []string)
 	}
 	// Get the diagnostics for this view if we have not done it before.
 	if r.diagnostics == nil {
-		r.diagnostics = make(map[span.URI][]*source.Diagnostic)
+		r.diagnostics = make(map[span.URI]map[string]*source.Diagnostic)
 		// Always run diagnostics with analysis.
 		reports, _ := r.server.diagnose(r.ctx, view.Snapshot(), true)
 		for key, diags := range reports {

internal/lsp/regtest/diagnostics_test.go

@@ -258,8 +258,17 @@ func Hello() {
 			env.SaveBuffer("go.mod")
 			env.Await(
 				EmptyDiagnostics("main.go"),
+			)
+			metBy := env.Await(
 				env.DiagnosticAtRegexp("bob/bob.go", "x"),
 			)
+			d, ok := metBy[0].(*protocol.PublishDiagnosticsParams)
+			if !ok {
+				t.Fatalf("unexpected met by result %v (%T)", metBy, metBy)
+			}
+			if len(d.Diagnostics) != 1 {
+				t.Fatalf("expected 1 diagnostic, got %v", len(d.Diagnostics))
+			}
 		})
 	})
 	t.Run("initialized", func(t *testing.T) {