gopls/internal/lsp: fix initial diagnostics pass

CL 477979 changed the package diagnostics pass to record all diagnostics
resulting from a call to PackageDiagnostics, rather than just those for
CompiledGoFiles. However, as a result we stopped storing empty
diagnostics sets for files without any package diagnostics. This caused
our early diagnostics pass for changed files not to clear diagnostics,
and so diagnostics were not cleared until after the diagnosticsDelay
expired (at which point the set of reports for a given snapshot is
assumed to be complete, and so no report is equivalent to an empty
report).

Fix this by explicitly storing empty diagnostics for all package files.
Also fix a related bug, where the published hash was not being updated
for reports that were skipped.

Unfortunately, this fix is very hard to test with the current setup,
because the client has no insight into whether diagnostics are published
in the initial pass or second pass. I added a much needed TODO to
simplify the entire pass, a goal of which should be to improve
testability.

Also optimize diagnostic hashing for the empty set, and remove some
unhelpful Trace level logging.

Fixes golang/go#59587

Change-Id: I1cf4c61cadad7933f534199b1ee06ea30a046b1d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/484195
Reviewed-by: Alan Donovan <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Run-TryBot: Robert Findley <[email protected]>
gopls-CI: kokoro <[email protected]>

Author: findleyr

gopls/internal/lsp/cache/analysis.go

