gopls/internal/cache: pass overlays to go mod commands
Before, gopls did not use -overlay with go mod commands,
even though gopls always wrote the overlay file, so go mod tidy
would read Go files from disk without respecting unsaved edits.
This led to races with file save events, causing some tests
to flake. (This regression occurred in CL 730220.)
Because go mod tidy now sees unsaved buffers, its cached results no
longer need to be invalidated when a file is saved; it suffices to
invalidate only when there's a change to package metadata or a
go.{mod,sum} file.
Also, invalidating a tidy result now causes the view to be
re-diagnosed. Previously this happened only by accident, when the
changed go.mod file shared a directory with the Go files of some
package.
This CL passes -overlay with all go mod commands, and adds
regression tests that deterministically reproduced the
problem in the flaky test.
Fixes golang/go#82027
Fixes golang/go#53881
Change-Id: I09699f60f68a3836199833d85243f36a1b30e48e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/847425
LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Peter Weinberger <pjw@google.com>
diff --git a/gopls/internal/cache/snapshot.go b/gopls/internal/cache/snapshot.go
index 4ec7ca7..f781923 100644
--- a/gopls/internal/cache/snapshot.go
+++ b/gopls/internal/cache/snapshot.go
@@ -1678,10 +1678,13 @@
_, oldOpen := oldFH.(*overlay)
_, newOpen := newFH.(*overlay)
+ oldExists := oldFH != nil && fileExists(oldFH)
+ newExists := fileExists(newFH)
+
// TODO(rfindley): consolidate with 'metadataChanges' logic below, which
// also considers existential changes.
- anyFileOpenedOrClosed = anyFileOpenedOrClosed || (oldOpen != newOpen)
- anyPkgFileChanged = anyPkgFileChanged || (oldFH == nil || !fileExists(oldFH)) && fileExists(newFH)
+ anyFileOpenedOrClosed = anyFileOpenedOrClosed || oldOpen != newOpen
+ anyPkgFileChanged = anyPkgFileChanged || !oldExists && newExists
// If uri is a Go file, check if it has changed in a way that would
// invalidate metadata. Note that we can't use s.view.FileKind here,
@@ -1707,14 +1710,23 @@
directIDs[id] = directIDs[id] || invalidateMetadata // may insert 'false'
}
- // Invalidate the previous modTidyHandle if any of the files have been
- // saved or if any of the metadata has been invalidated.
+ // contentChanged reports whether the content of
+ // the file changed, including by creation or deletion.
+ contentChanged := func() bool {
+ return oldExists != newExists || oldExists && oldFH.Identity() != newFH.Identity()
+ }
+
+ // Invalidate the previous modTidyHandle if any of the metadata has
+ // been invalidated, or if the content of a go.mod or go.sum file
+ // (such as that of a replaced module, or a new nested module) has
+ // changed. Other non-Go files (such as .s files) cannot affect
+ // the results.
//
- // TODO(rfindley): this seems like too-aggressive invalidation of mod
- // results. We should instead thread through overlays to the Go command
- // invocation and only run this if invalidateMetadata (and perhaps then
- // still do it less frequently).
- if invalidateMetadata || fileWasSaved(oldFH, newFH) {
+ // The go mod tidy and go mod why commands are passed the snapshot's
+ // unsaved buffers using -overlay (see GoCommandInvocation), so their
+ // results depend only on the snapshot's file contents, not on
+ // whether those contents have been saved.
+ if invalidateMetadata || (isGoMod(uri) || uri.Base() == "go.sum") && contentChanged() {
// Only invalidate mod tidy results for the most relevant modfile in the
// workspace. This is a potentially lossy optimization for workspaces
// with many modules (such as google-cloud-go, which has 145 modules as
@@ -1727,18 +1739,29 @@
// ignores GOWORK, so the two modules would have to be related by a chain
// of replace directives.
//
- // We could improve accuracy by inspecting replace directives, using
- // overlays in go mod tidy, and/or checking for metadata changes from the
- // on-disk content.
+ // We could improve accuracy by inspecting replace directives.
//
// Note that we iterate the modTidyHandles map here, rather than e.g.
// using nearestModFile, because we don't have access to an accurate
// FileSource at this point in the snapshot clone.
+ //
+ // Invalidating a mod tidy result requires that the view be
+ // re-diagnosed, even if no package was invalidated: for example,
+ // a change to the go.mod file of a replaced module whose
+ // directory contains no Go files (go.dev/cl/847425).
+ //
+ // TODO(adonovan): this heuristic is incomplete and won't catch
+ // a change to a module replaced from outside the main module.
+ // A content-addressed cache of go.{mod,sum} files and the
+ // workspace import graph would make invalidation precise.
const onlyInvalidateMostRelevant = true
if onlyInvalidateMostRelevant {
- deleteMostRelevantModFile(result.modTidyHandles, uri)
+ if deleteMostRelevantModFile(result.modTidyHandles, uri) {
+ needsDiagnosis = true
+ }
} else {
result.modTidyHandles.Clear()
+ needsDiagnosis = true
}
// TODO(rfindley): should we apply the above heuristic to mod vuln or mod
@@ -1914,7 +1937,9 @@
// changed that happens not to be present in the map, but that's OK: the goal
// of this function is to guarantee that IF the nearest mod file is present in
// the map, it is invalidated.
-func deleteMostRelevantModFile(m *persistent.Map[protocol.DocumentURI, *memoize.Promise], changed protocol.DocumentURI) {
+//
+// It reports whether an entry was deleted.
+func deleteMostRelevantModFile(m *persistent.Map[protocol.DocumentURI, *memoize.Promise], changed protocol.DocumentURI) bool {
var mostRelevant protocol.DocumentURI
changedFile := changed.Path()
@@ -1925,9 +1950,7 @@
}
}
}
- if mostRelevant != "" {
- m.Delete(mostRelevant)
- }
+ return mostRelevant != "" && m.Delete(mostRelevant)
}
// invalidatedPackageIDs returns all packages invalidated by a change to uri.
@@ -1992,32 +2015,6 @@
return invalidated
}
-// fileWasSaved reports whether the FileHandle passed in has been saved. It
-// accomplishes this by checking to see if the original and current FileHandles
-// are both overlays, and if the current FileHandle is saved while the original
-// FileHandle was not saved.
-func fileWasSaved(originalFH, currentFH file.Handle) bool {
- if originalFH == nil || currentFH == nil {
- return true // should not happen for valid file handles
- }
-
- // If the file identity has not changed, the content has not changed.
- // Therefore, the "saved" state (from the perspective of go mod tidy)
- // has not changed.
- if originalFH.Identity() == currentFH.Identity() {
- return false
- }
- c, ok := currentFH.(*overlay)
- if !ok || c == nil {
- return true
- }
- o, ok := originalFH.(*overlay)
- if !ok || o == nil {
- return c.saved
- }
- return !o.saved && c.saved
-}
-
// metadataChanges detects features of the change from oldFH->newFH that may
// affect package metadata.
//
diff --git a/gopls/internal/test/integration/diagnostics/diagnostics_test.go b/gopls/internal/test/integration/diagnostics/diagnostics_test.go
index 9022abc..0067d06 100644
--- a/gopls/internal/test/integration/diagnostics/diagnostics_test.go
+++ b/gopls/internal/test/integration/diagnostics/diagnostics_test.go
@@ -784,6 +784,152 @@
})
}
+// Test for go.dev/issue/82027: go mod tidy diagnostics reflect unsaved
+// edits, and are unaffected by saving.
+//
+// Previously, go mod tidy read Go files from disk, so its results
+// depended on whether (and when) buffers were saved.
+func TestModTidyUnsavedEdits_Issue82027(t *testing.T) {
+ t.Parallel()
+ const files = `
+-- go.mod --
+module mod.com
+
+go 1.14
+-- main.go --
+package main
+
+import "github.com/ardanlabs/conf"
+
+func main() {
+ _ = conf.ErrHelpWanted
+}
+`
+ WithOptions(
+ ProxyFiles(ardanLabsProxy),
+ CacheFiles(ardanLabsProxy),
+ ).Run(t, files, func(t *testing.T, env *Env) {
+ // Add the requirement (and go.sum) using the "go get" quick fix.
+ env.OpenFile("go.mod")
+ env.OpenFile("main.go")
+ var d protocol.PublishDiagnosticsParams
+ env.AfterChange(
+ Diagnostics(
+ env.AtRegexp("main.go", `"github.com/ardanlabs/conf"`),
+ WithMessage("no required module provides package")),
+ ReadDiagnostics("main.go", &d),
+ )
+ env.ApplyQuickFixes("main.go", d.Diagnostics)
+ env.SaveBuffer("go.mod")
+ env.AfterChange(
+ NoDiagnostics(ForFile("main.go")),
+ NoDiagnostics(ForFile("go.mod")),
+ )
+
+ // Remove the import and its use in the buffer only.
+ // The requirement is reported as unused, even though
+ // the on-disk main.go still imports conf.
+ env.RegexpReplace("main.go", `import "github.com/ardanlabs/conf"`, "")
+ env.RegexpReplace("main.go", `_ = conf.ErrHelpWanted`, "")
+ unused := Diagnostics(
+ env.AtRegexp("go.mod", "require github.com/ardanlabs/conf"),
+ WithMessage("not used in this module"))
+ env.AfterChange(
+ NoDiagnostics(ForFile("main.go")),
+ unused,
+ )
+
+ // Saving the buffer doesn't change anything.
+ env.SaveBufferWithoutActions("main.go")
+ env.AfterChange(unused)
+
+ // Restore the import and its use in the buffer only.
+ // The requirement is no longer reported as unused, even
+ // though the on-disk main.go doesn't import conf.
+ env.RegexpReplace("main.go", `package main`, `package main; import "github.com/ardanlabs/conf"`)
+ env.RegexpReplace("main.go", `func main\(\) {`, "func main() { _ = conf.ErrHelpWanted")
+ env.AfterChange(
+ NoDiagnostics(ForFile("main.go")),
+ NoDiagnostics(ForFile("go.mod")),
+ )
+ })
+}
+
+// Test that go mod tidy diagnostics reflect unsaved edits to the go.mod
+// file of a replaced module (go.dev/issue/82027, go.dev/issue/53881).
+//
+// The replaced module's package is in subdirectory a/sub so that no Go
+// file shares a directory with a/go.mod. Otherwise the same-directory
+// heuristic of invalidatedPackageIDs would invalidate package example.com/a
+// and trigger re-diagnosis even if invalidating the tidy result did not.
+func TestModTidyUnsavedReplacedGoMod(t *testing.T) {
+ t.Parallel()
+ const files = `
+-- go.mod --
+module mod.com
+
+go 1.14
+
+require (
+ example.com/a v0.0.0
+ example.com/b v0.0.0
+)
+
+replace (
+ example.com/a => ./a
+ example.com/b => ./b
+)
+
+-- main.go --
+package main
+
+import _ "example.com/a/sub"
+
+func main() {}
+
+-- a/go.mod --
+module example.com/a
+
+go 1.14
+
+require example.com/b v0.0.0
+
+-- a/sub/a.go --
+package sub
+
+import _ "example.com/b"
+
+-- b/go.mod --
+module example.com/b
+
+go 1.14
+
+-- b/b.go --
+package b
+`
+ Run(t, files, func(t *testing.T, env *Env) {
+ // Module a requires b, so the main module's
+ // requirement on b is redundant.
+ env.OpenFile("go.mod")
+ unusedB := Diagnostics(
+ env.AtRegexp("go.mod", `example.com/b v0.0.0`),
+ WithMessage("not used in this module"))
+ env.AfterChange(unusedB)
+
+ // Remove a's requirement on b, in the buffer only.
+ // Now the main module must require b.
+ env.OpenFile("a/go.mod")
+ env.RegexpReplace("a/go.mod", `require example.com/b v0.0.0`, "")
+ env.AfterChange(
+ NoDiagnostics(ForFile("go.mod"), WithMessage("not used in this module")),
+ )
+
+ // Restore it, again in the buffer only.
+ env.RegexpReplace("a/go.mod", `go 1.14`, "go 1.14\n\nrequire example.com/b v0.0.0")
+ env.AfterChange(unusedB)
+ })
+}
+
// Test for golang/go#38207.
func TestNewModule_Issue38207(t *testing.T) {
t.Parallel()
diff --git a/internal/gocommand/invoke.go b/internal/gocommand/invoke.go
index 45ccf27..ec4e713 100644
--- a/internal/gocommand/invoke.go
+++ b/internal/gocommand/invoke.go
@@ -232,6 +232,7 @@
// mod needs the sub-verb before flags.
goArgs = append(goArgs, i.Args[0])
appendModFile()
+ appendOverlayFlag() // all 'go mod' subcommands accept -overlay
goArgs = append(goArgs, i.Args[1:]...)
case "get":
goArgs = append(goArgs, i.BuildFlags...)
