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 <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
gopls-CI: kokoro <[email protected]>
Run-TryBot: Robert Findley <[email protected]>

Author: findleyr

gopls/doc/settings.md

@@ -161,19 +161,6 @@ be removed.
 
 Default: `false`.
 
-#### **experimentalUseInvalidMetadata** *bool*
-
-**This setting is experimental and may be deleted.**
-
-experimentalUseInvalidMetadata enables gopls to fall back on outdated
-package metadata to provide editor features if the go command fails to
-load packages for some reason (like an invalid go.mod file).
-
-Deprecated: this setting is deprecated and will be removed in a future
-version of gopls (https://go.dev/issue/55333).
-
-Default: `false`.
-
 #### **standaloneTags** *[]string*
 
 standaloneTags specifies a set of build constraints that identify

gopls/internal/lsp/cache/check.go

@@ -46,7 +46,7 @@ type packageHandle struct {
 	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 @@ func (s *snapshot) buildPackageHandle(ctx context.Context, id PackageID, mode so
 		// 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 @@ func (s *snapshot) buildPackageHandle(ctx context.Context, id PackageID, mode so
 	// 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 @@ func (s *snapshot) buildPackageHandle(ctx context.Context, id PackageID, mode so
 	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,15 +162,15 @@ func (s *snapshot) buildPackageHandle(ctx context.Context, id PackageID, mode so
 	// 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)
 	}
 
 	// Check cache again in case another thread got there first.
 	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 @@ func (s *snapshot) workspaceParseMode(id PackageID) source.ParseMode {
 // 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)

gopls/internal/lsp/cache/debug.go

@@ -47,7 +47,7 @@ func (s *snapshot) dumpWorkspace(context string) {
 
 	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)
 	}
 }

gopls/internal/lsp/cache/graph.go

@@ -15,7 +15,7 @@ import (
 // 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 @@ type metadataGraph struct {
 
 // 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 @@ func (g *metadataGraph) build() {
 	}
 
 	// 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 @@ func (g *metadataGraph) build() {
 //
 // 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 @@ func (g *metadataGraph) reverseTransitiveClosure(includeInvalid bool, ids ...Pac
 			}
 			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

gopls/internal/lsp/cache/load.go

@@ -157,7 +157,7 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc
 
 	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 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc
 	//
 	// 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 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc
 	//
 	// 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 @@ func makeWorkspaceDir(ctx context.Context, workspace *workspace, fs source.FileS
 // 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 @@ func buildMetadata(ctx context.Context, pkg *packages.Package, cfg *packages.Con
 	}
 
 	// 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 @@ func containsPackageLocked(s *snapshot, m *Metadata) bool {
 // 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 containsFileInWorkspaceLocked(s *snapshot, m *Metadata) bool {
 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 @@ func computeWorkspacePackagesLocked(s *snapshot, meta *metadataGraph) map[Packag
 // 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
 			}
 		}

gopls/internal/lsp/cache/metadata.go

@@ -86,12 +86,3 @@ func (m *Metadata) IsIntermediateTestVariant() bool {
 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
-}

gopls/internal/lsp/cache/mod_tidy.go

@@ -196,7 +196,7 @@ func modTidyDiagnostics(ctx context.Context, snapshot *snapshot, pm *source.Pars
 		}
 
 		// 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
 		}