gopls/internal/lsp/cache: cleanups to active packages

This CL makes a number of preparatory cleanups before
I tackle golang/go#59674:

- (*snapshot).dumpWorkspace deleted (obsolete)
- cache.newMemoizedFS deleted (dead code)
- snapshot.workspaceMetadata deleted (use Snapshot.WorkspaceMetadata instead).
- Snapshot.ActiveMetadata renamed WorkspaceMetadata
  and obsolete comment updated
- (*snapshot).isWorkspacePackage deleted (dead code)
  [Interestingly this method didn't show up in the output of Rob's script.]
- Snapshot: begin to group methods in a meaningful way.
- Remove Get prefix from Snapshot.CriticalError
- terminological tweaks.

Updates golang/go#59674

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

Author: adonovan

gopls/internal/lsp/cache/debug.go

@@ -7,7 +7,6 @@ package cache
 import (
 	"fmt"
 	"os"
-	"sort"
 )
 
 // This file contains helpers that can be used to instrument code while
@@ -34,29 +33,3 @@ func assert(cond bool, msg string) {
 		panic(msg)
 	}
 }
-
-// If debugEnabled is true, dumpWorkspace prints a summary of workspace
-// packages to stderr. If debugEnabled is false, it is a no-op.
-//
-// TODO(rfindley): this has served its purpose. Delete.
-func (s *snapshot) dumpWorkspace(context string) {
-	if !debugEnabled {
-		return
-	}
-
-	debugf("workspace (after %s):", context)
-	var ids []PackageID
-	for id := range s.workspacePackages {
-		ids = append(ids, id)
-	}
-
-	sort.Slice(ids, func(i, j int) bool {
-		return ids[i] < ids[j]
-	})
-
-	for _, id := range ids {
-		pkgPath := s.workspacePackages[id]
-		_, ok := s.meta.metadata[id]
-		debugf("  %s:%s (metadata: %t)", id, pkgPath, ok)
-	}
-}

gopls/internal/lsp/cache/fs_memoized.go

@@ -27,10 +27,6 @@ type memoizedFS struct {
 	filesByID map[robustio.FileID][]*DiskFile
 }
 
-func newMemoizedFS() *memoizedFS {
-	return &memoizedFS{filesByID: make(map[robustio.FileID][]*DiskFile)}
-}
-
 // A DiskFile is a file on the filesystem, or a failure to read one.
 // It implements the source.FileHandle interface.
 type DiskFile struct {

gopls/internal/lsp/cache/load.go

@@ -244,7 +244,6 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc
 	s.workspacePackages = workspacePackages
 	s.resetActivePackagesLocked()
 
-	s.dumpWorkspace("load")
 	s.mu.Unlock()
 
 	// Opt: preLoad files in parallel.

gopls/internal/lsp/cache/mod_tidy.go

@@ -61,7 +61,7 @@ func (s *snapshot) ModTidy(ctx context.Context, pm *source.ParsedModule) (*sourc
 			}
 		}
 
