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)
diff --git a/gopls/internal/regtest/misc/definition_test.go b/gopls/internal/regtest/misc/definition_test.go
index a5030d7..8f8619e 100644
--- a/gopls/internal/regtest/misc/definition_test.go
+++ b/gopls/internal/regtest/misc/definition_test.go
@@ -5,7 +5,9 @@
package misc
import (
+ "os"
"path"
+ "path/filepath"
"strings"
"testing"
@@ -286,3 +288,87 @@
env.Editor.Server.Definition(env.Ctx, params)
})
}
+
+// TestVendoringInvalidatesMetadata ensures that gopls uses the
+// correct metadata even after an external 'go mod vendor' command
+// causes packages to move; see issue #55995.
+func TestVendoringInvalidatesMetadata(t *testing.T) {
+ const proxy = `
+-- other.com/b@v1.0.0/go.mod --
+module other.com/b
+go 1.14
+
+-- other.com/b@v1.0.0/b.go --
+package b
+const K = 0
+`
+ const src = `
+-- go.mod --
+module example.com/a
+go 1.14
+require other.com/b v1.0.0
+
+-- go.sum --
+other.com/b v1.0.0 h1:1wb3PMGdet5ojzrKl+0iNksRLnOM9Jw+7amBNqmYwqk=
+other.com/b v1.0.0/go.mod h1:TgHQFucl04oGT+vrUm/liAzukYHNxCwKNkQZEyn3m9g=
+
+-- a.go --
+package a
+import "other.com/b"
+const _ = b.K
+
+`
+ WithOptions(
+ ProxyFiles(proxy),
+ Modes(Default), // fails in 'experimental' mode
+ ).Run(t, src, func(t *testing.T, env *Env) {
+ // Enable to debug go.sum mismatch, which may appear as
+ // "module lookup disabled by GOPROXY=off", confusingly.
+ if false {
+ env.DumpGoSum(".")
+ }
+
+ env.OpenFile("a.go")
+ refPos := env.RegexpSearch("a.go", "K") // find "b.K" reference
+
+ // Initially, b.K is defined in the module cache.
+ gotFile, _ := env.GoToDefinition("a.go", refPos)
+ wantCache := filepath.ToSlash(env.Sandbox.GOPATH()) + "/pkg/mod/other.com/b@v1.0.0/b.go"
+ if gotFile != wantCache {
+ t.Errorf("GoToDefinition, before: got file %q, want %q", gotFile, wantCache)
+ }
+
+ // Run 'go mod vendor' outside the editor.
+ if err := env.Sandbox.RunGoCommand(env.Ctx, ".", "mod", []string{"vendor"}, true); err != nil {
+ t.Fatalf("go mod vendor: %v", err)
+ }
+
+ // Synchronize changes to watched files.
+ env.Await(env.DoneWithChangeWatchedFiles())
+
+ // Now, b.K is defined in the vendor tree.
+ gotFile, _ = env.GoToDefinition("a.go", refPos)
+ wantVendor := "vendor/other.com/b/b.go"
+ if gotFile != wantVendor {
+ t.Errorf("GoToDefinition, after go mod vendor: got file %q, want %q", gotFile, wantVendor)
+ }
+
+ // Delete the vendor tree.
+ if err := os.RemoveAll(env.Sandbox.Workdir.AbsPath("vendor")); err != nil {
+ t.Fatal(err)
+ }
+ // Notify the server of the deletion.
+ if err := env.Sandbox.Workdir.CheckForFileChanges(env.Ctx); err != nil {
+ t.Fatal(err)
+ }
+
+ // Synchronize again.
+ env.Await(env.DoneWithChangeWatchedFiles())
+
+ // b.K is once again defined in the module cache.
+ gotFile, _ = env.GoToDefinition("a.go", refPos)
+ if gotFile != wantCache {
+ t.Errorf("GoToDefinition, after rm -rf vendor: got file %q, want %q", gotFile, wantCache)
+ }
+ })
+}
diff --git a/internal/persistent/map.go b/internal/persistent/map.go
index f5dd102..b29cfe4 100644
--- a/internal/persistent/map.go
+++ b/internal/persistent/map.go
@@ -8,7 +8,9 @@
package persistent
import (
+ "fmt"
"math/rand"
+ "strings"
"sync/atomic"
)
@@ -41,6 +43,18 @@
root *mapNode
}
+func (m *Map) String() string {
+ var buf strings.Builder
+ buf.WriteByte('{')
+ var sep string
+ m.Range(func(k, v interface{}) {
+ fmt.Fprintf(&buf, "%s%v: %v", sep, k, v)
+ sep = ", "
+ })
+ buf.WriteByte('}')
+ return buf.String()
+}
+
type mapNode struct {
key interface{}
value *refValue
