From c7ed4b3c0e55eb0b33d8bd823fe463483d0967f7 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Wed, 16 Nov 2022 13:52:25 -0500 Subject: [PATCH] gopls/internal/lsp/cache: clean up tracking of GO111MODULE Gopls views stored at least 4 copies of GO111MODULE: - the value in view.environmentVariables - the value in view.goEnv - the value in view.userGo111Module - the value in view.effectiveGo111Module All of these values may differ from eachother, depending on the user's environment and go version, and their meaning is not clearly documented. Try to clean this up, by having environmentVariables track precisely the variables output by `go env`, and providing a method to implement the derived logic of userGo111Module and effectiveGo111Module. Ignore view.goEnv for now, but leave a TODO. Confusingly, the name 'effectiveGO111MODULE' turned out to be a more appropriate name for what was formerly 'userGo111Module', so the naming has switched. This change is intended to be a no-op cleanup. Change-Id: I529cc005c1875915483ef119a465bf17a96dec3c Reviewed-on: https://go-review.googlesource.com/c/tools/+/451355 TryBot-Result: Gopher Robot gopls-CI: kokoro Run-TryBot: Robert Findley Reviewed-by: Alan Donovan --- gopls/internal/lsp/cache/load.go | 2 +- gopls/internal/lsp/cache/session.go | 2 +- gopls/internal/lsp/cache/snapshot.go | 2 +- gopls/internal/lsp/cache/view.go | 87 +++++++++++++++------------- 4 files changed, 51 insertions(+), 42 deletions(-) diff --git a/gopls/internal/lsp/cache/load.go b/gopls/internal/lsp/cache/load.go index 0498264324..46e2e891ac 100644 --- a/gopls/internal/lsp/cache/load.go +++ b/gopls/internal/lsp/cache/load.go @@ -307,7 +307,7 @@ func (s *snapshot) workspaceLayoutError(ctx context.Context) *source.CriticalErr } // TODO(rfindley): both of the checks below should be delegated to the workspace. - if s.view.userGo111Module == off { + if s.view.effectiveGO111MODULE() == off { return nil } if s.workspace.moduleSource != legacyWorkspace { diff --git a/gopls/internal/lsp/cache/session.go b/gopls/internal/lsp/cache/session.go index fe88a46fc9..9b4355331a 100644 --- a/gopls/internal/lsp/cache/session.go +++ b/gopls/internal/lsp/cache/session.go @@ -219,7 +219,7 @@ func (s *Session) createView(ctx context.Context, name string, folder span.URI, goworkURI := span.URIFromPath(explicitGowork) // Build the gopls workspace, collecting active modules in the view. - workspace, err := newWorkspace(ctx, root, goworkURI, 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.effectiveGO111MODULE() == off, options.ExperimentalWorkspaceModule) if err != nil { return nil, nil, func() {}, err } diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go index 11288e5c75..634afdff71 100644 --- a/gopls/internal/lsp/cache/snapshot.go +++ b/gopls/internal/lsp/cache/snapshot.go @@ -453,7 +453,7 @@ func (s *snapshot) goCommandInvocation(ctx context.Context, flags source.Invocat // 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.Env = append(append(append(os.Environ(), s.view.options.EnvSlice()...), inv.Env...), "GO111MODULE="+s.view.GO111MODULE()) inv.BuildFlags = append([]string{}, s.view.options.BuildFlags...) s.view.optionsMu.Unlock() cleanup = func() {} // fallback diff --git a/gopls/internal/lsp/cache/view.go b/gopls/internal/lsp/cache/view.go index 5974fab726..f373c00508 100644 --- a/gopls/internal/lsp/cache/view.go +++ b/gopls/internal/lsp/cache/view.go @@ -125,22 +125,48 @@ type workspaceInformation struct { hasGopackagesDriver bool // `go env` variables that need to be tracked by gopls. - environmentVariables - - // userGo111Module is the user's value of GO111MODULE. // - // TODO(rfindley): is there really value in memoizing this variable? It seems - // simpler to make this a function/method. - userGo111Module go111module - - // The value of GO111MODULE we want to run with. - effectiveGo111Module string + // TODO(rfindley): eliminate this in favor of goEnv, or vice-versa. + environmentVariables // goEnv is the `go env` output collected when a view is created. // It includes the values of the environment variables above. goEnv map[string]string } +// effectiveGO111MODULE reports the value of GO111MODULE effective in the go +// command at this go version, accounting for default values at different go +// versions. +func (w workspaceInformation) effectiveGO111MODULE() go111module { + // Off by default until Go 1.12. + go111module := w.GO111MODULE() + if go111module == "off" || (w.goversion < 12 && go111module == "") { + return off + } + // On by default as of Go 1.16. + if go111module == "on" || (w.goversion >= 16 && go111module == "") { + return on + } + return auto +} + +// GO111MODULE returns the value of GO111MODULE to use for running the go +// command. It differs from the user's environment in order to allow for the +// more forgiving default value "auto" when using recent go versions. +// +// TODO(rfindley): it is probably not worthwhile diverging from the go command +// here. The extra forgiveness may be nice, but breaks the invariant that +// running the go command from the command line produces the same build list. +// +// Put differently: we shouldn't go out of our way to make GOPATH work, when +// the go command does not. +func (w workspaceInformation) GO111MODULE() string { + if w.goversion >= 16 && w.go111module == "" { + return "auto" + } + return w.go111module +} + type go111module int const ( @@ -149,8 +175,15 @@ const ( on ) +// environmentVariables holds important environment variables captured by a +// call to `go env`. type environmentVariables struct { - gocache, gopath, goroot, goprivate, gomodcache, go111module string + gocache, gopath, goroot, goprivate, gomodcache string + + // Don't use go111module directly, because we choose to use a different + // default (auto) on Go 1.16 and later, to avoid spurious errors. Use + // the workspaceInformation.GO111MODULE method instead. + go111module string } // workspaceMode holds various flags defining how the gopls workspace should @@ -804,20 +837,11 @@ func (s *Session) getWorkspaceInformation(ctx context.Context, folder span.URI, return nil, err } - go111module := os.Getenv("GO111MODULE") - if v, ok := options.Env["GO111MODULE"]; ok { - go111module = v - } // Make sure to get the `go env` before continuing with initialization. - envVars, env, err := s.getGoEnv(ctx, folder.Filename(), goversion, go111module, options.EnvSlice()) + envVars, env, err := s.getGoEnv(ctx, folder.Filename(), goversion, options.EnvSlice()) if err != nil { return nil, err } - // If using 1.16, change the default back to auto. The primary effect of - // GO111MODULE=on is to break GOPATH, which we aren't too interested in. - if goversion >= 16 && go111module == "" { - go111module = "auto" - } // 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 @@ -838,26 +862,12 @@ func (s *Session) getWorkspaceInformation(ctx context.Context, folder span.URI, return &workspaceInformation{ hasGopackagesDriver: hasGopackagesDriver, - effectiveGo111Module: go111module, - userGo111Module: go111moduleForVersion(go111module, goversion), goversion: goversion, environmentVariables: envVars, goEnv: env, }, nil } -func go111moduleForVersion(go111module string, goversion int) go111module { - // Off by default until Go 1.12. - if go111module == "off" || (goversion < 12 && go111module == "") { - return off - } - // On by default as of Go 1.16. - if go111module == "on" || (goversion >= 16 && go111module == "") { - return on - } - return auto -} - // findWorkspaceRoot searches for the best workspace root according to the // following heuristics: // - First, look for a parent directory containing a gopls.mod file @@ -938,7 +948,7 @@ func defaultCheckPathCase(path string) error { } // getGoEnv gets the view's various GO* values. -func (s *Session) getGoEnv(ctx context.Context, folder string, goversion int, go111module string, configEnv []string) (environmentVariables, map[string]string, error) { +func (s *Session) getGoEnv(ctx context.Context, folder string, goversion int, configEnv []string) (environmentVariables, map[string]string, error) { envVars := environmentVariables{} vars := map[string]*string{ "GOCACHE": &envVars.gocache, @@ -984,13 +994,12 @@ func (s *Session) getGoEnv(ctx context.Context, folder string, goversion int, go } // Old versions of Go don't have GOMODCACHE, so emulate it. + // + // TODO(rfindley): consistent with the treatment of go111module, we should + // provide a wrapper method rather than mutating this value. if envVars.gomodcache == "" && envVars.gopath != "" { envVars.gomodcache = filepath.Join(filepath.SplitList(envVars.gopath)[0], "pkg/mod") } - // GO111MODULE does not appear in `go env` output until Go 1.13. - if goversion < 13 { - envVars.go111module = go111module - } return envVars, env, err }