From 80cb79702c9e104dd1d60dd6f521936504a2a7d2 Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Tue, 21 Jul 2020 02:05:22 -0400 Subject: [PATCH] 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 TryBot-Result: Gobot Gobot Reviewed-by: Heschi Kreinick --- internal/lsp/diagnostics.go | 86 +++++++++++++++++------- internal/lsp/lsp_test.go | 11 +-- internal/lsp/regtest/diagnostics_test.go | 9 +++ 3 files changed, 79 insertions(+), 27 deletions(-) diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index fa62daeb31..9a92ee9675 100644 --- a/internal/lsp/diagnostics.go +++ b/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 } diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go index 6e38f4362f..1525f7ac5e 100644 --- a/internal/lsp/lsp_test.go +++ b/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 { diff --git a/internal/lsp/regtest/diagnostics_test.go b/internal/lsp/regtest/diagnostics_test.go index 8d61357c02..0398fa7f36 100644 --- a/internal/lsp/regtest/diagnostics_test.go +++ b/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) {