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 <[email protected]>
gopls-CI: kokoro <[email protected]>
Run-TryBot: Robert Findley <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>

Author: findleyr

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 {

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
 	}

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

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
 }