gopls/internal/lsp/cache: simplify DiagnosePackage

Previously Snapshot.DiagnosePackage would run after type checking,
would conditionally invoke the analysis framework just for type
error analyzers, and would then munge those diagnostics together
with those from type checking, eliminate duplicates and migrating
the suggested fixes onto the type-checker errors.

However, all its callers immediately then invoke the analysis framework
for the full suite for analyzers. This change separates the three
steps more clearly:
1) type checking, which produces list/parse/type errors, now
   available at Package.DiagnosticsForFile(URI).
2) analysis using all analyzers, including the type-error analyzers,
   which are no longer special. (Consequently the analyzers'
   findings are reported in more tests.)
3) munging of the results together, in source.CombineDiagnostics.
   The munging algorithm is no longer quadratic in the number of
   combined elements.

This change removes one of the blockers to the new analysis
driver implementation in CL 443099.

The tests were updated to assert that type-error analyzers run
whenever we see type checker diagnostics. This was painful to
do, even by the standards to which we have become accustomed.

Change-Id: I2393ad5bc13587ff4bfed86ae586ce658074a501
Reviewed-on: https://go-review.googlesource.com/c/tools/+/456643
Run-TryBot: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>

gopls/internal/lsp/cache/analysis.go

@@ -16,7 +16,6 @@ import (
 	"golang.org/x/sync/errgroup"
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/gopls/internal/lsp/source"
-	"golang.org/x/tools/gopls/internal/span"
 	"golang.org/x/tools/internal/bug"
 	"golang.org/x/tools/internal/event"
 	"golang.org/x/tools/internal/event/tag"
@@ -465,45 +464,3 @@ func factType(fact analysis.Fact) reflect.Type {
 	}
 	return t
 }
-
-func (s *snapshot) DiagnosePackage(ctx context.Context, id PackageID) (map[span.URI][]*source.Diagnostic, error) {
-	pkgs, err := s.TypeCheck(ctx, source.TypecheckFull, id)
-	if err != nil {
-		return nil, err
-	}
-	pkg := pkgs[0].(*pkg)
-	var errorAnalyzerDiag []*source.Diagnostic
-	if pkg.HasTypeErrors() {
-		// Apply type error analyzers.
-		// They augment type error diagnostics with their own fixes.
-		var analyzers []*source.Analyzer
-		for _, a := range s.View().Options().TypeErrorAnalyzers {
-			analyzers = append(analyzers, a)
-		}
-		var err error
-		errorAnalyzerDiag, err = s.Analyze(ctx, pkg.ID(), analyzers)
-		if err != nil {
-			// Keep going: analysis failures should not block diagnostics.
-			event.Error(ctx, "type error analysis failed", err, tag.Package.Of(string(pkg.ID())))
-		}
-	}
-	diags := map[span.URI][]*source.Diagnostic{}
-	for _, diag := range pkg.diagnostics {
-		for _, eaDiag := range errorAnalyzerDiag {
-			if eaDiag.URI == diag.URI && eaDiag.Range == diag.Range && eaDiag.Message == diag.Message {
-				// Type error analyzers just add fixes and tags. Make a copy,
-				// since we don't own either, and overwrite.
-				// The analyzer itself can't do this merge because
-				// analysis.Diagnostic doesn't have all the fields, and Analyze
-				// can't because it doesn't have the type error, notably its code.
-				clone := *diag
-				clone.SuggestedFixes = eaDiag.SuggestedFixes
-				clone.Tags = eaDiag.Tags
-				clone.Analyzer = eaDiag.Analyzer
-				diag = &clone
-			}
-		}
-		diags[diag.URI] = append(diags[diag.URI], diag)
-	}
-	return diags, nil
-}

gopls/internal/lsp/cache/check.go