-		if criticalErr := s.GetCriticalError(ctx); criticalErr != nil {
+		if criticalErr := s.CriticalError(ctx); criticalErr != nil {
 			return &source.TidiedModule{
 				Diagnostics: criticalErr.Diagnostics,
 			}, nil
@@ -192,10 +192,15 @@ func modTidyDiagnostics(ctx context.Context, snapshot *snapshot, pm *source.Pars
 		missingModuleFixes[req] = srcDiag.SuggestedFixes
 		diagnostics = append(diagnostics, srcDiag)
 	}
+
 	// Add diagnostics for missing modules anywhere they are imported in the
 	// workspace.
+	metas, err := snapshot.WorkspaceMetadata(ctx)
+	if err != nil {
+		return nil, err
+	}
 	// TODO(adonovan): opt: opportunities for parallelism abound.
-	for _, m := range snapshot.workspaceMetadata() {
+	for _, m := range metas {
 		// Read both lists of files of this package.
 		//
 		// Parallelism is not necessary here as the files will have already been

gopls/internal/lsp/cache/snapshot.go

@@ -859,21 +859,15 @@ func (s *snapshot) ReverseDependencies(ctx context.Context, id PackageID, transi
 	return rdeps, nil
 }
 
-func (s *snapshot) workspaceMetadata() (meta []*source.Metadata) {
-	s.mu.Lock()
-	defer s.mu.Unlock()
-
-	for id := range s.workspacePackages {
-		meta = append(meta, s.meta.metadata[id])
-	}
-	return meta
-}
-
 // -- Active package tracking --
 //
-// We say a package is "active" if any of its files are open. After
-// type-checking we keep active packages in memory. The activePackages
-// peristent map does bookkeeping for the set of active packages.
+// We say a package is "active" if any of its files are open.
+// This is an optimization: the "active" concept is an
+// implementation detail of the cache and is not exposed
+// in the source or Snapshot API.
+// After type-checking we keep active packages in memory.
+// The activePackages persistent map does bookkeeping for
+// the set of active packages.
 
 // getActivePackage returns a the memoized active package for id, if it exists.
 // If id is not active or has not yet been type-checked, it returns nil.
@@ -1100,11 +1094,19 @@ func (s *snapshot) knownFilesInDir(ctx context.Context, dir span.URI) []span.URI
 	return files
 }
 
-func (s *snapshot) ActiveMetadata(ctx context.Context) ([]*source.Metadata, error) {
+func (s *snapshot) WorkspaceMetadata(ctx context.Context) ([]*source.Metadata, error) {
 	if err := s.awaitLoaded(ctx); err != nil {
 		return nil, err
 	}
-	return s.workspaceMetadata(), nil
+
+	s.mu.Lock()
+	defer s.mu.Unlock()
+
+	meta := make([]*source.Metadata, 0, len(s.workspacePackages))
+	for id := range s.workspacePackages {
+		meta = append(meta, s.meta.metadata[id])
+	}
+	return meta, nil
 }
 
 // Symbols extracts and returns symbol information for every file contained in
@@ -1257,14 +1259,6 @@ func (s *snapshot) noValidMetadataForURILocked(uri span.URI) bool {
 	return true
 }
 
-func (s *snapshot) isWorkspacePackage(id PackageID) bool {
-	s.mu.Lock()
-	defer s.mu.Unlock()
-
-	_, ok := s.workspacePackages[id]
-	return ok
-}
-
 func (s *snapshot) FindFile(uri span.URI) source.FileHandle {
 	s.view.markKnown(uri)
 
@@ -1383,7 +1377,7 @@ func (s *snapshot) awaitLoaded(ctx context.Context) error {
 	return nil
 }
 
-func (s *snapshot) GetCriticalError(ctx context.Context) *source.CriticalError {
+func (s *snapshot) CriticalError(ctx context.Context) *source.CriticalError {
 	// If we couldn't compute workspace mod files, then the load below is
 	// invalid.
 	//
@@ -1400,7 +1394,7 @@ func (s *snapshot) GetCriticalError(ctx context.Context) *source.CriticalError {
 	// Even if packages didn't fail to load, we still may want to show
 	// additional warnings.
 	if loadErr == nil {
-		active, _ := s.ActiveMetadata(ctx)
+		active, _ := s.WorkspaceMetadata(ctx)
 		if msg := shouldShowAdHocPackagesWarning(s, active); msg != "" {
 			return &source.CriticalError{
 				MainError: errors.New(msg),
@@ -2079,7 +2073,6 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC
 	if workspaceModeChanged {
 		result.workspacePackages = map[PackageID]PackagePath{}
 	}
-	result.dumpWorkspace("clone")
 	return result, release
 }
 

gopls/internal/lsp/command.go

@@ -990,7 +990,7 @@ func collectViewStats(ctx context.Context, view *cache.View) (command.ViewStats,
 	}
 	allPackages := collectPackageStats(allMD)
 
-	wsMD, err := s.ActiveMetadata(ctx)
+	wsMD, err := s.WorkspaceMetadata(ctx)
 	if err != nil {
 		return command.ViewStats{}, err
 	}

gopls/internal/lsp/diagnostics.go

@@ -323,11 +323,11 @@ func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, analyze
 	}
 	store(modVulncheckSource, "diagnosing vulnerabilities", vulnReports, vulnErr, false)
 
-	activeMetas, activeErr := snapshot.ActiveMetadata(ctx)
+	activeMetas, activeErr := snapshot.WorkspaceMetadata(ctx)
 	if s.shouldIgnoreError(ctx, snapshot, activeErr) {
 		return
 	}
-	criticalErr := snapshot.GetCriticalError(ctx)
+	criticalErr := snapshot.CriticalError(ctx)
 	if ctx.Err() != nil { // must check ctx after GetCriticalError
 		return
 	}

gopls/internal/lsp/lsp_test.go

@@ -76,8 +76,8 @@ func testLSP(t *testing.T, datum *tests.Data) {
 	datum.ModfileFlagAvailable = len(snapshot.ModFiles()) > 0 && testenv.Go1Point() >= 14
 	release()
 
-	// Open all files for performance reasons. This is done because gopls only
-	// keeps active packages in memory for open files.
+	// Open all files for performance reasons, because gopls only
+	// keeps active packages (those with open files) in memory.
 	//
 	// In practice clients will only send document-oriented requests for open
 	// files.

gopls/internal/lsp/source/completion/package.go

@@ -203,7 +203,7 @@ func (c *completer) packageNameCompletions(ctx context.Context, fileURI span.URI
 // file. This also includes test packages for these packages (<pkg>_test) and
 // the directory name itself.
 func packageSuggestions(ctx context.Context, snapshot source.Snapshot, fileURI span.URI, prefix string) (packages []candidate, err error) {
-	active, err := snapshot.ActiveMetadata(ctx)
+	active, err := snapshot.WorkspaceMetadata(ctx)
 	if err != nil {
 		return nil, err
 	}

gopls/internal/lsp/source/rename.go

@@ -631,8 +631,9 @@ func renamePackageName(ctx context.Context, s Snapshot, f FileHandle, newName Pa
 	// Update any affected replace directives in go.mod files.
 	// TODO(adonovan): extract into its own function.
 	//
-	// TODO: should this operate on all go.mod files, irrespective of whether they are included in the workspace?
-	// Get all active mod files in the workspace
+	// Get all workspace modules.
+	// TODO(adonovan): should this operate on all go.mod files,
+	// irrespective of whether they are included in the workspace?
 	modFiles := s.ModFiles()
 	for _, m := range modFiles {
 		fh, err := s.ReadFile(ctx, m)

gopls/internal/lsp/source/view.go

@@ -153,25 +153,29 @@ type Snapshot interface {
 	// IsBuiltin reports whether uri is part of the builtin package.
 	IsBuiltin(ctx context.Context, uri span.URI) bool
 
+	// CriticalError returns any critical errors in the workspace.
+	//
+	// A nil result may mean success, or context cancellation.
+	CriticalError(ctx context.Context) *CriticalError
+
+	// Symbols returns all symbols in the snapshot.
+	Symbols(ctx context.Context) (map[span.URI][]Symbol, error)
+
+	// -- package metadata --
+
 	// ReverseDependencies returns a new mapping whose entries are
 	// the ID and Metadata of each package in the workspace that
 	// directly or transitively depend on the package denoted by id,
 	// excluding id itself.
 	ReverseDependencies(ctx context.Context, id PackageID, transitive bool) (map[PackageID]*Metadata, error)
 
-	// ActiveMetadata returns a new, unordered slice containing
-	// metadata for all packages considered 'active' in the workspace.
-	//
-	// In normal memory mode, this is all workspace packages. In degraded memory
-	// mode, this is just the reverse transitive closure of open packages.
-	ActiveMetadata(ctx context.Context) ([]*Metadata, error)
+	// WorkspaceMetadata returns a new, unordered slice containing
+	// metadata for all packages in the workspace.
+	WorkspaceMetadata(ctx context.Context) ([]*Metadata, error)
 
 	// AllMetadata returns a new unordered array of metadata for all packages in the workspace.
 	AllMetadata(ctx context.Context) ([]*Metadata, error)
 
-	// Symbols returns all symbols in the snapshot.
-	Symbols(ctx context.Context) (map[span.URI][]Symbol, error)
-
 	// Metadata returns the metadata for the specified package,
 	// or nil if it was not found.
 	Metadata(id PackageID) *Metadata
@@ -185,6 +189,8 @@ type Snapshot interface {
 	// It returns an error if the context was cancelled.
 	MetadataForFile(ctx context.Context, uri span.URI) ([]*Metadata, error)
 
+	// -- package type-checking --
+
 	// TypeCheck parses and type-checks the specified packages,
 	// and returns them in the same order as the ids.
 	// The resulting packages' types may belong to different importers,
@@ -215,11 +221,6 @@ type Snapshot interface {
 	// If these indexes cannot be loaded from cache, the requested packages may
 	// be type-checked.
 	MethodSets(ctx context.Context, ids ...PackageID) ([]*methodsets.Index, error)
-
-	// GetCriticalError returns any critical errors in the workspace.
-	//
-	// A nil result may mean success, or context cancellation.
-	GetCriticalError(ctx context.Context) *CriticalError
 }
 
 // NarrowestMetadataForFile returns metadata for the narrowest package

gopls/internal/lsp/source/workspace_symbol.go

@@ -467,12 +467,12 @@ func matchFile(store *symbolStore, symbolizer symbolizer, matcher matcherFunc, r
 		// All factors are multiplicative, meaning if more than one applies they are
 		// multiplied together.
 		const (
-			// nonWorkspaceFactor is applied to symbols outside of any active
-			// workspace. Developers are less likely to want to jump to code that they
+			// nonWorkspaceFactor is applied to symbols outside the workspace.
+			// Developers are less likely to want to jump to code that they
 			// are not actively working on.
 			nonWorkspaceFactor = 0.5
-			// nonWorkspaceUnexportedFactor is applied to unexported symbols outside of
-			// any active workspace. Since one wouldn't usually jump to unexported
+			// nonWorkspaceUnexportedFactor is applied to unexported symbols outside
+			// the workspace. Since one wouldn't usually jump to unexported
 			// symbols to understand a package API, they are particularly irrelevant.
 			nonWorkspaceUnexportedFactor = 0.5
 			// every field or method nesting level to access the field decreases