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 <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
diff --git a/gopls/internal/lsp/cache/debug.go b/gopls/internal/lsp/cache/debug.go
index 56072f9..0045617 100644
--- a/gopls/internal/lsp/cache/debug.go
+++ b/gopls/internal/lsp/cache/debug.go
@@ -7,7 +7,6 @@
import (
"fmt"
"os"
- "sort"
)
// This file contains helpers that can be used to instrument code while
@@ -34,29 +33,3 @@
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)
- }
-}
diff --git a/gopls/internal/lsp/cache/fs_memoized.go b/gopls/internal/lsp/cache/fs_memoized.go
index 33d4a7b..37f59e4 100644
--- a/gopls/internal/lsp/cache/fs_memoized.go
+++ b/gopls/internal/lsp/cache/fs_memoized.go
@@ -27,10 +27,6 @@
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 {
diff --git a/gopls/internal/lsp/cache/load.go b/gopls/internal/lsp/cache/load.go
index e7ce265..d24c792 100644
--- a/gopls/internal/lsp/cache/load.go
+++ b/gopls/internal/lsp/cache/load.go
@@ -244,7 +244,6 @@
s.workspacePackages = workspacePackages
s.resetActivePackagesLocked()
- s.dumpWorkspace("load")
s.mu.Unlock()
// Opt: preLoad files in parallel.
diff --git a/gopls/internal/lsp/cache/mod_tidy.go b/gopls/internal/lsp/cache/mod_tidy.go
index 0ba1188..b5e2dea 100644
--- a/gopls/internal/lsp/cache/mod_tidy.go
+++ b/gopls/internal/lsp/cache/mod_tidy.go
@@ -61,7 +61,7 @@
}
}
- 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 @@
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
diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go
index 294995a..6557a0c 100644
--- a/gopls/internal/lsp/cache/snapshot.go
+++ b/gopls/internal/lsp/cache/snapshot.go
@@ -859,21 +859,15 @@
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 @@
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 @@
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 @@
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 @@
// 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 @@
if workspaceModeChanged {
result.workspacePackages = map[PackageID]PackagePath{}
}
- result.dumpWorkspace("clone")
return result, release
}
diff --git a/gopls/internal/lsp/command.go b/gopls/internal/lsp/command.go
index bb43d06..6fa8312 100644
--- a/gopls/internal/lsp/command.go
+++ b/gopls/internal/lsp/command.go
@@ -990,7 +990,7 @@
}
allPackages := collectPackageStats(allMD)
- wsMD, err := s.ActiveMetadata(ctx)
+ wsMD, err := s.WorkspaceMetadata(ctx)
if err != nil {
return command.ViewStats{}, err
}
diff --git a/gopls/internal/lsp/diagnostics.go b/gopls/internal/lsp/diagnostics.go
index db8256e..4c595a9 100644
--- a/gopls/internal/lsp/diagnostics.go
+++ b/gopls/internal/lsp/diagnostics.go
@@ -323,11 +323,11 @@
}
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
}
diff --git a/gopls/internal/lsp/lsp_test.go b/gopls/internal/lsp/lsp_test.go
index 9699c04..9dec7d3 100644
--- a/gopls/internal/lsp/lsp_test.go
+++ b/gopls/internal/lsp/lsp_test.go
@@ -76,8 +76,8 @@
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.
diff --git a/gopls/internal/lsp/source/completion/package.go b/gopls/internal/lsp/source/completion/package.go
index f8a7b0a..00bf051 100644
--- a/gopls/internal/lsp/source/completion/package.go
+++ b/gopls/internal/lsp/source/completion/package.go
@@ -203,7 +203,7 @@
// 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
}
diff --git a/gopls/internal/lsp/source/rename.go b/gopls/internal/lsp/source/rename.go
index 1b3440b..34a8278 100644
--- a/gopls/internal/lsp/source/rename.go
+++ b/gopls/internal/lsp/source/rename.go
@@ -631,8 +631,9 @@
// 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)
diff --git a/gopls/internal/lsp/source/view.go b/gopls/internal/lsp/source/view.go
index 42d397b..9fe6599 100644
--- a/gopls/internal/lsp/source/view.go
+++ b/gopls/internal/lsp/source/view.go
@@ -153,25 +153,29 @@
// 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 @@
// 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 @@
// 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
diff --git a/gopls/internal/lsp/source/workspace_symbol.go b/gopls/internal/lsp/source/workspace_symbol.go
index 6ad1bf6..a0ffe3f 100644
--- a/gopls/internal/lsp/source/workspace_symbol.go
+++ b/gopls/internal/lsp/source/workspace_symbol.go
@@ -467,12 +467,12 @@
// 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
