From 01c9ff053696e40c9604de2bbd7960e82f6bc6df Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Wed, 3 Aug 2022 13:07:09 -0400 Subject: [PATCH] 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 Reviewed-by: Suzy Mueller gopls-CI: kokoro TryBot-Result: Gopher Robot --- .../internal/regtest/workspace/broken_test.go | 52 +++++++++++++++++++ internal/lsp/cache/check.go | 1 + internal/lsp/cache/load.go | 6 +++ internal/lsp/cache/snapshot.go | 23 +++++--- internal/lsp/source/util.go | 2 + 5 files changed, 78 insertions(+), 6 deletions(-) diff --git a/gopls/internal/regtest/workspace/broken_test.go b/gopls/internal/regtest/workspace/broken_test.go index cd5127ff3a..e88b98b50e 100644 --- a/gopls/internal/regtest/workspace/broken_test.go +++ b/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()) + }) +} diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go index 6c02d5348f..6beee1f7be 100644 --- a/internal/lsp/cache/check.go +++ b/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 { diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index ca906c8287..8afbb2e4ff 100644 --- a/internal/lsp/cache/load.go +++ b/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 } diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index 6d3ead5c37..5a51ae13e0 100644 --- a/internal/lsp/cache/snapshot.go +++ b/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] diff --git a/internal/lsp/source/util.go b/internal/lsp/source/util.go index 262447cf36..78448af417 100644 --- a/internal/lsp/source/util.go +++ b/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])) }