gopls/internal/lsp/cache: fix race in adhoc reloading
To avoid inconsistent state where we load command-line-arguments
packages for files that would be contained in an ad-hoc package, ensure
that the view is loaded before doing file loads, when in ad-hoc mode.
Along the way, introduce the concept of 'ViewType' discussed in our
zero-config-gopls design (golang/go#57979).
Furthermore, move certain data onto the immutable workspaceInformation
type:
- moduleMode depends only on ViewType
- inGOPATH can be precomputed
Updates golang/go#57979
Fixes golang/go#57209
Change-Id: If54cea65fbc72e6e704eccc6fe59d30ae5d01069
Reviewed-on: https://go-review.googlesource.com/c/tools/+/495256
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
diff --git a/gopls/internal/lsp/cache/check.go b/gopls/internal/lsp/cache/check.go
index 83ea177..cf212c6 100644
--- a/gopls/internal/lsp/cache/check.go
+++ b/gopls/internal/lsp/cache/check.go
@@ -1190,7 +1190,7 @@
relatedInformation: s.view.Options().RelatedInformationSupported,
linkTarget: s.view.Options().LinkTarget,
- moduleMode: s.moduleMode(),
+ moduleMode: s.view.moduleMode(),
}, nil
}
diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go
index 7988a72..64e1b55 100644
--- a/gopls/internal/lsp/cache/snapshot.go
+++ b/gopls/internal/lsp/cache/snapshot.go
@@ -327,47 +327,19 @@
if s.view.hasGopackagesDriver {
return true
}
+
// Check if the user is working within a module or if we have found
// multiple modules in the workspace.
if len(s.workspaceModFiles) > 0 {
return true
}
- // The user may have a multiple directories in their GOPATH.
- // Check if the workspace is within any of them.
+
// TODO(rfindley): this should probably be subject to "if GO111MODULES = off {...}".
- for _, gp := range filepath.SplitList(s.view.gopath) {
- if source.InDir(filepath.Join(gp, "src"), s.view.folder.Filename()) {
- return true
- }
- }
- return false
-}
-
-// moduleMode reports whether the current snapshot uses Go modules.
-//
-// From https://go.dev/ref/mod, module mode is active if either of the
-// following hold:
-// - GO111MODULE=on
-// - GO111MODULE=auto and we are inside a module or have a GOWORK value.
-//
-// Additionally, this method returns false if GOPACKAGESDRIVER is set.
-//
-// TODO(rfindley): use this more widely.
-func (s *snapshot) moduleMode() bool {
- // Since we only really understand the `go` command, if the user has a
- // different GOPACKAGESDRIVER, assume that their configuration is valid.
- if s.view.hasGopackagesDriver {
- return false
- }
-
- switch s.view.effectiveGO111MODULE() {
- case on:
+ if s.view.inGOPATH {
return true
- case off:
- return false
- default:
- return len(s.workspaceModFiles) > 0 || s.view.gowork != ""
}
+
+ return false
}
// workspaceMode describes the way in which the snapshot's workspace should
@@ -763,6 +735,18 @@
}
func (s *snapshot) MetadataForFile(ctx context.Context, uri span.URI) ([]*source.Metadata, error) {
+ if s.view.ViewType() == AdHocView {
+ // As described in golang/go#57209, in ad-hoc workspaces (where we load ./
+ // rather than ./...), preempting the directory load with file loads can
+ // lead to an inconsistent outcome, where certain files are loaded with
+ // command-line-arguments packages and others are loaded only in the ad-hoc
+ // package. Therefore, ensure that the workspace is loaded before doing any
+ // file loads.
+ if err := s.awaitLoaded(ctx); err != nil {
+ return nil, err
+ }
+ }
+
s.mu.Lock()
// Start with the set of package associations derived from the last load.
diff --git a/gopls/internal/lsp/cache/view.go b/gopls/internal/lsp/cache/view.go
index d999170..1dc13aa 100644
--- a/gopls/internal/lsp/cache/view.go
+++ b/gopls/internal/lsp/cache/view.go
@@ -130,6 +130,10 @@
// GOPACKAGESDRIVER environment variable or a gopackagesdriver binary on
// their machine.
hasGopackagesDriver bool
+
+ // inGOPATH reports whether the workspace directory is contained in a GOPATH
+ // directory.
+ inGOPATH bool
}
// effectiveGO111MODULE reports the value of GO111MODULE effective in the go
@@ -145,6 +149,79 @@
}
}
+// A ViewType describes how we load package information for a view.
+//
+// This is used for constructing the go/packages.Load query, and for
+// interpreting missing packages, imports, or errors.
+//
+// Each view has a ViewType which is derived from its immutable workspace
+// information -- any environment change that would affect the view type
+// results in a new view.
+type ViewType int
+
+const (
+ // GoPackagesDriverView is a view with a non-empty GOPACKAGESDRIVER
+ // environment variable.
+ GoPackagesDriverView ViewType = iota
+
+ // GOPATHView is a view in GOPATH mode.
+ //
+ // I.e. in GOPATH, with GO111MODULE=off, or GO111MODULE=auto with no
+ // go.mod file.
+ GOPATHView
+
+ // GoModuleView is a view in module mode with a single Go module.
+ GoModuleView
+
+ // GoWorkView is a view in module mode with a go.work file.
+ GoWorkView
+
+ // An AdHocView is a collection of files in a given directory, not in GOPATH
+ // or a module.
+ AdHocView
+)
+
+// ViewType derives the type of the view from its workspace information.
+//
+// TODO(rfindley): this logic is overlapping and slightly inconsistent with
+// validBuildConfiguration. As part of zero-config-gopls (golang/go#57979), fix
+// this inconsistency and consolidate on the ViewType abstraction.
+func (w workspaceInformation) ViewType() ViewType {
+ if w.hasGopackagesDriver {
+ return GoPackagesDriverView
+ }
+ go111module := w.effectiveGO111MODULE()
+ if w.gowork != "" && go111module != off {
+ return GoWorkView
+ }
+ if w.gomod != "" && go111module != off {
+ return GoModuleView
+ }
+ if w.inGOPATH && go111module != on {
+ return GOPATHView
+ }
+ return AdHocView
+}
+
+// moduleMode reports whether the current snapshot uses Go modules.
+//
+// From https://go.dev/ref/mod, module mode is active if either of the
+// following hold:
+// - GO111MODULE=on
+// - GO111MODULE=auto and we are inside a module or have a GOWORK value.
+//
+// Additionally, this method returns false if GOPACKAGESDRIVER is set.
+//
+// TODO(rfindley): use this more widely.
+func (w workspaceInformation) moduleMode() bool {
+ switch w.ViewType() {
+ case GoModuleView, GoWorkView:
+ return true
+ default:
+ return false
+ }
+}
+
// GOWORK returns the effective GOWORK value for this workspace, if
// any, in URI form.
//
@@ -740,6 +817,8 @@
})
}
+ // TODO(rfindley): this should be predicated on the s.view.moduleMode().
+ // There is no point loading ./... if we have an empty go.work.
if len(s.workspaceModFiles) > 0 {
for modURI := range s.workspaceModFiles {
// Verify that the modfile is valid before trying to load it.
@@ -881,7 +960,7 @@
if err != nil {
return info, err
}
- if err := info.goEnv.load(ctx, folder.Filename(), options.EnvSlice(), s.gocmdRunner); err != nil {
+ if err := info.load(ctx, folder.Filename(), options.EnvSlice(), s.gocmdRunner); err != nil {
return info, err
}
// The value of GOPACKAGESDRIVER is not returned through the go command.
@@ -899,6 +978,13 @@
return info, err
}
+ // Check if the workspace is within any GOPATH directory.
+ for _, gp := range filepath.SplitList(info.gopath) {
+ if source.InDir(filepath.Join(gp, "src"), folder.Filename()) {
+ info.inGOPATH = true
+ break
+ }
+ }
return info, nil
}
