internal/lsp/cache: invalidate packages that have added files
When a file is new or its package name has changed, we should invalidate
all packages for files in the current directory, not just packages
previously containing the file.
Fixes golang/go#52500
Change-Id: I9fc9857a7abcd4e730068871c899d274e1736967
Reviewed-on: https://go-review.googlesource.com/c/tools/+/401795
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Suzy Mueller <suzmue@golang.org>
diff --git a/gopls/internal/regtest/workspace/workspace_test.go b/gopls/internal/regtest/workspace/workspace_test.go
index ed2c9ef..9289b83 100644
--- a/gopls/internal/regtest/workspace/workspace_test.go
+++ b/gopls/internal/regtest/workspace/workspace_test.go
@@ -1241,3 +1241,52 @@
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 9b3ba76..5cc534a 100644
--- a/internal/lsp/cache/snapshot.go
+++ b/internal/lsp/cache/snapshot.go
@@ -1839,8 +1839,7 @@
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 @@
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.
