internal/lsp/cache: remove support for invalid metadata
Remove support for keeping track of invalid metadata, as discussed in
golang/go#55333.
Fixes golang/go#55333
Change-Id: I7727f43e2cffef610096d20af4898f326c5a8450
Reviewed-on: https://go-review.googlesource.com/c/tools/+/447741
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
diff --git a/gopls/internal/lsp/cache/check.go b/gopls/internal/lsp/cache/check.go
index 25da922..1f3622a 100644
--- a/gopls/internal/lsp/cache/check.go
+++ b/gopls/internal/lsp/cache/check.go
@@ -46,7 +46,7 @@
promise *memoize.Promise // [typeCheckResult]
// m is the metadata associated with the package.
- m *KnownMetadata
+ m *Metadata
// key is the hashed key for the package.
//
@@ -105,12 +105,8 @@
// Don't use invalid metadata for dependencies if the top-level
// metadata is valid. We only load top-level packages, so if the
// top-level is valid, all of its dependencies should be as well.
- if err != nil || m.Valid && !depHandle.m.Valid {
- if err != nil {
- event.Error(ctx, fmt.Sprintf("%s: no dep handle for %s", id, depID), err, source.SnapshotLabels(s)...)
- } else {
- event.Log(ctx, fmt.Sprintf("%s: invalid dep handle for %s", id, depID), source.SnapshotLabels(s)...)
- }
+ if err != nil {
+ event.Error(ctx, fmt.Sprintf("%s: no dep handle for %s", id, depID), err, source.SnapshotLabels(s)...)
// This check ensures we break out of the slow
// buildPackageHandle recursion quickly when
@@ -136,7 +132,7 @@
// syntax trees are available from (*pkg).File(URI).
// TODO(adonovan): consider parsing them on demand?
// The need should be rare.
- goFiles, compiledGoFiles, err := readGoFiles(ctx, s, m.Metadata)
+ goFiles, compiledGoFiles, err := readGoFiles(ctx, s, m)
if err != nil {
return nil, err
}
@@ -146,7 +142,7 @@
experimentalKey := s.View().Options().ExperimentalPackageCacheKey
phKey := computePackageKey(m.ID, compiledGoFiles, m, depKey, mode, experimentalKey)
promise, release := s.store.Promise(phKey, func(ctx context.Context, arg interface{}) interface{} {
- pkg, err := typeCheckImpl(ctx, arg.(*snapshot), goFiles, compiledGoFiles, m.Metadata, mode, deps)
+ pkg, err := typeCheckImpl(ctx, arg.(*snapshot), goFiles, compiledGoFiles, m, mode, deps)
return typeCheckResult{pkg, err}
})
@@ -166,7 +162,7 @@
// packageHandle to the handles for parsing its files and the
// handles for type-checking its immediate deps, at which
// point there will be no need to even access s.meta.)
- if s.meta.metadata[ph.m.ID].Metadata != ph.m.Metadata {
+ if s.meta.metadata[ph.m.ID] != ph.m {
return nil, fmt.Errorf("stale metadata for %s", ph.m.ID)
}
@@ -174,7 +170,7 @@
if prev, ok := s.packages.Get(packageKey); ok {
prevPH := prev.(*packageHandle)
release()
- if prevPH.m.Metadata != ph.m.Metadata {
+ if prevPH.m != ph.m {
return nil, bug.Errorf("existing package handle does not match for %s", ph.m.ID)
}
return prevPH, nil
@@ -225,7 +221,7 @@
// computePackageKey returns a key representing the act of type checking
// a package named id containing the specified files, metadata, and
// combined dependency hash.
-func computePackageKey(id PackageID, files []source.FileHandle, m *KnownMetadata, depsKey source.Hash, mode source.ParseMode, experimentalKey bool) packageHandleKey {
+func computePackageKey(id PackageID, files []source.FileHandle, m *Metadata, depsKey source.Hash, mode source.ParseMode, experimentalKey bool) packageHandleKey {
// TODO(adonovan): opt: no need to materalize the bytes; hash them directly.
// Also, use field separators to avoid spurious collisions.
b := bytes.NewBuffer(nil)
diff --git a/gopls/internal/lsp/cache/debug.go b/gopls/internal/lsp/cache/debug.go
index d665b01..fd82aff 100644
--- a/gopls/internal/lsp/cache/debug.go
+++ b/gopls/internal/lsp/cache/debug.go
@@ -47,7 +47,7 @@
for _, id := range ids {
pkgPath := s.workspacePackages[id]
- m, ok := s.meta.metadata[id]
- debugf(" %s:%s (metadata: %t; valid: %t)", id, pkgPath, ok, m.Valid)
+ _, ok := s.meta.metadata[id]
+ debugf(" %s:%s (metadata: %t)", id, pkgPath, ok)
}
}
diff --git a/gopls/internal/lsp/cache/graph.go b/gopls/internal/lsp/cache/graph.go
index 83336a2..be5a061 100644
--- a/gopls/internal/lsp/cache/graph.go
+++ b/gopls/internal/lsp/cache/graph.go
@@ -15,7 +15,7 @@
// graph of Go packages, as obtained from go/packages.
type metadataGraph struct {
// metadata maps package IDs to their associated metadata.
- metadata map[PackageID]*KnownMetadata
+ metadata map[PackageID]*Metadata
// importedBy maps package IDs to the list of packages that import them.
importedBy map[PackageID][]PackageID
@@ -27,12 +27,12 @@
// Clone creates a new metadataGraph, applying the given updates to the
// receiver.
-func (g *metadataGraph) Clone(updates map[PackageID]*KnownMetadata) *metadataGraph {
+func (g *metadataGraph) Clone(updates map[PackageID]*Metadata) *metadataGraph {
if len(updates) == 0 {
// Optimization: since the graph is immutable, we can return the receiver.
return g
}
- result := &metadataGraph{metadata: make(map[PackageID]*KnownMetadata, len(g.metadata))}
+ result := &metadataGraph{metadata: make(map[PackageID]*Metadata, len(g.metadata))}
// Copy metadata.
for id, m := range g.metadata {
result.metadata[id] = m
@@ -74,51 +74,25 @@
}
// Sort and filter file associations.
- //
- // We choose the first non-empty set of package associations out of the
- // following. For simplicity, call a non-command-line-arguments package a
- // "real" package.
- //
- // 1: valid real packages
- // 2: a valid command-line-arguments package
- // 3: invalid real packages
- // 4: an invalid command-line-arguments package
for uri, ids := range g.ids {
sort.Slice(ids, func(i, j int) bool {
- // 1. valid packages appear earlier.
- validi := g.metadata[ids[i]].Valid
- validj := g.metadata[ids[j]].Valid
- if validi != validj {
- return validi
- }
-
- // 2. command-line-args packages appear later.
cli := source.IsCommandLineArguments(ids[i])
clj := source.IsCommandLineArguments(ids[j])
if cli != clj {
return clj
}
- // 3. packages appear in name order.
+ // 2. packages appear in name order.
return ids[i] < ids[j]
})
// Choose the best IDs for each URI, according to the following rules:
// - If there are any valid real packages, choose them.
// - Else, choose the first valid command-line-argument package, if it exists.
- // - Else, keep using all the invalid metadata.
//
// TODO(rfindley): it might be better to track all IDs here, and exclude
// them later in PackagesForFile, but this is the existing behavior.
- hasValidMetadata := false
for i, id := range ids {
- m := g.metadata[id]
- if m.Valid {
- hasValidMetadata = true
- } else if hasValidMetadata {
- g.ids[uri] = ids[:i]
- break
- }
// If we've seen *anything* prior to command-line arguments package, take
// it. Note that ids[0] may itself be command-line-arguments.
if i > 0 && source.IsCommandLineArguments(id) {
@@ -134,7 +108,7 @@
//
// If includeInvalid is false, the algorithm ignores packages with invalid
// metadata (including those in the given list of ids).
-func (g *metadataGraph) reverseTransitiveClosure(includeInvalid bool, ids ...PackageID) map[PackageID]bool {
+func (g *metadataGraph) reverseTransitiveClosure(ids ...PackageID) map[PackageID]bool {
seen := make(map[PackageID]bool)
var visitAll func([]PackageID)
visitAll = func(ids []PackageID) {
@@ -144,7 +118,7 @@
}
m := g.metadata[id]
// Only use invalid metadata if we support it.
- if m == nil || !(m.Valid || includeInvalid) {
+ if m == nil {
continue
}
seen[id] = true
diff --git a/gopls/internal/lsp/cache/load.go b/gopls/internal/lsp/cache/load.go
index dcd343a..0498264 100644
--- a/gopls/internal/lsp/cache/load.go
+++ b/gopls/internal/lsp/cache/load.go
@@ -157,7 +157,7 @@
moduleErrs := make(map[string][]packages.Error) // module path -> errors
filterer := buildFilterer(s.view.rootURI.Filename(), s.view.gomodcache, s.view.Options())
- newMetadata := make(map[PackageID]*KnownMetadata)
+ newMetadata := make(map[PackageID]*Metadata)
for _, pkg := range pkgs {
// The Go command returns synthetic list results for module queries that
// encountered module errors.
@@ -222,10 +222,10 @@
//
// TODO(rfindley): perform a sanity check that metadata matches here. If not,
// we have an invalidation bug elsewhere.
- updates := make(map[PackageID]*KnownMetadata)
+ updates := make(map[PackageID]*Metadata)
var updatedIDs []PackageID
for _, m := range newMetadata {
- if existing := s.meta.metadata[m.ID]; existing == nil || !existing.Valid {
+ if existing := s.meta.metadata[m.ID]; existing == nil {
updates[m.ID] = m
updatedIDs = append(updatedIDs, m.ID)
delete(s.shouldLoad, m.ID)
@@ -238,7 +238,7 @@
//
// Note that the original metadata is being invalidated here, so we use the
// original metadata graph to compute the reverse closure.
- invalidatedPackages := s.meta.reverseTransitiveClosure(true, updatedIDs...)
+ invalidatedPackages := s.meta.reverseTransitiveClosure(updatedIDs...)
s.meta = s.meta.Clone(updates)
s.resetIsActivePackageLocked()
@@ -475,7 +475,7 @@
// buildMetadata populates the updates map with metadata updates to
// apply, based on the given pkg. It recurs through pkg.Imports to ensure that
// metadata exists for all dependencies.
-func buildMetadata(ctx context.Context, pkg *packages.Package, cfg *packages.Config, query []string, updates map[PackageID]*KnownMetadata, path []PackageID) error {
+func buildMetadata(ctx context.Context, pkg *packages.Package, cfg *packages.Config, query []string, updates map[PackageID]*Metadata, path []PackageID) error {
// Allow for multiple ad-hoc packages in the workspace (see #47584).
pkgPath := PackagePath(pkg.PkgPath)
id := PackageID(pkg.ID)
@@ -507,18 +507,15 @@
}
// Recreate the metadata rather than reusing it to avoid locking.
- m := &KnownMetadata{
- Metadata: &Metadata{
- ID: id,
- PkgPath: pkgPath,
- Name: PackageName(pkg.Name),
- ForTest: PackagePath(packagesinternal.GetForTest(pkg)),
- TypesSizes: pkg.TypesSizes,
- Config: cfg,
- Module: pkg.Module,
- depsErrors: packagesinternal.GetDepsErrors(pkg),
- },
- Valid: true,
+ m := &Metadata{
+ ID: id,
+ PkgPath: pkgPath,
+ Name: PackageName(pkg.Name),
+ ForTest: PackagePath(packagesinternal.GetForTest(pkg)),
+ TypesSizes: pkg.TypesSizes,
+ Config: cfg,
+ Module: pkg.Module,
+ depsErrors: packagesinternal.GetDepsErrors(pkg),
}
updates[id] = m
@@ -650,7 +647,7 @@
// the snapshot s.
//
// s.mu must be held while calling this function.
-func containsOpenFileLocked(s *snapshot, m *KnownMetadata) bool {
+func containsOpenFileLocked(s *snapshot, m *Metadata) bool {
uris := map[span.URI]struct{}{}
for _, uri := range m.CompiledGoFiles {
uris[uri] = struct{}{}
@@ -700,14 +697,7 @@
func computeWorkspacePackagesLocked(s *snapshot, meta *metadataGraph) map[PackageID]PackagePath {
workspacePackages := make(map[PackageID]PackagePath)
for _, m := range meta.metadata {
- // Don't consider invalid packages to be workspace packages. Doing so can
- // result in type-checking and diagnosing packages that no longer exist,
- // which can lead to memory leaks and confusing errors.
- if !m.Valid {
- continue
- }
-
- if !containsPackageLocked(s, m.Metadata) {
+ if !containsPackageLocked(s, m) {
continue
}
@@ -748,12 +738,12 @@
// function returns false.
//
// If m is not a command-line-arguments package, this is trivially true.
-func allFilesHaveRealPackages(g *metadataGraph, m *KnownMetadata) bool {
+func allFilesHaveRealPackages(g *metadataGraph, m *Metadata) bool {
n := len(m.CompiledGoFiles)
checkURIs:
for _, uri := range append(m.CompiledGoFiles[0:n:n], m.GoFiles...) {
for _, id := range g.ids[uri] {
- if !source.IsCommandLineArguments(id) && (g.metadata[id].Valid || !m.Valid) {
+ if !source.IsCommandLineArguments(id) {
continue checkURIs
}
}
diff --git a/gopls/internal/lsp/cache/metadata.go b/gopls/internal/lsp/cache/metadata.go
index 03037f0..135623f 100644
--- a/gopls/internal/lsp/cache/metadata.go
+++ b/gopls/internal/lsp/cache/metadata.go
@@ -86,12 +86,3 @@
func (m *Metadata) ModuleInfo() *packages.Module {
return m.Module
}
-
-// KnownMetadata is a wrapper around metadata that tracks its validity.
-type KnownMetadata struct {
- *Metadata
-
- // Valid is true if the given metadata is Valid.
- // Invalid metadata can still be used if a metadata reload fails.
- Valid bool
-}
diff --git a/gopls/internal/lsp/cache/mod_tidy.go b/gopls/internal/lsp/cache/mod_tidy.go
index cc604c1..fc97ee6 100644
--- a/gopls/internal/lsp/cache/mod_tidy.go
+++ b/gopls/internal/lsp/cache/mod_tidy.go
@@ -196,7 +196,7 @@
}
// Read both lists of files of this package, in parallel.
- goFiles, compiledGoFiles, err := readGoFiles(ctx, snapshot, m.Metadata)
+ goFiles, compiledGoFiles, err := readGoFiles(ctx, snapshot, m)
if err != nil {
return nil, err
}
diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go
index 70c26f2..656d08f 100644
--- a/gopls/internal/lsp/cache/snapshot.go
+++ b/gopls/internal/lsp/cache/snapshot.go
@@ -729,8 +729,6 @@
// If experimentalUseInvalidMetadata is set, this function may return package
// IDs with invalid metadata.
func (s *snapshot) getOrLoadIDsForURI(ctx context.Context, uri span.URI) ([]PackageID, error) {
- useInvalidMetadata := s.useInvalidMetadata()
-
s.mu.Lock()
// Start with the set of package associations derived from the last load.
@@ -741,7 +739,7 @@
for _, id := range ids {
// TODO(rfindley): remove the defensiveness here. s.meta.metadata[id] must
// exist.
- if m, ok := s.meta.metadata[id]; ok && m.Valid {
+ if _, ok := s.meta.metadata[id]; ok {
hasValidID = true
}
if len(s.shouldLoad[id]) > 0 {
@@ -757,16 +755,6 @@
s.mu.Unlock()
- // Special case: if experimentalUseInvalidMetadata is set and we have any
- // ids, just return them.
- //
- // This is arguably wrong: if the metadata is invalid we should try reloading
- // it. However, this was the pre-existing behavior, and
- // experimentalUseInvalidMetadata will be removed in a future release.
- if !shouldLoad && useInvalidMetadata && len(ids) > 0 {
- return ids, nil
- }
-
// Reload if loading is likely to improve the package associations for uri:
// - uri is not contained in any valid packages
// - ...or one of the packages containing uri is marked 'shouldLoad'
@@ -800,28 +788,19 @@
s.mu.Lock()
ids = s.meta.ids[uri]
- if !useInvalidMetadata {
- var validIDs []PackageID
- for _, id := range ids {
- // TODO(rfindley): remove the defensiveness here as well.
- if m, ok := s.meta.metadata[id]; ok && m.Valid {
- validIDs = append(validIDs, id)
- }
+ var validIDs []PackageID
+ for _, id := range ids {
+ // TODO(rfindley): remove the defensiveness here as well.
+ if _, ok := s.meta.metadata[id]; ok {
+ validIDs = append(validIDs, id)
}
- ids = validIDs
}
+ ids = validIDs
s.mu.Unlock()
return ids, nil
}
-// Only use invalid metadata for Go versions >= 1.13. Go 1.12 and below has
-// issues with overlays that will cause confusing error messages if we reuse
-// old metadata.
-func (s *snapshot) useInvalidMetadata() bool {
- return s.view.goversion >= 13 && s.view.Options().ExperimentalUseInvalidMetadata
-}
-
func (s *snapshot) GetReverseDependencies(ctx context.Context, id PackageID) ([]source.Package, error) {
if err := s.awaitLoaded(ctx); err != nil {
return nil, err
@@ -829,7 +808,7 @@
s.mu.Lock()
meta := s.meta
s.mu.Unlock()
- ids := meta.reverseTransitiveClosure(s.useInvalidMetadata(), id)
+ ids := meta.reverseTransitiveClosure(id)
// Make sure to delete the original package ID from the map.
delete(ids, id)
@@ -1216,9 +1195,7 @@
var meta []source.Metadata
for _, m := range s.meta.metadata {
- if m.Valid {
- meta = append(meta, m)
- }
+ meta = append(meta, m)
}
return meta, nil
}
@@ -1273,7 +1250,7 @@
return match
}
-func (s *snapshot) getMetadata(id PackageID) *KnownMetadata {
+func (s *snapshot) getMetadata(id PackageID) *Metadata {
s.mu.Lock()
defer s.mu.Unlock()
@@ -1319,7 +1296,7 @@
return true
}
for _, id := range ids {
- if m, ok := s.meta.metadata[id]; ok && m.Valid {
+ if _, ok := s.meta.metadata[id]; ok {
return false
}
}
@@ -1409,19 +1386,8 @@
func (s *snapshot) awaitLoaded(ctx context.Context) error {
loadErr := s.awaitLoadedAllErrors(ctx)
- s.mu.Lock()
- defer s.mu.Unlock()
-
- // If we still have absolutely no metadata, check if the view failed to
- // initialize and return any errors.
- if s.useInvalidMetadata() && len(s.meta.metadata) > 0 {
- return nil
- }
- for _, m := range s.meta.metadata {
- if m.Valid {
- return nil
- }
- }
+ // TODO(rfindley): eliminate this function as part of simplifying
+ // CriticalErrors.
if loadErr != nil {
return loadErr.MainError
}
@@ -1968,27 +1934,10 @@
result.shouldLoad[k] = v
}
- // TODO(rfindley): consolidate the this workspace mode detection with
- // workspace invalidation.
- workspaceModeChanged := s.workspaceMode() != result.workspaceMode()
-
- // We delete invalid metadata in the following cases:
- // - If we are forcing a reload of metadata.
- // - If the workspace mode has changed, as stale metadata may produce
- // confusing or incorrect diagnostics.
- //
- // TODO(rfindley): we should probably also clear metadata if we are
- // reinitializing the workspace, as otherwise we could leave around a bunch
- // of irrelevant and duplicate metadata (for example, if the module path
- // changed). However, this breaks the "experimentalUseInvalidMetadata"
- // feature, which relies on stale metadata when, for example, a go.mod file
- // is broken via invalid syntax.
- deleteInvalidMetadata := forceReloadMetadata || workspaceModeChanged
-
// Compute which metadata updates are required. We only need to invalidate
// packages directly containing the affected file, and only if it changed in
// a relevant way.
- metadataUpdates := make(map[PackageID]*KnownMetadata)
+ metadataUpdates := make(map[PackageID]*Metadata)
for k, v := range s.meta.metadata {
invalidateMetadata := idsToInvalidate[k]
@@ -2012,20 +1961,10 @@
}
// Check whether the metadata should be deleted.
- if skipID[k] || (invalidateMetadata && deleteInvalidMetadata) {
+ if skipID[k] || invalidateMetadata {
metadataUpdates[k] = nil
continue
}
-
- // Check if the metadata has changed.
- valid := v.Valid && !invalidateMetadata
- if valid != v.Valid {
- // Mark invalidated metadata rather than deleting it outright.
- metadataUpdates[k] = &KnownMetadata{
- Metadata: v.Metadata,
- Valid: valid,
- }
- }
}
// Update metadata, if necessary.
@@ -2042,6 +1981,10 @@
// Don't bother copying the importedBy graph,
// as it changes each time we update metadata.
+ // TODO(rfindley): consolidate the this workspace mode detection with
+ // workspace invalidation.
+ workspaceModeChanged := s.workspaceMode() != result.workspaceMode()
+
// If the snapshot's workspace mode has changed, the packages loaded using
// the previous mode are no longer relevant, so clear them out.
if workspaceModeChanged {
