internal/lsp/cache: invalidate broken packages when imports are deleted
In the presence of an import cycle, packages.Load will exclude the
problematic import from Package.Imports. This missing edge means that we
wouldn't correctly invalidate broken package metadata when a problematic
import is deleted. We also weren't treating import deletion as a change
that could invalidate metadata.
Fix this by invalidating any broken packages when an import path is
deleted, anywhere. This could be overly coarse, but there are various
problems with trying to do better.
Also change cycle errors from TypeError to ListError, as this
categorization was misleading.
Fixes golang/go#46667
Change-Id: Id19e7baf4009632caf3ae9365497552f9b64b5c7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/326589
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
diff --git a/gopls/internal/regtest/diagnostics/diagnostics_test.go b/gopls/internal/regtest/diagnostics/diagnostics_test.go
index 601376a..13be65d 100644
--- a/gopls/internal/regtest/diagnostics/diagnostics_test.go
+++ b/gopls/internal/regtest/diagnostics/diagnostics_test.go
@@ -1616,6 +1616,42 @@
})
}
+// Tests golang/go#46667: deleting a problematic import path should resolve
+// import cycle errors.
+func TestResolveImportCycle(t *testing.T) {
+ const mod = `
+-- go.mod --
+module mod.test
+
+go 1.16
+-- a/a.go --
+package a
+
+import "mod.test/b"
+
+const A = b.A
+const B = 2
+-- b/b.go --
+package b
+
+import "mod.test/a"
+
+const A = 1
+const B = a.B
+ `
+ Run(t, mod, func(t *testing.T, env *Env) {
+ env.OpenFile("a/a.go")
+ env.OpenFile("b/b.go")
+ env.Await(env.DiagnosticAtRegexp("a/a.go", `"mod.test/b"`))
+ env.RegexpReplace("b/b.go", `const B = a\.B`, "")
+ env.SaveBuffer("b/b.go")
+ env.Await(
+ EmptyDiagnostics("a/a.go"),
+ EmptyDiagnostics("b/b.go"),
+ )
+ })
+}
+
func TestBadImport(t *testing.T) {
testenv.NeedsGo1Point(t, 14)
diff --git a/internal/lsp/cache/errors.go b/internal/lsp/cache/errors.go
index 6cc3e45..402f2ae 100644
--- a/internal/lsp/cache/errors.go
+++ b/internal/lsp/cache/errors.go
@@ -34,7 +34,7 @@
URI: spn.URI(),
Range: rng,
Severity: protocol.SeverityError,
- Source: source.TypeError,
+ Source: source.ListError,
Message: msg,
}}, nil
}
diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go
index 1e93d07..1eee2de 100644
--- a/internal/lsp/cache/snapshot.go
+++ b/internal/lsp/cache/snapshot.go
@@ -1488,6 +1488,7 @@
}
changedPkgNames := map[packageID]struct{}{}
+ anyImportDeleted := false
for uri, change := range changes {
// Maybe reinitialize the view if we see a change in the vendor
// directory.
@@ -1500,7 +1501,8 @@
// Check if the file's package name or imports have changed,
// and if so, invalidate this file's packages' metadata.
- shouldInvalidateMetadata, pkgNameChanged := s.shouldInvalidateMetadata(ctx, result, originalFH, change.fileHandle)
+ shouldInvalidateMetadata, pkgNameChanged, importDeleted := s.shouldInvalidateMetadata(ctx, result, originalFH, change.fileHandle)
+ anyImportDeleted = anyImportDeleted || importDeleted
invalidateMetadata := forceReloadMetadata || workspaceReload || shouldInvalidateMetadata
// Mark all of the package IDs containing the given file.
@@ -1541,6 +1543,27 @@
delete(result.unloadableFiles, uri)
}
+ // Deleting an import can cause list errors due to import cycles to be
+ // resolved. The best we can do without parsing the list error message is to
+ // hope that list errors may have been resolved by a deleted import.
+ //
+ // We could do better by parsing the list error message. We already do this
+ // to assign a better range to the list error, but for such critical
+ // functionality as metadata, it's better to be conservative until it proves
+ // impractical.
+ //
+ // We could also do better by looking at which imports were deleted and
+ // trying to find cycles they are involved in. This fails when the file goes
+ // from an unparseable state to a parseable state, as we don't have a
+ // starting point to compare with.
+ if anyImportDeleted {
+ for id, metadata := range s.metadata {
+ if len(metadata.errors) > 0 {
+ directIDs[id] = true
+ }
+ }
+ }
+
// Invalidate reverse dependencies too.
// TODO(heschi): figure out the locking model and use transitiveReverseDeps?
// transitiveIDs keeps track of transitive reverse dependencies.
@@ -1745,13 +1768,13 @@
// shouldInvalidateMetadata reparses a file's package and import declarations to
// determine if the file requires a metadata reload.
-func (s *snapshot) shouldInvalidateMetadata(ctx context.Context, newSnapshot *snapshot, originalFH, currentFH source.FileHandle) (invalidate, pkgNameChanged bool) {
+func (s *snapshot) shouldInvalidateMetadata(ctx context.Context, newSnapshot *snapshot, originalFH, currentFH source.FileHandle) (invalidate, pkgNameChanged, importDeleted bool) {
if originalFH == nil {
- return true, false
+ return true, false, false
}
// If the file hasn't changed, there's no need to reload.
if originalFH.FileIdentity() == currentFH.FileIdentity() {
- return false, false
+ return false, false, false
}
// Get the original and current parsed files in order to check package name
// and imports. Use the new snapshot to parse to avoid modifying the
@@ -1759,53 +1782,77 @@
original, originalErr := newSnapshot.ParseGo(ctx, originalFH, source.ParseHeader)
current, currentErr := newSnapshot.ParseGo(ctx, currentFH, source.ParseHeader)
if originalErr != nil || currentErr != nil {
- return (originalErr == nil) != (currentErr == nil), false
+ return (originalErr == nil) != (currentErr == nil), false, (currentErr != nil) // we don't know if an import was deleted
}
// Check if the package's metadata has changed. The cases handled are:
// 1. A package's name has changed
// 2. A file's imports have changed
if original.File.Name.Name != current.File.Name.Name {
- return true, true
+ invalidate = true
+ pkgNameChanged = true
}
- importSet := make(map[string]struct{})
+ origImportSet := make(map[string]struct{})
for _, importSpec := range original.File.Imports {
- importSet[importSpec.Path.Value] = struct{}{}
+ origImportSet[importSpec.Path.Value] = struct{}{}
+ }
+ curImportSet := make(map[string]struct{})
+ for _, importSpec := range current.File.Imports {
+ curImportSet[importSpec.Path.Value] = struct{}{}
}
// If any of the current imports were not in the original imports.
- for _, importSpec := range current.File.Imports {
- if _, ok := importSet[importSpec.Path.Value]; ok {
+ for path := range curImportSet {
+ if _, ok := origImportSet[path]; ok {
+ delete(origImportSet, path)
continue
}
// If the import path is obviously not valid, we can skip reloading
// metadata. For now, valid means properly quoted and without a
// terminal slash.
- path, err := strconv.Unquote(importSpec.Path.Value)
- if err != nil {
+ if isBadImportPath(path) {
continue
}
- if path == "" {
- continue
- }
- if path[len(path)-1] == '/' {
- continue
- }
- return true, false
+ invalidate = true
}
- // Re-evaluate build constraints and embed patterns. It would be preferable
- // to only do this on save, but we don't have the prior versions accessible.
- oldComments := extractMagicComments(original.File)
- newComments := extractMagicComments(current.File)
+ for path := range origImportSet {
+ if !isBadImportPath(path) {
+ invalidate = true
+ importDeleted = true
+ }
+ }
+
+ if !invalidate {
+ invalidate = magicCommentsChanged(original.File, current.File)
+ }
+ return invalidate, pkgNameChanged, importDeleted
+}
+
+func magicCommentsChanged(original *ast.File, current *ast.File) bool {
+ oldComments := extractMagicComments(original)
+ newComments := extractMagicComments(current)
if len(oldComments) != len(newComments) {
- return true, false
+ return true
}
for i := range oldComments {
if oldComments[i] != newComments[i] {
- return true, false
+ return true
}
}
+ return false
+}
- return false, false
+func isBadImportPath(path string) bool {
+ path, err := strconv.Unquote(path)
+ if err != nil {
+ return true
+ }
+ if path == "" {
+ return true
+ }
+ if path[len(path)-1] == '/' {
+ return true
+ }
+ return false
}
var buildConstraintOrEmbedRe = regexp.MustCompile(`^//(go:embed|go:build|\s*\+build).*`)
