diff --git a/gopls/internal/regtest/workspace/workspace_test.go b/gopls/internal/regtest/workspace/workspace_test.go index ed2c9effda..9289b83735 100644 --- a/gopls/internal/regtest/workspace/workspace_test.go +++ b/gopls/internal/regtest/workspace/workspace_test.go @@ -1241,3 +1241,52 @@ use ( env.Await(NoOutstandingDiagnostics()) }) } + +// Tests the fix for golang/go#52500. +func TestChangeTestVariant_Issue52500(t *testing.T) { + // This test fails for unknown reasons at Go <= 15. Presumably the loading of + // test variants behaves differently, possibly due to lack of support for + // native overlays. + testenv.NeedsGo1Point(t, 16) + const src = ` +-- go.mod -- +module mod.test + +go 1.12 +-- main_test.go -- +package main_test + +type Server struct{} + +const mainConst = otherConst +-- other_test.go -- +package main_test + +const otherConst = 0 + +func (Server) Foo() {} +` + + Run(t, src, func(t *testing.T, env *Env) { + env.OpenFile("other_test.go") + env.RegexpReplace("other_test.go", "main_test", "main") + + // For this test to function, it is necessary to wait on both of the + // expectations below: the bug is that when switching the package name in + // other_test.go from main->main_test, metadata for main_test is not marked + // as invalid. So we need to wait for the metadata of main_test.go to be + // updated before moving other_test.go back to the main_test package. + env.Await( + env.DiagnosticAtRegexpWithMessage("other_test.go", "Server", "undeclared"), + env.DiagnosticAtRegexpWithMessage("main_test.go", "otherConst", "undeclared"), + ) + env.RegexpReplace("other_test.go", "main", "main_test") + env.Await( + EmptyDiagnostics("other_test.go"), + EmptyDiagnostics("main_test.go"), + ) + + // This will cause a test failure if other_test.go is not in any package. + _, _ = env.GoToDefinition("other_test.go", env.RegexpSearch("other_test.go", "Server")) + }) +} diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index 9b3ba767d1..5cc534aa79 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -1839,8 +1839,7 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC anyImportDeleted = anyImportDeleted || importDeleted // Mark all of the package IDs containing the given file. - // TODO: if the file has moved into a new package, we should invalidate that too. - filePackageIDs := guessPackageIDsForURI(uri, s.ids) + filePackageIDs := invalidatedPackageIDs(uri, s.ids, originalFH == nil || pkgNameChanged) if pkgNameChanged { for _, id := range filePackageIDs { changedPkgNames[id] = struct{}{} @@ -2089,15 +2088,22 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC return result } -// guessPackageIDsForURI returns all packages related to uri. If we haven't -// seen this URI before, we guess based on files in the same directory. This -// is of course incorrect in build systems where packages are not organized by -// directory. -func guessPackageIDsForURI(uri span.URI, known map[span.URI][]PackageID) []PackageID { - packages := known[uri] - if len(packages) > 0 { - // We've seen this file before. - return packages +// invalidatedPackageIDs returns all packages invalidated by a change to uri. +// If we haven't seen this URI before, we guess based on files in the same +// directory. This is of course incorrect in build systems where packages are +// not organized by directory. +// +// If newPackageFile is set, the file is either a new file, or has a new +// package name. In this case, all known packages in the directory will be +// invalidated. +func invalidatedPackageIDs(uri span.URI, known map[span.URI][]PackageID, newPackageFile bool) []PackageID { + // If the file didn't move to a new package, we should only invalidate the + // packages it is currently contained inside. + if !newPackageFile { + if packages := known[uri]; len(packages) > 0 { + // We've seen this file before. + return packages + } } // This is a file we don't yet know about. Guess relevant packages by // considering files in the same directory.