internal/lsp/cache: invalid packages should not be workspace packages

To support the experimentalUseInvalidMetadata mode, we keep around
invalid metadata. This can help gopls features work when, for example,
the go.mod file is broken. It is debatable whether this feature is worth
supporting (see golang/go#54180), but in the meantime there is a very
negative side-effect when module paths are changed in the go.mod file:
we keep around a bunch of workspace packages with a stale module path.
As a result we can be left with a lots of extra type-checked packages
in memory, and many inaccurate diagnostics.

Fix this by skipping packages with invalid metadata when computing
workspace packages. While we may want to use invalid metadata when
finding the best package for a file, it does not make sense to re-
type-check and diagnose all those stale packages.

Fixes golang/go#43186

Change-Id: Id73b47ea138ec80a9de63b03dae41d4e509b8d5a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/420956
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Suzy Mueller <suzmue@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>

gopls/internal/regtest/workspace/broken_test.go

@@ -115,3 +115,55 @@ const CompleteMe = 222
 		}
 	})
 }
+
+// Test for golang/go#43186: correcting the module path should fix errors
+// without restarting gopls.
+func TestBrokenWorkspace_WrongModulePath(t *testing.T) {
+	const files = `
+-- go.mod --
+module mod.testx
+
+go 1.18
+-- p/internal/foo/foo.go --
+package foo
+
+const C = 1
+-- p/internal/bar/bar.go --
+package bar
+
+import "mod.test/p/internal/foo"
+
+const D = foo.C + 1
+-- p/internal/bar/bar_test.go --
+package bar_test
+
+import (
+	"mod.test/p/internal/foo"
+	. "mod.test/p/internal/bar"
+)
+
+const E = D + foo.C
+-- p/internal/baz/baz_test.go --
+package baz_test
+
+import (
+	named "mod.test/p/internal/bar"
+)
+
+const F = named.D - 3
+`
+
+	Run(t, files, func(t *testing.T, env *Env) {
+		env.OpenFile("p/internal/bar/bar.go")
+		env.Await(
+			OnceMet(
+				env.DoneWithOpen(),
+				env.DiagnosticAtRegexp("p/internal/bar/bar.go", "\"mod.test/p/internal/foo\""),
+			),
+		)
+		env.OpenFile("go.mod")
+		env.RegexpReplace("go.mod", "mod.testx", "mod.test")
+		env.SaveBuffer("go.mod") // saving triggers a reload
+		env.Await(NoOutstandingDiagnostics())
+	})
+}

internal/lsp/cache/check.go

@@ -355,6 +355,7 @@ func typeCheckImpl(ctx context.Context, snapshot *snapshot, goFiles, compiledGoF
 			missing, unexpected = filter.ProcessErrors(pkg.typeErrors)
 		}
 		if len(unexpected) != 0 || len(missing) != 0 {
+			// TODO(rfindley): remove this distracting log
 			event.Log(ctx, fmt.Sprintf("falling back to safe trimming due to type errors: %v or still-missing identifiers: %v", unexpected, missing), tag.Package.Of(string(m.ID)))
 			pkg, err = doTypeCheck(ctx, snapshot, goFiles, compiledGoFiles, m, mode, deps, nil)
 			if err != nil {

internal/lsp/cache/load.go

@@ -635,6 +635,12 @@ 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) {
 			continue
 		}

internal/lsp/cache/snapshot.go

@@ -1874,13 +1874,8 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC
 		result.actions.Delete(key)
 	}
 
-	// If the workspace mode has changed, we must delete all metadata, as it
-	// is unusable and may produce confusing or incorrect diagnostics.
 	// If a file has been deleted, we must delete metadata for all packages
 	// containing that file.
-	workspaceModeChanged := s.workspaceMode() != result.workspaceMode()
-
-	// Don't keep package metadata for packages that have lost files.
 	//
 	// TODO(rfindley): why not keep invalid metadata in this case? If we
 	// otherwise allow operate on invalid metadata, why not continue to do so,
@@ -1907,11 +1902,27 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC
 		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)
-	deleteInvalidMetadata := forceReloadMetadata || workspaceModeChanged
 	for k, v := range s.meta.metadata {
 		invalidateMetadata := idsToInvalidate[k]
 

internal/lsp/source/util.go

@@ -555,6 +555,8 @@ func IsValidImport(pkgPath, importPkgPath string) bool {
 	if IsCommandLineArguments(string(pkgPath)) {
 		return true
 	}
+	// TODO(rfindley): this is wrong. mod.testx/p should not be able to
+	// import mod.test/internal: https://go.dev/play/p/-Ca6P-E4V4q
 	return strings.HasPrefix(string(pkgPath), string(importPkgPath[:i]))
 }