@@ -605,6 +605,11 @@ func parseCompiledGoFiles(ctx context.Context, compiledGoFiles []source.FileHand
 
 	// Optionally remove parts that don't affect the exported API.
 	if mode == source.ParseExported {
+		// TODO(adonovan): opt: experiment with pre-parser
+		// trimming, either a scanner-based implementation
+		// such as https://go.dev/play/p/KUrObH1YkX8 (~31%
+		// speedup), or a byte-oriented implementation (2x
+		// speedup).
 		if astFilter != nil {
 			// aggressive pruning based on reachability
 			var files []*ast.File

gopls/internal/lsp/cache/pkg.go

@@ -161,3 +161,13 @@ func (p *pkg) HasListOrParseErrors() bool {
 func (p *pkg) HasTypeErrors() bool {
 	return len(p.typeErrors) != 0
 }
+
+func (p *pkg) DiagnosticsForFile(uri span.URI) []*source.Diagnostic {
+	var res []*source.Diagnostic
+	for _, diag := range p.diagnostics {
+		if diag.URI == uri {
+			res = append(res, diag)
+		}
+	}
+	return res
+}

gopls/internal/lsp/cmd/test/check.go

@@ -15,8 +15,8 @@ import (
 	"golang.org/x/tools/gopls/internal/span"
 )
 
-// Diagnostics runs the gopls command on a single file, parses its
-// diagnostics, and compares against the expectations defined by
+// Diagnostics runs the "gopls check" command on a single file, parses
+// its diagnostics, and compares against the expectations defined by
 // markers in the source file.
 func (r *runner) Diagnostics(t *testing.T, uri span.URI, want []*source.Diagnostic) {
 	out, _ := r.runGoplsCmd(t, "check", uri.Filename())

gopls/internal/lsp/code_action.go

@@ -155,23 +155,19 @@ func (s *Server) codeAction(ctx context.Context, params *protocol.CodeActionPara
 		if ctx.Err() != nil {
 			return nil, ctx.Err()
 		}
-		metas, err := snapshot.MetadataForFile(ctx, fh.URI())
-		if err != nil {
-			return nil, err
-		}
-		if len(metas) == 0 {
-			return nil, fmt.Errorf("no package containing file %q", fh.URI())
-		}
-		id := metas[len(metas)-1].ID // last => widest package
-		pkgDiagnostics, err := snapshot.DiagnosePackage(ctx, id)
+
+		// Type-check the package and also run analysis,
+		// then combine their diagnostics.
+		pkg, err := snapshot.PackageForFile(ctx, fh.URI(), source.TypecheckFull, source.WidestPackage)
 		if err != nil {
 			return nil, err
 		}
-		analysisDiags, err := source.Analyze(ctx, snapshot, id, true)
+		analysisDiags, err := source.Analyze(ctx, snapshot, pkg.ID(), true)
 		if err != nil {
 			return nil, err
 		}
-		fileDiags := append(pkgDiagnostics[uri], analysisDiags[uri]...)
+		var fileDiags []*source.Diagnostic
+		source.CombineDiagnostics(pkg, fh.URI(), analysisDiags, &fileDiags, &fileDiags)
 
 		// Split diagnostics into fixes, which must match incoming diagnostics,
 		// and non-fixes, which must match the requested range. Build actions

gopls/internal/lsp/diagnostics.go

@@ -178,6 +178,9 @@ func (s *Server) diagnoseChangedFiles(ctx context.Context, snapshot source.Snaps
 	ctx, done := event.Start(ctx, "Server.diagnoseChangedFiles", source.SnapshotLabels(snapshot)...)
 	defer done()
 
+	// TODO(adonovan): safety: refactor so that group.Go is called
+	// in a second loop, so that if we should later add an early
+	// return to the first loop, we don't leak goroutines.
 	var group errgroup.Group
 	seen := make(map[*source.Metadata]bool)
 	for _, uri := range uris {
@@ -366,27 +369,36 @@ func (s *Server) diagnosePkg(ctx context.Context, snapshot source.Snapshot, m *s
 		return
 	}
 
-	pkgDiagnostics, err := snapshot.DiagnosePackage(ctx, m.ID)
+	pkgs, err := snapshot.TypeCheck(ctx, source.TypecheckFull, m.ID)
 	if err != nil {
-		event.Error(ctx, "warning: diagnosing package", err, append(source.SnapshotLabels(snapshot), tag.Package.Of(string(m.ID)))...)
+		event.Error(ctx, "warning: typecheck failed", err, append(source.SnapshotLabels(snapshot), tag.Package.Of(string(m.ID)))...)
 		return
 	}
-	for _, uri := range m.CompiledGoFiles {
-		// builtin.go exists only for documentation purposes, and is not valid Go code.
-		// Don't report distracting errors
-		if !snapshot.IsBuiltin(ctx, uri) {
-			s.storeDiagnostics(snapshot, uri, typeCheckSource, pkgDiagnostics[uri], true)
-		}
-	}
+	pkg := pkgs[0]
+
+	// Get diagnostics from analysis framework.
+	// This includes type-error analyzers, which suggest fixes to compiler errors.
+	var analysisDiags map[span.URI][]*source.Diagnostic
 	if includeAnalysis {
-		reports, err := source.Analyze(ctx, snapshot, m.ID, false)
+		diags, err := source.Analyze(ctx, snapshot, m.ID, false)
 		if err != nil {
 			event.Error(ctx, "warning: analyzing package", err, append(source.SnapshotLabels(snapshot), tag.Package.Of(string(m.ID)))...)
 			return
 		}
-		for _, uri := range m.CompiledGoFiles {
-			s.storeDiagnostics(snapshot, uri, analysisSource, reports[uri], true)
+		analysisDiags = diags
+	}
+
+	// For each file, update the server's diagnostics state.
+	for _, cgf := range pkg.CompiledGoFiles() {
+		// builtin.go exists only for documentation purposes and
+		// is not valid Go code. Don't report distracting errors.
+		if snapshot.IsBuiltin(ctx, cgf.URI) {
+			continue
 		}
+		var tdiags, adiags []*source.Diagnostic
+		source.CombineDiagnostics(pkg, cgf.URI, analysisDiags, &tdiags, &adiags)
+		s.storeDiagnostics(snapshot, cgf.URI, typeCheckSource, tdiags, true)
+		s.storeDiagnostics(snapshot, cgf.URI, analysisSource, adiags, true)
 	}
 
 	// If gc optimization details are requested, add them to the

gopls/internal/lsp/mod/diagnostics.go

@@ -11,11 +11,9 @@ import (
 	"fmt"
 	"sort"
 	"strings"
-	"sync"
 
 	"golang.org/x/mod/modfile"
 	"golang.org/x/mod/semver"
-	"golang.org/x/sync/errgroup"
 	"golang.org/x/tools/gopls/internal/govulncheck"
 	"golang.org/x/tools/gopls/internal/lsp/command"
 	"golang.org/x/tools/gopls/internal/lsp/protocol"
@@ -89,35 +87,28 @@ func ModDiagnostics(ctx context.Context, snapshot source.Snapshot, fh source.Fil
 	}
 
 	// Packages in the workspace can contribute diagnostics to go.mod files.
-	// TODO(rfindley): Try to avoid calling DiagnosePackage on all packages in the workspace here,
+	// TODO(rfindley): Try to avoid type checking all packages in the workspace here,
 	// for every go.mod file. If gc_details is enabled, it looks like this could lead to extra
 	// go command invocations (as gc details is not memoized).
 	active, err := snapshot.ActiveMetadata(ctx)
 	if err != nil && !source.IsNonFatalGoModError(err) {
 		event.Error(ctx, fmt.Sprintf("workspace packages: diagnosing %s", pm.URI), err)
 	}
 	if err == nil {
-		// Diagnose packages in parallel.
-		// (It's possible this is the first operation after the initial
-		// metadata load to demand type-checking of all the active packages.)
-		var group errgroup.Group
-		var mu sync.Mutex
-		for _, m := range active {
-			m := m
-			group.Go(func() error {
-				pkgDiagnostics, err := snapshot.DiagnosePackage(ctx, m.ID)
-				if err != nil {
-					return err
-				}
-				mu.Lock()
-				diagnostics = append(diagnostics, pkgDiagnostics[fh.URI()]...)
-				mu.Unlock()
-				return nil
-			})
-		}
-		if err := group.Wait(); err != nil {
+		// Type-check packages in parallel and gather list/parse/type errors.
+		// (This may be the first operation after the initial metadata load
+		// to demand type-checking of all active packages.)
+		ids := make([]source.PackageID, len(active))
+		for i, meta := range active {
+			ids[i] = meta.ID
+		}
+		pkgs, err := snapshot.TypeCheck(ctx, source.TypecheckFull, ids...)
+		if err != nil {
 			return nil, err
 		}
+		for _, pkg := range pkgs {
+			diagnostics = append(diagnostics, pkg.DiagnosticsForFile(fh.URI())...)
+		}
 	}
 
 	tidied, err := snapshot.ModTidy(ctx, pm)

gopls/internal/lsp/source/diagnostics.go

@@ -6,7 +6,6 @@ package source
 
 import (
 	"context"
-	"fmt"
 
 	"golang.org/x/tools/gopls/internal/lsp/protocol"
 	"golang.org/x/tools/gopls/internal/span"
@@ -37,6 +36,7 @@ func Analyze(ctx context.Context, snapshot Snapshot, pkgid PackageID, includeCon
 	categories := []map[string]*Analyzer{
 		options.DefaultAnalyzers,
 		options.StaticcheckAnalyzers,
+		options.TypeErrorAnalyzers,
 	}
 	if includeConvenience { // e.g. for codeAction
 		categories = append(categories, options.ConvenienceAnalyzers) // e.g. fillstruct
@@ -55,14 +55,15 @@ func Analyze(ctx context.Context, snapshot Snapshot, pkgid PackageID, includeCon
 	}
 
 	// Report diagnostics and errors from root analyzers.
-	reports := map[span.URI][]*Diagnostic{}
+	reports := make(map[span.URI][]*Diagnostic)
 	for _, diag := range analysisDiagnostics {
 		reports[diag.URI] = append(reports[diag.URI], diag)
 	}
 	return reports, nil
 }
 
-// FileDiagnostics reports diagnostics in the specified file.
+// FileDiagnostics reports diagnostics in the specified file,
+// as used by the "gopls check" command.
 //
 // TODO(adonovan): factor in common with (*Server).codeAction, which
 // executes { PackageForFile; DiagnosePackage; Analyze } too?
@@ -76,22 +77,66 @@ func FileDiagnostics(ctx context.Context, snapshot Snapshot, uri span.URI) (Vers
 	if err != nil {
 		return VersionedFileIdentity{}, nil, err
 	}
-	metas, err := snapshot.MetadataForFile(ctx, uri)
+	pkg, err := snapshot.PackageForFile(ctx, uri, TypecheckFull, NarrowestPackage)
 	if err != nil {
 		return VersionedFileIdentity{}, nil, err
 	}
-	if len(metas) == 0 {
-		return VersionedFileIdentity{}, nil, fmt.Errorf("no package containing file %q", uri)
-	}
-	id := metas[0].ID // 0 => narrowest package
-	diagnostics, err := snapshot.DiagnosePackage(ctx, id)
+	adiags, err := Analyze(ctx, snapshot, pkg.ID(), false)
 	if err != nil {
 		return VersionedFileIdentity{}, nil, err
 	}
-	analysisDiags, err := Analyze(ctx, snapshot, id, false)
-	if err != nil {
-		return VersionedFileIdentity{}, nil, err
-	}
-	fileDiags := append(diagnostics[fh.URI()], analysisDiags[fh.URI()]...)
+	var fileDiags []*Diagnostic // combine load/parse/type + analysis diagnostics
+	CombineDiagnostics(pkg, fh.URI(), adiags, &fileDiags, &fileDiags)
 	return fh.VersionedFileIdentity(), fileDiags, nil
 }
+
+// CombineDiagnostics combines and filters list/parse/type diagnostics
+// from pkg.DiagnosticsForFile(uri) with analysisDiagnostics[uri], and
+// appends the two lists to *outT and *outA, respectively.
+//
+// Type-error analyzers produce diagnostics that are redundant
+// with type checker diagnostics, but more detailed (e.g. fixes).
+// Rather than report two diagnostics for the same problem,
+// we combine them by augmenting the type-checker diagnostic
+// and discarding the analyzer diagnostic.
+//
+// If an analysis diagnostic has the same range and message as
+// a list/parse/type diagnostic, the suggested fix information
+// (et al) of the latter is merged into a copy of the former.
+// This handles the case where a type-error analyzer suggests
+// a fix to a type error, and avoids duplication.
+//
+// The use of out-slices, though irregular, allows the caller to
+// easily choose whether to keep the results separate or combined.
+//
+// The arguments are not modified.
+func CombineDiagnostics(pkg Package, uri span.URI, analysisDiagnostics map[span.URI][]*Diagnostic, outT, outA *[]*Diagnostic) {
+
+	// Build index of (list+parse+)type errors.
+	type key struct {
+		Range   protocol.Range
+		message string
+	}
+	index := make(map[key]int) // maps (Range,Message) to index in tdiags slice
+	tdiags := pkg.DiagnosticsForFile(uri)
+	for i, diag := range tdiags {
+		index[key{diag.Range, diag.Message}] = i
+	}
+
+	// Filter out analysis diagnostics that match type errors,
+	// retaining their suggested fix (etc) fields.
+	for _, diag := range analysisDiagnostics[uri] {
+		if i, ok := index[key{diag.Range, diag.Message}]; ok {
+			copy := *tdiags[i]
+			copy.SuggestedFixes = diag.SuggestedFixes
+			copy.Tags = diag.Tags
+			copy.Analyzer = diag.Analyzer
+			tdiags[i] = &copy
+			continue
+		}
+
+		*outA = append(*outA, diag)
+	}
+
+	*outT = append(*outT, tdiags...)
+}