internal/lsp: handle deletion of workspace packages

When a workspace package is deleted, we need to stop trying to load it.
Check that every workspace package still has at least one .go file when
we copy them between snapshots.

I think this is correct, but even if it's not, orphaned file loading
should patch it up.

Fixes golang/go#38977.

Change-Id: I0b11010a40aac09f619f54b5ba02e2467b15a36c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/238028
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>

internal/lsp/cache/snapshot.go

@@ -747,7 +747,6 @@ func (s *snapshot) clone(ctx context.Context, withoutURIs map[span.URI]source.Fi
 	// If an ID is present in the map, invalidate its types.
 	// If an ID's value is true, invalidate its metadata too.
 	transitiveIDs := make(map[packageID]bool)
-
 	for withoutURI, currentFH := range withoutURIs {
 		directIDs := map[packageID]struct{}{}
 
@@ -847,25 +846,38 @@ func (s *snapshot) clone(ctx context.Context, withoutURIs map[span.URI]source.Fi
 	}
 	// Copy the URI to package ID mappings, skipping only those URIs whose
 	// metadata will be reloaded in future calls to load.
-outer:
+copyIDs:
 	for k, ids := range s.ids {
 		for _, id := range ids {
 			if invalidateMetadata, ok := transitiveIDs[id]; invalidateMetadata && ok {
-				continue outer
+				continue copyIDs
 			}
 		}
 		result.ids[k] = ids
 	}
 	// Copy the set of initally loaded packages.
 	for id, pkgPath := range s.workspacePackages {
-		// TODO(rstambler): For now, we only invalidate "command-line-arguments"
-		// from workspace packages, but in general, we need to handle deletion
-		// of a package.
 		if id == "command-line-arguments" {
 			if invalidateMetadata, ok := transitiveIDs[id]; invalidateMetadata && ok {
 				continue
 			}
 		}
+
+		// If all the files we know about in a package have been deleted,
+		// the package is gone and we should no longer try to load it.
+		if m := s.metadata[id]; m != nil {
+			hasFiles := false
+			for _, uri := range s.metadata[id].goFiles {
+				if _, ok := result.files[uri]; ok {
+					hasFiles = true
+					break
+				}
+			}
+			if !hasFiles {
+				continue
+			}
+		}
+
 		result.workspacePackages[id] = pkgPath
 	}
 	// Don't bother copying the importedBy graph,

internal/lsp/regtest/diagnostics_test.go

@@ -131,7 +131,7 @@ func TestDiagnosticClearingOnDelete_Issue37049(t *testing.T) {
 	runner.Run(t, badPackage, func(t *testing.T, env *Env) {
 		env.OpenFile("a.go")
 		env.Await(env.DiagnosticAtRegexp("a.go", "a = 1"), env.DiagnosticAtRegexp("b.go", "a = 2"))
-		env.RemoveFileFromWorkspace("b.go")
+		env.RemoveWorkspaceFile("b.go")
 
 		env.Await(EmptyDiagnostics("a.go"), EmptyDiagnostics("b.go"))
 	})
@@ -206,7 +206,7 @@ func TestRmTest38878Close(t *testing.T) {
 		env.OpenFile("a_test.go")
 		env.Await(DiagnosticAt("a_test.go", 5, 3))
 		env.CloseBuffer("a_test.go")
-		env.RemoveFileFromWorkspace("a_test.go")
+		env.RemoveWorkspaceFile("a_test.go")
 		// diagnostics go away
 		env.Await(EmptyDiagnostics("a_test.go"))
 	})
@@ -822,3 +822,43 @@ var _ = foo.Bar
 			))
 	})
 }
+
+// Partially reproduces golang/go#38977, moving a file between packages.
+// It also gets hit by some go command bug fixed in 1.15, but we don't
+// care about that so much here.
+func TestDeletePackage(t *testing.T) {
+	const ws = `
+-- go.mod --
+module mod.com
+
+go 1.15
+-- a/a.go --
+package a
+
+const A = 1
+
+-- b/b.go --
+package b
+
+import "mod.com/a"
+
+const B = a.A
+
+-- c/c.go --
+package c
+
+import "mod.com/a"
+
+const C = a.A
+`
+	runner.Run(t, ws, func(t *testing.T, env *Env) {
+		env.OpenFile("b/b.go")
+		env.Await(CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidOpen), 1))
+		// Delete c/c.go, the only file in package c.
+		env.RemoveWorkspaceFile("c/c.go")
+
+		// We should still get diagnostics for files that exist.
+		env.RegexpReplace("b/b.go", `a.A`, "a.Nonexistant")
+		env.Await(env.DiagnosticAtRegexp("b/b.go", `Nonexistant`))
+	})
+}

internal/lsp/regtest/wrappers.go

@@ -14,9 +14,9 @@ import (
 	"golang.org/x/tools/internal/lsp/protocol"
 )
 
-// RemoveFileFromWorkspace deletes a file on disk but does nothing in the
+// RemoveWorkspaceFile deletes a file on disk but does nothing in the
 // editor. It calls t.Fatal on any error.
-func (e *Env) RemoveFileFromWorkspace(name string) {
+func (e *Env) RemoveWorkspaceFile(name string) {
 	e.T.Helper()
 	if err := e.Sandbox.Workdir.RemoveFile(e.Ctx, name); err != nil {
 		e.T.Fatal(err)
@@ -34,6 +34,15 @@ func (e *Env) ReadWorkspaceFile(name string) string {
 	return content
 }
 
+// RemoveFileFromWorkspace deletes a file on disk but does nothing in the
+// editor. It calls t.Fatal on any error.
+func (e *Env) WriteWorkspaceFile(name, content string) {
+	e.T.Helper()
+	if err := e.Sandbox.Workdir.WriteFile(e.Ctx, name, content); err != nil {
+		e.T.Fatal(err)
+	}
+}
+
 // OpenFile opens a file in the editor, calling t.Fatal on any error.
 func (e *Env) OpenFile(name string) {
 	e.T.Helper()