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>
diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go
index 0d400c8..16857be 100644
--- a/internal/lsp/cache/snapshot.go
+++ b/internal/lsp/cache/snapshot.go
@@ -747,7 +747,6 @@
// 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 @@
}
// 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,
diff --git a/internal/lsp/regtest/diagnostics_test.go b/internal/lsp/regtest/diagnostics_test.go
index e1452d8..39eb67c 100644
--- a/internal/lsp/regtest/diagnostics_test.go
+++ b/internal/lsp/regtest/diagnostics_test.go
@@ -131,7 +131,7 @@
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 @@
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 @@
))
})
}
+
+// 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`))
+ })
+}
diff --git a/internal/lsp/regtest/wrappers.go b/internal/lsp/regtest/wrappers.go
index d25b039..e69e7ca 100644
--- a/internal/lsp/regtest/wrappers.go
+++ b/internal/lsp/regtest/wrappers.go
@@ -14,9 +14,9 @@
"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 @@
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()
