gopls/internal/lsp/cache: invalidate metadata after vendor change
This change causes the snapshot to watch the vendor/modules.txt
file and to invalidate the metadata if it changes, since it
could cause modules to move.
Also, add a regression test for the bug in which running
go mod vendor causes a module to move, but the Definitions
operation reports the old location. (And the reverse case
of deleting the vendor tree.)
Also, add persistent.Map.String method, for debugging.
Fixes golang/go#55995
Change-Id: I48302416586c763b4a4de7d67aaa88fde52ea400
Reviewed-on: https://go-review.googlesource.com/c/tools/+/454315
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
diff --git a/gopls/internal/lsp/cache/session.go b/gopls/internal/lsp/cache/session.go
index 3b0958e..51c8bb7 100644
--- a/gopls/internal/lsp/cache/session.go
+++ b/gopls/internal/lsp/cache/session.go
@@ -581,7 +581,6 @@
// the directory.
func (s *Session) ExpandModificationsToDirectories(ctx context.Context, changes []source.FileModification) []source.FileModification {
var snapshots []*snapshot
-
s.viewMu.Lock()
for _, v := range s.views {
snapshot, release := v.getSnapshot()
diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go
index 5a15641..80de11a 100644
--- a/gopls/internal/lsp/cache/snapshot.go
+++ b/gopls/internal/lsp/cache/snapshot.go
@@ -917,7 +917,7 @@
dirName := dir.Filename()
// If the directory is within the view's folder, we're already watching
- // it with the pattern above.
+ // it with the first pattern above.
if source.InDir(s.view.folder.Filename(), dirName) {
continue
}
@@ -1654,16 +1654,18 @@
// Most likely, each call site of inVendor needs to be reconsidered to
// understand and correctly implement the desired behavior.
func inVendor(uri span.URI) bool {
- if !strings.Contains(string(uri), "/vendor/") {
- return false
- }
- // Only packages in _subdirectories_ of /vendor/ are considered vendored
+ _, after, found := cut(string(uri), "/vendor/")
+ // Only subdirectories of /vendor/ are considered vendored
// (/vendor/a/foo.go is vendored, /vendor/foo.go is not).
- split := strings.Split(string(uri), "/vendor/")
- if len(split) < 2 {
- return false
+ return found && strings.Contains(after, "/")
+}
+
+// TODO(adonovan): replace with strings.Cut when we can assume go1.18.
+func cut(s, sep string) (before, after string, found bool) {
+ if i := strings.Index(s, sep); i >= 0 {
+ return s[:i], s[i+len(sep):], true
}
- return strings.Contains(split[1], "/")
+ return s, "", false
}
// unappliedChanges is a file source that handles an uncloned snapshot.
@@ -1691,14 +1693,16 @@
s.mu.Lock()
defer s.mu.Unlock()
- // If there is an initialization error and a vendor directory changed, try to
- // reinit.
- if s.initializedErr != nil {
- for uri := range changes {
- if inVendor(uri) {
- reinit = true
- break
- }
+ // Changes to vendor tree may require reinitialization,
+ // either because of an initialization error
+ // (e.g. "inconsistent vendoring detected"), or because
+ // one or more modules may have moved into or out of the
+ // vendor tree after 'go mod vendor' or 'rm -fr vendor/'.
+ for uri := range changes {
+ if inVendor(uri) && s.initializedErr != nil ||
+ strings.HasSuffix(string(uri), "/vendor/modules.txt") {
+ reinit = true
+ break
}
}
@@ -1782,7 +1786,7 @@
}
// directIDs keeps track of package IDs that have directly changed.
- // It maps id->invalidateMetadata.
+ // Note: this is not a set, it's a map from id to invalidateMetadata.
directIDs := map[PackageID]bool{}
// Invalidate all package metadata if the workspace module has changed.
@@ -1821,7 +1825,7 @@
// Mark all of the package IDs containing the given file.
filePackageIDs := invalidatedPackageIDs(uri, s.meta.ids, pkgFileChanged)
for id := range filePackageIDs {
- directIDs[id] = directIDs[id] || invalidateMetadata
+ directIDs[id] = directIDs[id] || invalidateMetadata // may insert 'false'
}
// Invalidate the previous modTidyHandle if any of the files have been
diff --git a/gopls/internal/lsp/cache/view.go b/gopls/internal/lsp/cache/view.go
index 54b4122..bcb6f3a 100644
--- a/gopls/internal/lsp/cache/view.go
+++ b/gopls/internal/lsp/cache/view.go
@@ -1128,28 +1128,34 @@
// FileHandle from the cache for temporary files is problematic, since we
// cannot delete it.
func (s *snapshot) vendorEnabled(ctx context.Context, modURI span.URI, modContent []byte) (bool, error) {
+ // Legacy GOPATH workspace?
if s.workspaceMode()&moduleMode == 0 {
return false, nil
}
+
+ // Explicit -mod flag?
matches := modFlagRegexp.FindStringSubmatch(s.view.goEnv["GOFLAGS"])
- var modFlag string
if len(matches) != 0 {
- modFlag = matches[1]
- }
- if modFlag != "" {
- // Don't override an explicit '-mod=vendor' argument.
- // We do want to override '-mod=readonly': it would break various module code lenses,
- // and on 1.16 we know -modfile is available, so we won't mess with go.mod anyway.
- return modFlag == "vendor", nil
+ modFlag := matches[1]
+ if modFlag != "" {
+ // Don't override an explicit '-mod=vendor' argument.
+ // We do want to override '-mod=readonly': it would break various module code lenses,
+ // and on 1.16 we know -modfile is available, so we won't mess with go.mod anyway.
+ return modFlag == "vendor", nil
+ }
}
modFile, err := modfile.Parse(modURI.Filename(), modContent, nil)
if err != nil {
return false, err
}
+
+ // No vendor directory?
if fi, err := os.Stat(filepath.Join(s.view.rootURI.Filename(), "vendor")); err != nil || !fi.IsDir() {
return false, nil
}
+
+ // Vendoring enabled by default by go declaration in go.mod?
vendorEnabled := modFile.Go != nil && modFile.Go.Version != "" && semver.Compare("v"+modFile.Go.Version, "v1.14") >= 0
return vendorEnabled, nil
}
diff --git a/gopls/internal/lsp/cache/view_test.go b/gopls/internal/lsp/cache/view_test.go
index 99daff1..f57fc80 100644
--- a/gopls/internal/lsp/cache/view_test.go
+++ b/gopls/internal/lsp/cache/view_test.go
@@ -111,18 +111,11 @@
path string
inVendor bool
}{
- {
- path: "foo/vendor/x.go",
- inVendor: false,
- },
- {
- path: "foo/vendor/x/x.go",
- inVendor: true,
- },
- {
- path: "foo/x.go",
- inVendor: false,
- },
+ {"foo/vendor/x.go", false},
+ {"foo/vendor/x/x.go", true},
+ {"foo/x.go", false},
+ {"foo/vendor/foo.txt", false},
+ {"foo/vendor/modules.txt", false},
} {
if got := inVendor(span.URIFromPath(tt.path)); got != tt.inVendor {
t.Errorf("expected %s inVendor %v, got %v", tt.path, tt.inVendor, got)
