gopls/internal/lsp: clarify control around diagnostics

This CL includes some clarifications while trying to
understand the performance of the initial workspace load
and analysis. No significant behavior changes.

Server.diagnose:
- Factor the four copies of the logic for dealing
  with diagnostics and errors.
- Make the ActivePackages blocking step explicit.
  Previously mod.Diagnostics would do this implicitly,
  making it look more expensive than it is.
Server.addFolders:
- eliminate TODO. The logic is not in fact fishy.
- use informative names and comments for WaitGroups.
- use a channel in place of a non-counting WaitGroup.

Also, give pkg a String method.

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

gopls/internal/lsp/cache/pkg.go

@@ -36,6 +36,8 @@ type pkg struct {
 	analyses memoize.Store // maps analyzer.Name to Promise[actionResult]
 }
 
+func (p *pkg) String() string { return p.ID() }
+
 // A loadScope defines a package loading scope for use with go/packages.
 type loadScope interface {
 	aScope()

gopls/internal/lsp/diagnostics.go

@@ -217,6 +217,10 @@ func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, forceAn
 	defer done()
 
 	// Wait for a free diagnostics slot.
+	// TODO(adonovan): opt: shouldn't it be the analysis implementation's
+	// job to de-dup and limit resource consumption? In any case this
+	// this function spends most its time waiting for awaitLoaded, at
+	// least initially.
 	select {
 	case <-ctx.Done():
 		return
@@ -226,73 +230,62 @@ func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, forceAn
 		<-s.diagnosticsSema
 	}()
 
-	// First, diagnose the go.mod file.
-	modReports, modErr := mod.Diagnostics(ctx, snapshot)
-	if ctx.Err() != nil {
-		log.Trace.Log(ctx, "diagnose cancelled")
-		return
-	}
-	if modErr != nil {
-		event.Error(ctx, "warning: diagnose go.mod", modErr, tag.Directory.Of(snapshot.View().Folder().Filename()), tag.Snapshot.Of(snapshot.ID()))
-	}
-	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
+	// common code for dispatching diagnostics
+	store := func(dsource diagnosticSource, operation string, diagsByFileID map[source.VersionedFileIdentity][]*source.Diagnostic, err error) {
+		if err != nil {
+			event.Error(ctx, "warning: while "+operation, err,
+				tag.Directory.Of(snapshot.View().Folder().Filename()),
+				tag.Snapshot.Of(snapshot.ID()))
 		}
-		s.storeDiagnostics(snapshot, id.URI, modSource, diags)
-	}
-	upgradeModReports, upgradeErr := mod.UpgradeDiagnostics(ctx, snapshot)
-	if ctx.Err() != nil {
-		log.Trace.Log(ctx, "diagnose cancelled")
-		return
-	}
-	if upgradeErr != nil {
-		event.Error(ctx, "warning: diagnose go.mod upgrades", upgradeErr, tag.Directory.Of(snapshot.View().Folder().Filename()), tag.Snapshot.Of(snapshot.ID()))
-	}
-	for id, diags := range upgradeModReports {
-		if id.URI == "" {
-			event.Error(ctx, "missing URI for module diagnostics", fmt.Errorf("empty URI"), tag.Directory.Of(snapshot.View().Folder().Filename()))
-			continue
+		for id, diags := range diagsByFileID {
+			if id.URI == "" {
+				event.Error(ctx, "missing URI while "+operation, fmt.Errorf("empty URI"), tag.Directory.Of(snapshot.View().Folder().Filename()))
+				continue
+			}
+			s.storeDiagnostics(snapshot, id.URI, dsource, diags)
 		}
-		s.storeDiagnostics(snapshot, id.URI, modCheckUpgradesSource, diags)
-	}
-	vulnerabilityReports, vulnErr := mod.VulnerabilityDiagnostics(ctx, snapshot)
-	if ctx.Err() != nil {
-		log.Trace.Log(ctx, "diagnose cancelled")
-		return
-	}
-	if vulnErr != nil {
-		event.Error(ctx, "warning: checking vulnerabilities", vulnErr, tag.Directory.Of(snapshot.View().Folder().Filename()), tag.Snapshot.Of(snapshot.ID()))
-	}
-	for id, diags := range vulnerabilityReports {
-		if id.URI == "" {
-			event.Error(ctx, "missing URI for module diagnostics", fmt.Errorf("empty URI"), tag.Directory.Of(snapshot.View().Folder().Filename()))
-			continue
-		}
-		s.storeDiagnostics(snapshot, id.URI, modVulncheckSource, diags)
 	}
 
-	// Diagnose the go.work file, if it exists.
+	// 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)
+
+	// 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)
+
+	// Diagnose go.work file.
 	workReports, workErr := work.Diagnostics(ctx, snapshot)
 	if ctx.Err() != nil {
 		log.Trace.Log(ctx, "diagnose cancelled")
 		return
 	}
-	if workErr != nil {
-		event.Error(ctx, "warning: diagnose go.work", workErr, tag.Directory.Of(snapshot.View().Folder().Filename()), tag.Snapshot.Of(snapshot.ID()))
-	}
-	for id, diags := range workReports {
-		if id.URI == "" {
-			event.Error(ctx, "missing URI for work file diagnostics", fmt.Errorf("empty URI"), tag.Directory.Of(snapshot.View().Folder().Filename()))
-			continue
-		}
-		s.storeDiagnostics(snapshot, id.URI, workSource, diags)
-	}
+	store(workSource, "diagnosing go.work file", workReports, workErr)
 
-	// Diagnose all of the packages in the workspace.
-	wsPkgs, err := snapshot.ActivePackages(ctx)
-	if s.shouldIgnoreError(ctx, snapshot, err) {
+	// All subsequent steps depend on the completion of
+	// type-checking of the all active packages in the workspace.
+	// This step may take many seconds initially.
+	// (mod.Diagnostics would implicitly wait for this too,
+	// but the control is clearer if it is explicit here.)
+	activePkgs, activeErr := snapshot.ActivePackages(ctx)
+
+	// Diagnose go.mod file.
+	modReports, modErr := mod.Diagnostics(ctx, snapshot)
+	if ctx.Err() != nil {
+		log.Trace.Log(ctx, "diagnose cancelled")
+		return
+	}
+	store(modSource, "diagnosing go.mod file", modReports, modErr)
+
+	if s.shouldIgnoreError(ctx, snapshot, activeErr) {
 		return
 	}
 	criticalErr := snapshot.GetCriticalError(ctx)
@@ -303,7 +296,7 @@ func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, forceAn
 	// error progress reports will be closed.
 	s.showCriticalErrorStatus(ctx, snapshot, criticalErr)
 
-	// There may be .tmpl files.
+	// Diagnose template (.tmpl) files.
 	for _, f := range snapshot.Templates() {
 		diags := template.Diagnose(f)
 		s.storeDiagnostics(snapshot, f.URI(), typeCheckSource, diags)
@@ -311,29 +304,31 @@ func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, forceAn
 
 	// If there are no workspace packages, there is nothing to diagnose and
 	// there are no orphaned files.
-	if len(wsPkgs) == 0 {
+	if len(activePkgs) == 0 {
 		return
 	}
 
+	// Run go/analysis diagnosis of packages in parallel.
+	// TODO(adonovan): opt: it may be more efficient to
+	// have diagnosePkg take a set of packages.
 	var (
 		wg   sync.WaitGroup
 		seen = map[span.URI]struct{}{}
 	)
-	for _, pkg := range wsPkgs {
-		wg.Add(1)
-
+	for _, pkg := range activePkgs {
 		for _, pgf := range pkg.CompiledGoFiles() {
 			seen[pgf.URI] = struct{}{}
 		}
 
+		wg.Add(1)
 		go func(pkg source.Package) {
 			defer wg.Done()
-
 			s.diagnosePkg(ctx, snapshot, pkg, forceAnalysis)
 		}(pkg)
 	}
 	wg.Wait()
 
+	// Orphaned files.
 	// Confirm that every opened file belongs to a package (if any exist in
 	// the workspace). Otherwise, add a diagnostic to the file.
 	for _, o := range s.session.Overlays() {

gopls/internal/lsp/general.go

@@ -308,18 +308,18 @@ func (s *Server) addFolders(ctx context.Context, folders []protocol.WorkspaceFol
 	originalViews := len(s.session.Views())
 	viewErrors := make(map[span.URI]error)
 
-	var wg sync.WaitGroup
+	var ndiagnose sync.WaitGroup // number of unfinished diagnose calls
 	if s.session.Options().VerboseWorkDoneProgress {
 		work := s.progress.Start(ctx, DiagnosticWorkTitle(FromInitialWorkspaceLoad), "Calculating diagnostics for initial workspace load...", nil, nil)
 		defer func() {
 			go func() {
-				wg.Wait()
+				ndiagnose.Wait()
 				work.End(ctx, "Done.")
 			}()
 		}()
 	}
 	// Only one view gets to have a workspace.
-	var allFoldersWg sync.WaitGroup
+	var nsnapshots sync.WaitGroup // number of unfinished snapshot initializations
 	for _, folder := range folders {
 		uri := span.URIFromURI(folder.URI)
 		// Ignore non-file URIs.
@@ -338,41 +338,40 @@ func (s *Server) addFolders(ctx context.Context, folders []protocol.WorkspaceFol
 		}
 		// Inv: release() must be called once.
 
-		var swg sync.WaitGroup
-		swg.Add(1)
-		allFoldersWg.Add(1)
-		// TODO(adonovan): this looks fishy. Is AwaitInitialized
-		// supposed to be called once per folder?
-		go func() {
-			defer swg.Done()
-			defer allFoldersWg.Done()
-			snapshot.AwaitInitialized(ctx)
-			work.End(ctx, "Finished loading packages.")
-		}()
-
 		// Print each view's environment.
-		buf := &bytes.Buffer{}
-		if err := snapshot.WriteEnv(ctx, buf); err != nil {
+		var buf bytes.Buffer
+		if err := snapshot.WriteEnv(ctx, &buf); err != nil {
 			viewErrors[uri] = err
 			release()
 			continue
 		}
 		event.Log(ctx, buf.String())
 
-		// Diagnose the newly created view.
-		wg.Add(1)
+		// Initialize snapshot asynchronously.
+		initialized := make(chan struct{})
+		nsnapshots.Add(1)
+		go func() {
+			snapshot.AwaitInitialized(ctx)
+			work.End(ctx, "Finished loading packages.")
+			nsnapshots.Done()
+			close(initialized) // signal
+		}()
+
+		// Diagnose the newly created view asynchronously.
+		ndiagnose.Add(1)
 		go func() {
 			s.diagnoseDetached(snapshot)
-			swg.Wait()
+			<-initialized
 			release()
-			wg.Done()
+			ndiagnose.Done()
 		}()
 	}
 
+	// Wait for snapshots to be initialized so that all files are known.
+	// (We don't need to wait for diagnosis to finish.)
+	nsnapshots.Wait()
+
 	// Register for file watching notifications, if they are supported.
-	// Wait for all snapshots to be initialized first, since all files might
-	// not yet be known to the snapshots.
-	allFoldersWg.Wait()
 	if err := s.updateWatchedDirectories(ctx); err != nil {
 		event.Error(ctx, "failed to register for file watching notifications", err)
 	}

gopls/internal/lsp/mod/diagnostics.go

@@ -25,6 +25,8 @@ import (
 )
 
 // Diagnostics returns diagnostics for the modules in the workspace.
+//
+// It waits for completion of type-checking of all active packages.
 func Diagnostics(ctx context.Context, snapshot source.Snapshot) (map[source.VersionedFileIdentity][]*source.Diagnostic, error) {
 	ctx, done := event.Start(ctx, "mod.Diagnostics", tag.Snapshot.Of(snapshot.ID()))
 	defer done()
@@ -73,8 +75,9 @@ func collectDiagnostics(ctx context.Context, snapshot source.Snapshot, diagFn fu
 	return reports, nil
 }
 
-// ModDiagnostics returns diagnostics from diagnosing the packages in the workspace and
-// from tidying the go.mod file.
+// ModDiagnostics waits for completion of type-checking of all active
+// packages, then returns diagnostics from diagnosing the packages in
+// the workspace and from tidying the go.mod file.
 func ModDiagnostics(ctx context.Context, snapshot source.Snapshot, fh source.FileHandle) (diagnostics []*source.Diagnostic, err error) {
 	pm, err := snapshot.ParseMod(ctx, fh)
 	if err != nil {