gopls/internal/lsp/cache: UX improvements for analysis

When analysis takes a long time, it can be a poor experience for our
users. Address this in a number of ways:

- Limit parallelism of analysis, so that it doesn't consume all
  available memory.
- Add progress reporting if analysis takes longer than 5s, for better
  visibility and to allow cancellation. In case this is annoying to
  users, add a setting to disable this feature.

For golang/go#61178

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

Author: findleyr

gopls/doc/settings.md

@@ -344,6 +344,20 @@ This option must be set to a valid duration string, for example `"250ms"`.
 
 Default: `"1s"`.
 
+##### **analysisProgressReporting** *bool*
+
+analysisProgressReporting controls whether gopls sends progress
+notifications when construction of its index of analysis facts is taking a
+long time. Cancelling these notifications will cancel the indexing task,
+though it will restart after the next change in the workspace.
+
+When a package is opened for the first time and heavyweight analyses such as
+staticcheck are enabled, it can take a while to construct the index of
+analysis facts for all its dependencies. The index is cached in the
+filesystem, so subsequent analysis should be faster.
+
+Default: `true`.
+
 #### Documentation
 
 ##### **hoverKind** *enum*

gopls/internal/lsp/cache/analysis.go

@@ -20,6 +20,7 @@ import (
 	"log"
 	urlpkg "net/url"
 	"reflect"
+	"runtime"
 	"runtime/debug"
 	"sort"
 	"strings"
@@ -32,6 +33,7 @@ import (
 	"golang.org/x/tools/gopls/internal/bug"
 	"golang.org/x/tools/gopls/internal/lsp/filecache"
 	"golang.org/x/tools/gopls/internal/lsp/frob"
+	"golang.org/x/tools/gopls/internal/lsp/progress"
 	"golang.org/x/tools/gopls/internal/lsp/protocol"
 	"golang.org/x/tools/gopls/internal/lsp/source"
 	"golang.org/x/tools/internal/event"
@@ -154,14 +156,23 @@ import (
 //        View() *View // for Options
 //   - share cache.{goVersionRx,parseGoImpl}
 
+// AnalysisProgressTitle is the title of the progress report for ongoing
+// analysis. It is sought by regression tests for the progress reporting
+// feature.
+const AnalysisProgressTitle = "Analyzing Dependencies"
+
 // Analyze applies a set of analyzers to the package denoted by id,
 // and returns their diagnostics for that package.
 //
 // The analyzers list must be duplicate free; order does not matter.
 //
+// Notifications of progress may be sent to the optional reporter.
+//
 // Precondition: all analyzers within the process have distinct names.
 // (The names are relied on by the serialization logic.)
-func (snapshot *snapshot) Analyze(ctx context.Context, pkgs map[PackageID]unit, analyzers []*source.Analyzer) ([]*source.Diagnostic, error) {
+func (snapshot *snapshot) Analyze(ctx context.Context, pkgs map[PackageID]unit, analyzers []*source.Analyzer, reporter *progress.Tracker) ([]*source.Diagnostic, error) {
+	start := time.Now() // for progress reporting
+
 	var tagStr string // sorted comma-separated list of PackageIDs
 	{
 		// TODO(adonovan): replace with a generic map[S]any -> string
@@ -296,21 +307,84 @@ func (snapshot *snapshot) Analyze(ctx context.Context, pkgs map[PackageID]unit,
 
 	// Now that we have read all files,
 	// we no longer need the snapshot.
+	// (but options are needed for progress reporting)
+	options := snapshot.view.Options()
 	snapshot = nil
 
+	// Progress reporting. If supported, gopls reports progress on analysis
+	// passes that are taking a long time.
+	maybeReport := func(completed int64) {}
+
+	// Enable progress reporting if enabled by the user
+	// and we have a capable reporter.
+	if reporter != nil && reporter.SupportsWorkDoneProgress() && options.AnalysisProgressReporting {
+		var reportAfter = options.ReportAnalysisProgressAfter // tests may set this to 0
+		const reportEvery = 1 * time.Second
+
+		ctx, cancel := context.WithCancel(ctx)
+		defer cancel()
+
+		var (
+			reportMu   sync.Mutex
+			lastReport time.Time
+			wd         *progress.WorkDone
+		)
+		defer func() {
+			reportMu.Lock()
+			defer reportMu.Unlock()
+
+			if wd != nil {
+				wd.End(ctx, "Done.") // ensure that the progress report exits
+			}
+		}()
+		maybeReport = func(completed int64) {
+			now := time.Now()
+			if now.Sub(start) < reportAfter {
+				return
+			}
+
+			reportMu.Lock()
+			defer reportMu.Unlock()
+
+			if wd == nil {
+				wd = reporter.Start(ctx, AnalysisProgressTitle, "", nil, cancel)
+			}
+
+			if now.Sub(lastReport) > reportEvery {
+				lastReport = now
+				// Trailing space is intentional: some LSP clients strip newlines.
+				msg := fmt.Sprintf(`Constructing index of analysis facts... (%d/%d packages). 
+(Set "analysisProgressReporting" to false to disable notifications.)`,
+					completed, len(nodes))
+				pct := 100 * float64(completed) / float64(len(nodes))
+				wd.Report(ctx, msg, pct)
+			}
+		}
+	}
+
 	// Execute phase: run leaves first, adding
 	// new nodes to the queue as they become leaves.
 	var g errgroup.Group
-	// Avoid g.SetLimit here: it makes g.Go stop accepting work,
-	// which prevents workers from enqeuing, and thus finishing,
-	// and thus allowing the group to make progress: deadlock.
+
+	// Analysis is CPU-bound.
+	//
+	// Note: avoid g.SetLimit here: it makes g.Go stop accepting work, which
+	// prevents workers from enqeuing, and thus finishing, and thus allowing the
+	// group to make progress: deadlock.
+	limiter := make(chan unit, runtime.GOMAXPROCS(0))
+	var completed int64
+
 	var enqueue func(*analysisNode)
 	enqueue = func(an *analysisNode) {
 		g.Go(func() error {
+			limiter <- unit{}
+			defer func() { <-limiter }()
+
 			summary, err := an.runCached(ctx)
 			if err != nil {
 				return err // cancelled, or failed to produce a package
 			}
+			maybeReport(atomic.AddInt64(&completed, 1))
 			an.summary = summary
 
 			// Notify each waiting predecessor,

gopls/internal/lsp/code_action.go

@@ -203,7 +203,7 @@ func (s *Server) codeAction(ctx context.Context, params *protocol.CodeActionPara
 		if err != nil {
 			return nil, err
 		}
-		analysisDiags, err := source.Analyze(ctx, snapshot, map[source.PackageID]unit{pkg.Metadata().ID: {}}, true)
+		analysisDiags, err := source.Analyze(ctx, snapshot, map[source.PackageID]unit{pkg.Metadata().ID: {}}, true, s.progress)
 		if err != nil {
 			return nil, err
 		}

gopls/internal/lsp/diagnostics.go

@@ -448,7 +448,7 @@ func (s *Server) diagnosePkgs(ctx context.Context, snapshot source.Snapshot, toD
 	wg.Add(1)
 	go func() {
 		defer wg.Done()
-		diags, err := source.Analyze(ctx, snapshot, toAnalyze, false)
+		diags, err := source.Analyze(ctx, snapshot, toAnalyze, false, s.progress)
 		if err != nil {
 			var tagStr string // sorted comma-separated list of package IDs
 			{

gopls/internal/lsp/progress/progress.go

@@ -33,10 +33,22 @@ func NewTracker(client protocol.Client) *Tracker {
 	}
 }
 
+// SetSupportsWorkDoneProgress sets whether the client supports work done
+// progress reporting. It must be set before using the tracker.
+//
+// TODO(rfindley): fix this broken initialization pattern.
+// Also: do we actually need the fall-back progress behavior using ShowMessage?
+// Surely ShowMessage notifications are too noisy to be worthwhile.
 func (tracker *Tracker) SetSupportsWorkDoneProgress(b bool) {
 	tracker.supportsWorkDoneProgress = b
 }
 
+// SupportsWorkDoneProgress reports whether the tracker supports work done
+// progress reporting.
+func (tracker *Tracker) SupportsWorkDoneProgress() bool {
+	return tracker.supportsWorkDoneProgress
+}
+
 // Start notifies the client of work being done on the server. It uses either
 // ShowMessage RPCs or $/progress messages, depending on the capabilities of
 // the client.  The returned WorkDone handle may be used to report incremental

gopls/internal/lsp/regtest/expectation.go

@@ -105,6 +105,26 @@ func describeExpectations(expectations ...Expectation) string {
 	return strings.Join(descriptions, "\n")
 }
 
+// Not inverts the sense of an expectation: a met expectation is unmet, and an
+// unmet expectation is met.
+func Not(e Expectation) Expectation {
+	check := func(s State) Verdict {
+		switch v := e.Check(s); v {
+		case Met:
+			return Unmet
+		case Unmet, Unmeetable:
+			return Met
+		default:
+			panic(fmt.Sprintf("unexpected verdict %v", v))
+		}
+	}
+	description := describeExpectations(e)
+	return Expectation{
+		Check:       check,
+		Description: fmt.Sprintf("not: %s", description),
+	}
+}
+
 // AnyOf returns an expectation that is satisfied when any of the given
 // expectations is met.
 func AnyOf(anyOf ...Expectation) Expectation {

gopls/internal/lsp/source/api_json.go

@@ -9,6 +9,7 @@ import (
 	"encoding/json"
 
 	"golang.org/x/tools/gopls/internal/bug"
+	"golang.org/x/tools/gopls/internal/lsp/progress"
 	"golang.org/x/tools/gopls/internal/lsp/protocol"
 	"golang.org/x/tools/gopls/internal/span"
 )
@@ -21,7 +22,10 @@ type SuggestedFix struct {
 }
 
 // Analyze reports go/analysis-framework diagnostics in the specified package.
-func Analyze(ctx context.Context, snapshot Snapshot, pkgIDs map[PackageID]unit, includeConvenience bool) (map[span.URI][]*Diagnostic, error) {
+//
+// 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 Snapshot, pkgIDs map[PackageID]unit, includeConvenience bool, tracker *progress.Tracker) (map[span.URI][]*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 {
@@ -45,7 +49,7 @@ func Analyze(ctx context.Context, snapshot Snapshot, pkgIDs map[PackageID]unit,
 		}
 	}
 
-	analysisDiagnostics, err := snapshot.Analyze(ctx, pkgIDs, analyzers)
+	analysisDiagnostics, err := snapshot.Analyze(ctx, pkgIDs, analyzers, tracker)
 	if err != nil {
 		return nil, err
 	}
@@ -81,7 +85,7 @@ func FileDiagnostics(ctx context.Context, snapshot Snapshot, uri span.URI) (File
 	if err != nil {
 		return nil, nil, err
 	}
-	adiags, err := Analyze(ctx, snapshot, map[PackageID]unit{pkg.Metadata().ID: {}}, false)
+	adiags, err := Analyze(ctx, snapshot, map[PackageID]unit{pkg.Metadata().ID: {}}, false, nil /* progress tracker */)
 	if err != nil {
 		return nil, nil, err
 	}

gopls/internal/lsp/source/diagnostics.go

@@ -124,14 +124,15 @@ func DefaultOptions() *Options {
 				},
 				UIOptions: UIOptions{
 					DiagnosticOptions: DiagnosticOptions{
-						DiagnosticsDelay: 1 * time.Second,
 						Annotations: map[Annotation]bool{
 							Bounds: true,
 							Escape: true,
 							Inline: true,
 							Nil:    true,
 						},
-						Vulncheck: ModeVulncheckOff,
+						Vulncheck:                 ModeVulncheckOff,
+						DiagnosticsDelay:          1 * time.Second,
+						AnalysisProgressReporting: true,
 					},
 					InlayHintOptions: InlayHintOptions{},
 					DocumentationOptions: DocumentationOptions{
@@ -162,14 +163,15 @@ func DefaultOptions() *Options {
 				},
 			},
 			InternalOptions: InternalOptions{
-				LiteralCompletions:      true,
-				TempModfile:             true,
-				CompleteUnimported:      true,
-				CompletionDocumentation: true,
-				DeepCompletion:          true,
-				ChattyDiagnostics:       true,
-				NewDiff:                 "both",
-				SubdirWatchPatterns:     SubdirWatchPatternsAuto,
+				LiteralCompletions:          true,
+				TempModfile:                 true,
+				CompleteUnimported:          true,
+				CompletionDocumentation:     true,
+				DeepCompletion:              true,
+				ChattyDiagnostics:           true,
+				NewDiff:                     "both",
+				SubdirWatchPatterns:         SubdirWatchPatternsAuto,
+				ReportAnalysisProgressAfter: 5 * time.Second,
 			},
 			Hooks: Hooks{
 				// TODO(adonovan): switch to new diff.Strings implementation.
@@ -428,6 +430,17 @@ type DiagnosticOptions struct {
 	//
 	// This option must be set to a valid duration string, for example `"250ms"`.
 	DiagnosticsDelay time.Duration `status:"advanced"`
+
+	// AnalysisProgressReporting controls whether gopls sends progress
+	// notifications when construction of its index of analysis facts is taking a
+	// long time. Cancelling these notifications will cancel the indexing task,
+	// though it will restart after the next change in the workspace.
+	//
+	// When a package is opened for the first time and heavyweight analyses such as
+	// staticcheck are enabled, it can take a while to construct the index of
+	// analysis facts for all its dependencies. The index is cached in the
+	// filesystem, so subsequent analysis should be faster.
+	AnalysisProgressReporting bool
 }
 
 type InlayHintOptions struct {
@@ -540,7 +553,7 @@ type Hooks struct {
 // by the user.
 //
 // TODO(rfindley): even though these settings are not intended for
-// modification, we should surface them in our documentation.
+// modification, some of them should be surfaced in our documentation.
 type InternalOptions struct {
 	// LiteralCompletions controls whether literal candidates such as
 	// "&someStruct{}" are offered. Tests disable this flag to simplify
@@ -630,6 +643,12 @@ type InternalOptions struct {
 	// example, if like VS Code it drops file notifications), please file an
 	// issue.
 	SubdirWatchPatterns SubdirWatchPatterns
+
+	// ReportAnalysisProgressAfter sets the duration for gopls to wait before starting
+	// progress reporting for ongoing go/analysis passes.
+	//
+	// It is intended to be used for testing only.
+	ReportAnalysisProgressAfter time.Duration
 }
 
 type SubdirWatchPatterns string
@@ -1171,6 +1190,9 @@ func (o *Options) set(name string, value interface{}, seen map[string]struct{})
 	case "diagnosticsDelay":
 		result.setDuration(&o.DiagnosticsDelay)
 
+	case "analysisProgressReporting":
+		result.setBool(&o.AnalysisProgressReporting)
+
 	case "experimentalWatchedFileDelay":
 		result.deprecated("")
 
@@ -1208,6 +1230,9 @@ func (o *Options) set(name string, value interface{}, seen map[string]struct{})
 			o.SubdirWatchPatterns = SubdirWatchPatterns(s)
 		}
 
+	case "reportAnalysisProgressAfter":
+		result.setDuration(&o.ReportAnalysisProgressAfter)
+
 	// Replaced settings.
 	case "experimentalDisabledAnalyses":
 		result.deprecated("analyses")

gopls/internal/lsp/source/options.go

@@ -23,6 +23,7 @@ import (
 	"golang.org/x/tools/go/packages"
 	"golang.org/x/tools/go/types/objectpath"
 	"golang.org/x/tools/gopls/internal/govulncheck"
+	"golang.org/x/tools/gopls/internal/lsp/progress"
 	"golang.org/x/tools/gopls/internal/lsp/protocol"
 	"golang.org/x/tools/gopls/internal/lsp/safetoken"
 	"golang.org/x/tools/gopls/internal/lsp/source/methodsets"
@@ -93,7 +94,10 @@ type Snapshot interface {
 	ParseGo(ctx context.Context, fh FileHandle, mode parser.Mode) (*ParsedGoFile, error)
 
 	// Analyze runs the specified analyzers on the given packages at this snapshot.
-	Analyze(ctx context.Context, pkgIDs map[PackageID]unit, analyzers []*Analyzer) ([]*Diagnostic, error)
+	//
+	// If the provided tracker is non-nil, it may be used to report progress of
+	// the analysis pass.
+	Analyze(ctx context.Context, pkgIDs map[PackageID]unit, analyzers []*Analyzer, tracker *progress.Tracker) ([]*Diagnostic, error)
 
 	// RunGoCommandPiped runs the given `go` command, writing its output
 	// to stdout and stderr. Verb, Args, and WorkingDir must be specified.