gopls/internal/server: analyze widest packages for open files

This change causes server.diagnose to choose a covering set
of packages for the open files preferring the widest package
for a given package path. This has two benefits: correctness,
for analyzers (such as unusedparams) that make incorrect
deductions when additional files may be added to the package;
and efficiency, as it requires fewer packages in general.

Change-Id: I21576c6ce4c136d6c72ccce76971a782abd4df53
Reviewed-on: https://go-review.googlesource.com/c/tools/+/556816
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Robert Findley <[email protected]>

Author: adonovan

gopls/internal/analysis/unusedparams/unusedparams.go

@@ -52,6 +52,15 @@ func run(pass *analysis.Pass) (any, error) {
 	//    use(func() { ... })				address-taken
 	//
 
+	// Note: this algorithm relies on the assumption that the
+	// analyzer is called only for the "widest" package for a
+	// given file: that is, p_test in preference to p, if both
+	// exist. Analyzing only package p may produce diagnostics
+	// that would be falsified based on declarations in p_test.go
+	// files. The gopls analysis driver does this, but most
+	// drivers to not, so running this command in, say,
+	// unitchecker or multichecker may produce incorrect results.
+
 	// Gather global information:
 	// - uses of functions not in call position
 	// - unexported interface methods

gopls/internal/lsp/cache/analysis.go

@@ -172,7 +172,7 @@ const AnalysisProgressTitle = "Analyzing Dependencies"
 // The analyzers list must be duplicate free; order does not matter.
 //
 // Notifications of progress may be sent to the optional reporter.
-func (s *Snapshot) Analyze(ctx context.Context, pkgs map[PackageID]unit, analyzers []*settings.Analyzer, reporter *progress.Tracker) ([]*Diagnostic, error) {
+func (s *Snapshot) Analyze(ctx context.Context, pkgs map[PackageID]*metadata.Package, analyzers []*settings.Analyzer, reporter *progress.Tracker) ([]*Diagnostic, error) {
 	start := time.Now() // for progress reporting
 
 	var tagStr string // sorted comma-separated list of PackageIDs

gopls/internal/lsp/source/diagnostics.go

@@ -8,6 +8,7 @@ import (
 	"context"
 
 	"golang.org/x/tools/gopls/internal/lsp/cache"
+	"golang.org/x/tools/gopls/internal/lsp/cache/metadata"
 	"golang.org/x/tools/gopls/internal/lsp/progress"
 	"golang.org/x/tools/gopls/internal/lsp/protocol"
 	"golang.org/x/tools/gopls/internal/settings"
@@ -18,7 +19,7 @@ import (
 //
 // If the provided tracker is non-nil, it may be used to provide notifications
 // of the ongoing analysis pass.
-func Analyze(ctx context.Context, snapshot *cache.Snapshot, pkgIDs map[PackageID]unit, tracker *progress.Tracker) (map[protocol.DocumentURI][]*cache.Diagnostic, error) {
+func Analyze(ctx context.Context, snapshot *cache.Snapshot, pkgIDs map[PackageID]*metadata.Package, tracker *progress.Tracker) (map[protocol.DocumentURI][]*cache.Diagnostic, error) {
 	// Exit early if the context has been canceled. This also protects us
 	// from a race on Options, see golang/go#36699.
 	if ctx.Err() != nil {

gopls/internal/server/diagnostics.go

@@ -415,9 +415,30 @@ func (s *server) diagnose(ctx context.Context, snapshot *cache.Snapshot) (diagMa
 	}()
 
 	// Run type checking and go/analysis diagnosis of packages in parallel.
+	//
+	// For analysis, we use the *widest* package for each open file,
+	// for two reasons:
+	//
+	// - Correctness: some analyzers (e.g. unusedparam) depend
+	//   on it. If applied to a non-test package for which a
+	//   corresponding test package exists, they make assumptions
+	//   that are falsified in the test package, for example that
+	//   all references to unexported symbols are visible to the
+	//   analysis.
+	//
+	// - Efficiency: it may yield a smaller covering set of
+	//   PackageIDs for a given set of files. For example, {x.go,
+	//   x_test.go} is covered by the single package x_test using
+	//   "widest". (Using "narrowest", it would be covered only by
+	//   the pair of packages {x, x_test}, Originally we used all
+	//   covering packages, so {x.go} alone would be analyzed
+	//   twice.)
 	var (
 		toDiagnose = make(map[metadata.PackageID]*metadata.Package)
-		toAnalyze  = make(map[metadata.PackageID]unit)
+		toAnalyze  = make(map[metadata.PackageID]*metadata.Package)
+
+		// secondary index, used to eliminate narrower packages.
+		toAnalyzeWidest = make(map[source.PackagePath]*metadata.Package)
 	)
 	for _, mp := range workspacePkgs {
 		var hasNonIgnored, hasOpenFile bool
@@ -432,7 +453,16 @@ func (s *server) diagnose(ctx context.Context, snapshot *cache.Snapshot) (diagMa
 		if hasNonIgnored {
 			toDiagnose[mp.ID] = mp
 			if hasOpenFile {
-				toAnalyze[mp.ID] = unit{}
+				if prev, ok := toAnalyzeWidest[mp.PkgPath]; ok {
+					if len(prev.CompiledGoFiles) >= len(mp.CompiledGoFiles) {
+						// Previous entry is not narrower; keep it.
+						continue
+					}
+					// Evict previous (narrower) entry.
+					delete(toAnalyze, prev.ID)
+				}
+				toAnalyze[mp.ID] = mp
+				toAnalyzeWidest[mp.PkgPath] = mp
 			}
 		}
 	}