diff --git a/gopls/internal/regtest/workspace/broken_test.go b/gopls/internal/regtest/workspace/broken_test.go index b1f35b38d3..fbc41de161 100644 --- a/gopls/internal/regtest/workspace/broken_test.go +++ b/gopls/internal/regtest/workspace/broken_test.go @@ -16,6 +16,10 @@ import ( // 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 @@ func TestBrokenWorkspace_DuplicateModules(t *testing.T) { 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 0000000000..1d95160eaa --- /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 984e8c1e75..d11c06d1e7 100644 --- a/internal/lsp/cache/session.go +++ b/internal/lsp/cache/session.go @@ -7,6 +7,7 @@ package cache import ( "context" "fmt" + "os" "strconv" "strings" "sync" @@ -199,8 +200,14 @@ func (s *Session) createView(ctx context.Context, name string, folder span.URI, } } + 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 @@ func (s *Session) createView(ctx context.Context, name string, folder span.URI, 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 da4fe6265a..6ed6fe5360 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -436,6 +436,12 @@ func (s *snapshot) goCommandInvocation(ctx context.Context, flags source.Invocat 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 1aca36e7d4..b23ed614f6 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -27,6 +27,7 @@ import ( "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 @@ type View struct { // 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 @@ func (v *View) relevantChange(c source.FileModification) bool { // 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 @@ func (s *Session) getWorkspaceInformation(ctx context.Context, folder span.URI, } // 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 9182cb9ad5..f04fbe8eec 100644 --- a/internal/lsp/cache/workspace.go +++ b/internal/lsp/cache/workspace.go @@ -46,6 +46,19 @@ func (s workspaceSource) String() string { } } +// 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 @@ func (s workspaceSource) String() string { // 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 @@ type workspace struct { // // 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 @@ func newWorkspace(ctx context.Context, root span.URI, fs source.FileSource, excl // 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 @@ func (w *workspace) Clone(ctx context.Context, changes map[span.URI]*fileChange, // 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 @@ func handleWorkspaceFileChanges(ctx context.Context, ws *workspace, changes map[ // 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 @@ func handleWorkspaceFileChanges(ctx context.Context, ws *workspace, changes map[ } // 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 871e4bb9b9..f1cd00b711 100644 --- a/internal/lsp/cache/workspace_test.go +++ b/internal/lsp/cache/workspace_test.go @@ -280,7 +280,7 @@ replace gopls.test => ../../gopls.test2`, false}, 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 @@ func workspaceFromTxtar(t *testing.T, files string) (*workspace, func(), error) 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 }