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])) }