internal/lsp/cache: track explicit go.work files outside the workspace
In order to correctly process changes to the go.work file, the workspace
must know about GOWORK settings configured in the users environment.
Compute this when initializing the view, and thread this through to the
workspace.
At this point, workspace information is spread around in a few places.
Add some TODOs to clean this up.
Also remove some module data that was not used in
TestBrokenWorkspace_DuplicateModules.
Updates golang/go#53631
Change-Id: Ie0577d702c8a229304387bc7fe53a8befb544acb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/421500
Reviewed-by: Suzy Mueller <suzmue@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
diff --git a/gopls/internal/regtest/workspace/broken_test.go b/gopls/internal/regtest/workspace/broken_test.go
index b1f35b3..fbc41de 100644
--- a/gopls/internal/regtest/workspace/broken_test.go
+++ b/gopls/internal/regtest/workspace/broken_test.go
@@ -16,6 +16,10 @@
// This file holds various tests for UX with respect to broken workspaces.
//
// TODO: consolidate other tests here.
+//
+// TODO: write more tests:
+// - an explicit GOWORK value that doesn't exist
+// - using modules and/or GOWORK inside of GOPATH?
// Test for golang/go#53933
func TestBrokenWorkspace_DuplicateModules(t *testing.T) {
@@ -28,8 +32,6 @@
module example.com/foo
go 1.12
--- example.com/foo@v1.2.3/foo.go --
-package foo
`
const src = `
diff --git a/gopls/internal/regtest/workspace/fromenv_test.go b/gopls/internal/regtest/workspace/fromenv_test.go
new file mode 100644
index 0000000..1d95160
--- /dev/null
+++ b/gopls/internal/regtest/workspace/fromenv_test.go
@@ -0,0 +1,54 @@
+// Copyright 2022 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package workspace
+
+import (
+ "testing"
+
+ . "golang.org/x/tools/internal/lsp/regtest"
+)
+
+// Test that setting go.work via environment variables or settings works.
+func TestUseGoWorkOutsideTheWorkspace(t *testing.T) {
+ const files = `
+-- work/a/go.mod --
+module a.com
+
+go 1.12
+-- work/a/a.go --
+package a
+-- work/b/go.mod --
+module b.com
+
+go 1.12
+-- work/b/b.go --
+package b
+
+func _() {
+ x := 1 // unused
+}
+-- config/go.work --
+go 1.18
+
+use (
+ $SANDBOX_WORKDIR/work/a
+ $SANDBOX_WORKDIR/work/b
+)
+`
+
+ WithOptions(
+ EnvVars{"GOWORK": "$SANDBOX_WORKDIR/config/go.work"},
+ ).Run(t, files, func(t *testing.T, env *Env) {
+ // Even though work/b is not open, we should get its diagnostics as it is
+ // included in the workspace.
+ env.OpenFile("work/a/a.go")
+ env.Await(
+ OnceMet(
+ env.DoneWithOpen(),
+ env.DiagnosticAtRegexpWithMessage("work/b/b.go", "x := 1", "not used"),
+ ),
+ )
+ })
+}
diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go
index 984e8c1..d11c06d 100644
--- a/internal/lsp/cache/session.go
+++ b/internal/lsp/cache/session.go
@@ -7,6 +7,7 @@
import (
"context"
"fmt"
+ "os"
"strconv"
"strings"
"sync"
@@ -199,8 +200,14 @@
}
}
+ explicitGowork := os.Getenv("GOWORK")
+ if v, ok := options.Env["GOWORK"]; ok {
+ explicitGowork = v
+ }
+ goworkURI := span.URIFromPath(explicitGowork)
+
// Build the gopls workspace, collecting active modules in the view.
- workspace, err := newWorkspace(ctx, root, s, pathExcludedByFilterFunc(root.Filename(), wsInfo.gomodcache, options), wsInfo.userGo111Module == off, options.ExperimentalWorkspaceModule)
+ workspace, err := newWorkspace(ctx, root, goworkURI, s, pathExcludedByFilterFunc(root.Filename(), wsInfo.gomodcache, options), wsInfo.userGo111Module == off, options.ExperimentalWorkspaceModule)
if err != nil {
return nil, nil, func() {}, err
}
@@ -223,6 +230,7 @@
filesByURI: map[span.URI]*fileBase{},
filesByBase: map[string][]*fileBase{},
rootURI: root,
+ explicitGowork: goworkURI,
workspaceInformation: *wsInfo,
}
v.importsState = &importsState{
diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go
index da4fe62..6ed6fe5 100644
--- a/internal/lsp/cache/snapshot.go
+++ b/internal/lsp/cache/snapshot.go
@@ -436,6 +436,12 @@
s.view.optionsMu.Lock()
allowModfileModificationOption := s.view.options.AllowModfileModifications
allowNetworkOption := s.view.options.AllowImplicitNetworkAccess
+
+ // TODO(rfindley): this is very hard to follow, and may not even be doing the
+ // right thing: should inv.Env really trample view.options? Do we ever invoke
+ // this with a non-empty inv.Env?
+ //
+ // We should refactor to make it clearer that the correct env is being used.
inv.Env = append(append(append(os.Environ(), s.view.options.EnvSlice()...), inv.Env...), "GO111MODULE="+s.view.effectiveGo111Module)
inv.BuildFlags = append([]string{}, s.view.options.BuildFlags...)
s.view.optionsMu.Unlock()
diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go
index 1aca36e..b23ed61 100644
--- a/internal/lsp/cache/view.go
+++ b/internal/lsp/cache/view.go
@@ -27,6 +27,7 @@
"golang.org/x/tools/internal/event"
"golang.org/x/tools/internal/gocommand"
"golang.org/x/tools/internal/imports"
+ "golang.org/x/tools/internal/lsp/bug"
"golang.org/x/tools/internal/lsp/protocol"
"golang.org/x/tools/internal/lsp/source"
"golang.org/x/tools/internal/span"
@@ -98,6 +99,13 @@
// is just the folder. If we are in module mode, this is the module rootURI.
rootURI span.URI
+ // explicitGowork is, if non-empty, the URI for the explicit go.work file
+ // provided via the users environment.
+ //
+ // TODO(rfindley): this is duplicated in the workspace type. Refactor to
+ // eliminate this duplication.
+ explicitGowork span.URI
+
// workspaceInformation tracks various details about this view's
// environment variables, go version, and use of modules.
workspaceInformation
@@ -469,7 +477,7 @@
// TODO(rstambler): Make sure the go.work/gopls.mod files are always known
// to the view.
for _, src := range []workspaceSource{goWorkWorkspace, goplsModWorkspace} {
- if c.URI == uriForSource(v.rootURI, src) {
+ if c.URI == uriForSource(v.rootURI, v.explicitGowork, src) {
return true
}
}
@@ -813,9 +821,13 @@
}
// The value of GOPACKAGESDRIVER is not returned through the go command.
gopackagesdriver := os.Getenv("GOPACKAGESDRIVER")
+ // TODO(rfindley): this looks wrong, or at least overly defensive. If the
+ // value of GOPACKAGESDRIVER is not returned from the go command... why do we
+ // look it up here?
for _, s := range env {
split := strings.SplitN(s, "=", 2)
if split[0] == "GOPACKAGESDRIVER" {
+ bug.Reportf("found GOPACKAGESDRIVER from the go command") // see note above
gopackagesdriver = split[1]
}
}
diff --git a/internal/lsp/cache/workspace.go b/internal/lsp/cache/workspace.go
index 9182cb9..f04fbe8 100644
--- a/internal/lsp/cache/workspace.go
+++ b/internal/lsp/cache/workspace.go
@@ -46,6 +46,19 @@
}
}
+// workspaceCommon holds immutable information about the workspace setup.
+//
+// TODO(rfindley): there is some redundancy here with workspaceInformation.
+// Reconcile these two types.
+type workspaceCommon struct {
+ root span.URI
+ excludePath func(string) bool
+
+ // explicitGowork is, if non-empty, the URI for the explicit go.work file
+ // provided via the user's environment.
+ explicitGowork span.URI
+}
+
// workspace tracks go.mod files in the workspace, along with the
// gopls.mod file, to provide support for multi-module workspaces.
//
@@ -58,8 +71,8 @@
// This type is immutable (or rather, idempotent), so that it may be shared
// across multiple snapshots.
type workspace struct {
- root span.URI
- excludePath func(string) bool
+ workspaceCommon
+
moduleSource workspaceSource
// activeModFiles holds the active go.mod files.
@@ -98,17 +111,21 @@
//
// TODO(rfindley): newWorkspace should perhaps never fail, relying instead on
// the criticalError method to surface problems in the workspace.
-// TODO(rfindley): this function should accept the GOWORK value, if specified
-// by the user.
-func newWorkspace(ctx context.Context, root span.URI, fs source.FileSource, excludePath func(string) bool, go111moduleOff, useWsModule bool) (*workspace, error) {
+func newWorkspace(ctx context.Context, root, explicitGowork span.URI, fs source.FileSource, excludePath func(string) bool, go111moduleOff, useWsModule bool) (*workspace, error) {
ws := &workspace{
- root: root,
- excludePath: excludePath,
+ workspaceCommon: workspaceCommon{
+ root: root,
+ explicitGowork: explicitGowork,
+ excludePath: excludePath,
+ },
}
// The user may have a gopls.mod or go.work file that defines their
// workspace.
- if err := loadExplicitWorkspaceFile(ctx, ws, fs); err == nil {
+ //
+ // TODO(rfindley): if GO111MODULE=off, this looks wrong, though there are
+ // probably other problems.
+ if err := ws.loadExplicitWorkspaceFile(ctx, fs); err == nil {
return ws, nil
}
@@ -140,15 +157,15 @@
// loadExplicitWorkspaceFile loads workspace information from go.work or
// gopls.mod files, setting the active modules, mod file, and module source
// accordingly.
-func loadExplicitWorkspaceFile(ctx context.Context, ws *workspace, fs source.FileSource) error {
+func (ws *workspace) loadExplicitWorkspaceFile(ctx context.Context, fs source.FileSource) error {
for _, src := range []workspaceSource{goWorkWorkspace, goplsModWorkspace} {
- fh, err := fs.GetFile(ctx, uriForSource(ws.root, src))
+ fh, err := fs.GetFile(ctx, uriForSource(ws.root, ws.explicitGowork, src))
if err != nil {
return err
}
contents, err := fh.Read()
if err != nil {
- continue
+ continue // TODO(rfindley): is it correct to proceed here?
}
var file *modfile.File
var activeModFiles map[span.URI]struct{}
@@ -313,15 +330,14 @@
// Clone the workspace. This may be discarded if nothing changed.
changed := false
result := &workspace{
- root: w.root,
- moduleSource: w.moduleSource,
- knownModFiles: make(map[span.URI]struct{}),
- activeModFiles: make(map[span.URI]struct{}),
- workFile: w.workFile,
- mod: w.mod,
- sum: w.sum,
- wsDirs: w.wsDirs,
- excludePath: w.excludePath,
+ workspaceCommon: w.workspaceCommon,
+ moduleSource: w.moduleSource,
+ knownModFiles: make(map[span.URI]struct{}),
+ activeModFiles: make(map[span.URI]struct{}),
+ workFile: w.workFile,
+ mod: w.mod,
+ sum: w.sum,
+ wsDirs: w.wsDirs,
}
for k, v := range w.knownModFiles {
result.knownModFiles[k] = v
@@ -391,7 +407,7 @@
// exists or walk the filesystem if it has been deleted.
// go.work should override the gopls.mod if both exist.
for _, src := range []workspaceSource{goWorkWorkspace, goplsModWorkspace} {
- uri := uriForSource(ws.root, src)
+ uri := uriForSource(ws.root, ws.explicitGowork, src)
// File opens/closes are just no-ops.
change, ok := changes[uri]
if !ok {
@@ -460,12 +476,15 @@
}
// goplsModURI returns the URI for the gopls.mod file contained in root.
-func uriForSource(root span.URI, src workspaceSource) span.URI {
+func uriForSource(root, explicitGowork span.URI, src workspaceSource) span.URI {
var basename string
switch src {
case goplsModWorkspace:
basename = "gopls.mod"
case goWorkWorkspace:
+ if explicitGowork != "" {
+ return explicitGowork
+ }
basename = "go.work"
default:
return ""
diff --git a/internal/lsp/cache/workspace_test.go b/internal/lsp/cache/workspace_test.go
index 871e4bb..f1cd00b 100644
--- a/internal/lsp/cache/workspace_test.go
+++ b/internal/lsp/cache/workspace_test.go
@@ -280,7 +280,7 @@
fs := &osFileSource{}
excludeNothing := func(string) bool { return false }
- w, err := newWorkspace(ctx, root, fs, excludeNothing, false, !test.legacyMode)
+ w, err := newWorkspace(ctx, root, "", fs, excludeNothing, false, !test.legacyMode)
if err != nil {
t.Fatal(err)
}
@@ -325,7 +325,7 @@
fs := &osFileSource{}
excludeNothing := func(string) bool { return false }
- workspace, err := newWorkspace(ctx, root, fs, excludeNothing, false, false)
+ workspace, err := newWorkspace(ctx, root, "", fs, excludeNothing, false, false)
return workspace, cleanup, err
}
