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>

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"))
+	})
+}

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.