@@ -262,8 +262,10 @@ type analyzeSummary struct {
 // TODO(adonovan): generalize and move to a library when we can use generics.
 type actionsMap map[string]*actionSummary
 
-var _ gob.GobEncoder = (actionsMap)(nil)
-var _ gob.GobDecoder = (*actionsMap)(nil)
+var (
+	_ gob.GobEncoder = (actionsMap)(nil)
+	_ gob.GobDecoder = (*actionsMap)(nil)
+)
 
 type actionsMapEntry struct {
 	K string
@@ -443,10 +445,8 @@ func analyzeImpl(ctx context.Context, snapshot *snapshot, analyzers []*analysis.
 	if data, err := filecache.Get(cacheKind, key); err == nil {
 		// cache hit
 		mustDecode(data, &summary)
-
 	} else if err != filecache.ErrNotFound {
 		return nil, bug.Errorf("internal error reading shared cache: %v", err)
-
 	} else {
 		// Cache miss: do the work.
 		var err error
@@ -566,7 +566,6 @@ func analysisCacheKey(analyzers []*analysis.Analyzer, m *source.Metadata, compil
 // actuallyAnalyze implements the cache-miss case.
 // This function does not access the snapshot.
 func actuallyAnalyze(ctx context.Context, analyzers []*analysis.Analyzer, m *source.Metadata, vdeps map[PackageID]*analyzeSummary, compiledGoFiles []source.FileHandle) (*analyzeSummary, error) {
-
 	// Create a local FileSet for processing this package only.
 	fset := token.NewFileSet()
 
@@ -952,7 +951,7 @@ func (act *action) exec() (interface{}, *actionSummary, error) {
 	}
 
 	// Gather analysis Result values from horizontal dependencies.
-	var inputs = make(map[*analysis.Analyzer]interface{})
+	inputs := make(map[*analysis.Analyzer]interface{})
 	for _, dep := range act.hdeps {
 		inputs[dep.a] = dep.result
 	}

gopls/internal/lsp/diagnostics.go

@@ -15,7 +15,6 @@ import (
 	"sync"
 	"time"
 
-	"golang.org/x/tools/gopls/internal/lsp/debug/log"
 	"golang.org/x/tools/gopls/internal/lsp/mod"
 	"golang.org/x/tools/gopls/internal/lsp/protocol"
 	"golang.org/x/tools/gopls/internal/lsp/source"
@@ -28,6 +27,10 @@ import (
 	"golang.org/x/tools/internal/xcontext"
 )
 
+// TODO(rfindley): simplify this very complicated logic for publishing
+// diagnostics. While doing so, ensure that we can test subtle logic such as
+// for multi-pass diagnostics.
+
 // diagnosticSource differentiates different sources of diagnostics.
 //
 // Diagnostics from the same source overwrite each other, whereas diagnostics
@@ -85,7 +88,7 @@ type fileReports struct {
 	mustPublish bool
 
 	// The last stored diagnostics for each diagnostic source.
-	reports map[diagnosticSource]diagnosticReport
+	reports map[diagnosticSource]*diagnosticReport
 }
 
 func (d diagnosticSource) String() string {
@@ -114,17 +117,35 @@ func (d diagnosticSource) String() string {
 }
 
 // hashDiagnostics computes a hash to identify diags.
+//
+// hashDiagnostics mutates its argument (via sorting).
 func hashDiagnostics(diags ...*source.Diagnostic) string {
+	if len(diags) == 0 {
+		return emptyDiagnosticsHash
+	}
+	return computeDiagnosticHash(diags...)
+}
+
+// opt: pre-computed hash for empty diagnostics
+var emptyDiagnosticsHash = computeDiagnosticHash()
+
+// computeDiagnosticHash should only be called from hashDiagnostics.
+//
+// TODO(rfindley): this should use source.Hash.
+func computeDiagnosticHash(diags ...*source.Diagnostic) string {
 	source.SortDiagnostics(diags)
 	h := sha256.New()
 	for _, d := range diags {
 		for _, t := range d.Tags {
-			fmt.Fprintf(h, "%s", t)
+			fmt.Fprintf(h, "tag: %s\n", t)
 		}
 		for _, r := range d.Related {
-			fmt.Fprintf(h, "%s%s%s", r.Location.URI.SpanURI(), r.Message, r.Location.Range)
+			fmt.Fprintf(h, "related: %s %s %s\n", r.Location.URI.SpanURI(), r.Message, r.Location.Range)
 		}
-		fmt.Fprintf(h, "%s%s%s%s", d.Message, d.Range, d.Severity, d.Source)
+		fmt.Fprintf(h, "message: %s\n", d.Message)
+		fmt.Fprintf(h, "range: %s\n", d.Range)
+		fmt.Fprintf(h, "severity: %s\n", d.Severity)
+		fmt.Fprintf(h, "source: %s\n", d.Source)
 	}
 	return fmt.Sprintf("%x", h.Sum(nil))
 }
@@ -171,6 +192,9 @@ func (s *Server) diagnoseSnapshot(snapshot source.Snapshot, changedURIs []span.U
 		//
 		// TODO(rfindley): it would be cleaner to simply put the diagnostic
 		// debouncer on the view, and remove the "key" argument to debouncing.
+		//
+		// TODO(rfindley): debounce should accept a context, so that we don't hold onto
+		// the snapshot when the BackgroundContext is cancelled.
 		if ok := <-s.diagDebouncer.debounce(snapshot.View().Name(), snapshot.SequenceID(), time.After(delay)); ok {
 			s.diagnose(ctx, snapshot, analyzeOpenPackages)
 			s.publishDiagnostics(ctx, true, snapshot)
@@ -280,31 +304,27 @@ func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, analyze
 	// Diagnose go.work file.
 	workReports, workErr := work.Diagnostics(ctx, snapshot)
 	if ctx.Err() != nil {
-		log.Trace.Log(ctx, "diagnose cancelled")
 		return
 	}
 	store(workSource, "diagnosing go.work file", workReports, workErr, true)
 
 	// Diagnose go.mod file.
 	modReports, modErr := mod.Diagnostics(ctx, snapshot)
 	if ctx.Err() != nil {
-		log.Trace.Log(ctx, "diagnose cancelled")
 		return
 	}
 	store(modParseSource, "diagnosing go.mod file", modReports, modErr, true)
 
 	// Diagnose go.mod upgrades.
 	upgradeReports, upgradeErr := mod.UpgradeDiagnostics(ctx, snapshot)
 	if ctx.Err() != nil {
-		log.Trace.Log(ctx, "diagnose cancelled")
 		return
 	}
 	store(modCheckUpgradesSource, "diagnosing go.mod upgrades", upgradeReports, upgradeErr, true)
 
 	// Diagnose vulnerabilities.
 	vulnReports, vulnErr := mod.VulnerabilityDiagnostics(ctx, snapshot)
 	if ctx.Err() != nil {
-		log.Trace.Log(ctx, "diagnose cancelled")
 		return
 	}
 	store(modVulncheckSource, "diagnosing vulnerabilities", vulnReports, vulnErr, false)
@@ -479,8 +499,26 @@ func (s *Server) diagnosePkgs(ctx context.Context, snapshot source.Snapshot, toD
 		s.storeDiagnostics(snapshot, uri, analysisSource, adiags2, true)
 	}
 
+	// golang/go#59587: guarantee that we store type-checking diagnostics for every compiled
+	// package file.
+	//
+	// Without explicitly storing empty diagnostics, the eager diagnostics
+	// publication for changed files will not publish anything for files with
+	// empty diagnostics.
+	storedPkgDiags := make(map[span.URI]bool)
+	for _, m := range toDiagnose {
+		for _, uri := range m.CompiledGoFiles {
+			s.storeDiagnostics(snapshot, uri, typeCheckSource, pkgDiags[uri], true)
+			storedPkgDiags[uri] = true
+		}
+	}
 	// Store the package diagnostics.
 	for uri, diags := range pkgDiags {
+		if storedPkgDiags[uri] {
+			continue
+		}
+		// builtin.go exists only for documentation purposes, and is not valid Go code.
+		// Don't report distracting errors
 		if snapshot.IsBuiltin(ctx, uri) {
 			bug.Reportf("type checking reported diagnostics for the builtin file: %v", diags)
 			continue
@@ -549,7 +587,7 @@ func (s *Server) mustPublishDiagnostics(uri span.URI) {
 	if s.diagnostics[uri] == nil {
 		s.diagnostics[uri] = &fileReports{
 			publishedHash: hashDiagnostics(), // Hash for 0 diagnostics.
-			reports:       map[diagnosticSource]diagnosticReport{},
+			reports:       map[diagnosticSource]*diagnosticReport{},
 		}
 	}
 	s.diagnostics[uri].mustPublish = true
@@ -558,6 +596,8 @@ func (s *Server) mustPublishDiagnostics(uri span.URI) {
 // storeDiagnostics stores results from a single diagnostic source. If merge is
 // true, it merges results into any existing results for this snapshot.
 //
+// Mutates (sorts) diags.
+//
 // TODO(hyangah): investigate whether we can unconditionally overwrite previous report.diags
 // with the new diags and eliminate the need for the `merge` flag.
 func (s *Server) storeDiagnostics(snapshot source.Snapshot, uri span.URI, dsource diagnosticSource, diags []*source.Diagnostic, merge bool) {
@@ -573,10 +613,14 @@ func (s *Server) storeDiagnostics(snapshot source.Snapshot, uri span.URI, dsourc
 	if s.diagnostics[uri] == nil {
 		s.diagnostics[uri] = &fileReports{
 			publishedHash: hashDiagnostics(), // Hash for 0 diagnostics.
-			reports:       map[diagnosticSource]diagnosticReport{},
+			reports:       map[diagnosticSource]*diagnosticReport{},
 		}
 	}
 	report := s.diagnostics[uri].reports[dsource]
+	if report == nil {
+		report = new(diagnosticReport)
+		s.diagnostics[uri].reports[dsource] = report
+	}
 	// Don't set obsolete diagnostics.
 	if report.snapshotID > snapshot.GlobalID() {
 		return
@@ -588,7 +632,6 @@ func (s *Server) storeDiagnostics(snapshot source.Snapshot, uri span.URI, dsourc
 	for _, d := range diags {
 		report.diags[hashDiagnostics(d)] = d
 	}
-	s.diagnostics[uri].reports[dsource] = report
 }
 
 // clearDiagnosticSource clears all diagnostics for a given source type. It is
@@ -735,6 +778,7 @@ func (s *Server) publishDiagnostics(ctx context.Context, final bool, snapshot so
 				diags = append(diags, d)
 				reportDiags = append(reportDiags, d)
 			}
+
 			hash := hashDiagnostics(reportDiags...)
 			if hash != report.publishedHash {
 				anyReportsChanged = true
@@ -748,7 +792,6 @@ func (s *Server) publishDiagnostics(ctx context.Context, final bool, snapshot so
 			continue
 		}
 
-		source.SortDiagnostics(diags)
 		hash := hashDiagnostics(diags...)
 		if hash == r.publishedHash && !r.mustPublish {
 			// Update snapshotID to be the latest snapshot for which this diagnostic
@@ -768,15 +811,22 @@ func (s *Server) publishDiagnostics(ctx context.Context, final bool, snapshot so
 			r.publishedHash = hash
 			r.mustPublish = false // diagnostics have been successfully published
 			r.publishedSnapshotID = snapshot.GlobalID()
-			for dsource, hash := range reportHashes {
-				report := r.reports[dsource]
-				report.publishedHash = hash
-				r.reports[dsource] = report
+			// When we publish diagnostics for a file, we must update the
+			// publishedHash for every report, not just the reports that were
+			// published. Eliding a report is equivalent to publishing empty
+			// diagnostics.
+			for dsource, report := range r.reports {
+				if hash, ok := reportHashes[dsource]; ok {
+					report.publishedHash = hash
+				} else {
+					// The report was not (yet) stored for this snapshot. Record that we
+					// published no diagnostics from this source.
+					report.publishedHash = hashDiagnostics()
+				}
 			}
 		} else {
 			if ctx.Err() != nil {
 				// Publish may have failed due to a cancelled context.
-				log.Trace.Log(ctx, "publish cancelled")
 				return
 			}
 			event.Error(ctx, "publishReports: failed to deliver diagnostic", err, tag.URI.Of(uri))
@@ -852,7 +902,7 @@ func (s *Server) Diagnostics() map[string][]string {
 	return ans
 }
 
-func auxStr(v *source.Diagnostic, d diagnosticReport, typ diagnosticSource) string {
+func auxStr(v *source.Diagnostic, d *diagnosticReport, typ diagnosticSource) string {
 	// Tags? RelatedInformation?
 	msg := fmt.Sprintf("(%s)%q(source:%q,code:%q,severity:%s,snapshot:%d,type:%s)",
 		v.Range, v.Message, v.Source, v.Code, v.Severity, d.snapshotID, typ)