all: rework goimports environment, support GOMODCACHE

This CL got away from me a little.

For a number of reasons, the existing goimports API of passing in values
for various GO* values was not working. For one, the number of necessary
variables kept growing. For another, we tried to avoid calling `go env`
in GOPATH mode by using `build.Default`, but that turns out to be buggy;
see golang/go#39838. And finally, it created massive confusion about
whether the values were intended to be read from the OS environment, or
fully evaluated by the `go` command.

There are only two users of the internal imports API, so there really
shouldn't need to be more than two modes. For the command line tool, we
have to call `go env` to deal with the `go/build` bug. So we just do it.
Tests use that same path, but can augment the enviroment to set
themselves up. In contrast, `gopls` needs to fully specify the
environment. It can simply pass in the fully evaluated GO* values.

Finally, make the change I was actually here to make: propagate
GOMODCACHE and use it where appropriate.

Fixes golang/go#39761.

Change-Id: I720c69839d91d66d98e94dfc5f065ba0279c5542
Reviewed-on: https://go-review.googlesource.com/c/tools/+/239754
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>

cmd/goimports/goimports.go

@@ -10,7 +10,6 @@ import (
 	"errors"
 	"flag"
 	"fmt"
-	"go/build"
 	"go/scanner"
 	"io"
 	"io/ioutil"
@@ -43,15 +42,6 @@ var (
 		TabIndent: true,
 		Comments:  true,
 		Fragment:  true,
-		// This environment, and its caches, will be reused for the whole run.
-		Env: &imports.ProcessEnv{
-			GOPATH:      build.Default.GOPATH,
-			GOROOT:      build.Default.GOROOT,
-			GOFLAGS:     os.Getenv("GOFLAGS"),
-			GO111MODULE: os.Getenv("GO111MODULE"),
-			GOPROXY:     os.Getenv("GOPROXY"),
-			GOSUMDB:     os.Getenv("GOSUMDB"),
-		},
 	}
 	exitCode = 0
 )

imports/forward.go

@@ -3,9 +3,7 @@
 package imports // import "golang.org/x/tools/imports"
 
 import (
-	"go/build"
 	"log"
-	"os"
 
 	intimp "golang.org/x/tools/internal/imports"
 )
@@ -42,12 +40,6 @@ func Process(filename string, src []byte, opt *Options) ([]byte, error) {
 	}
 	intopt := &intimp.Options{
 		Env: &intimp.ProcessEnv{
-			GOPATH:      build.Default.GOPATH,
-			GOROOT:      build.Default.GOROOT,
-			GOFLAGS:     os.Getenv("GOFLAGS"),
-			GO111MODULE: os.Getenv("GO111MODULE"),
-			GOPROXY:     os.Getenv("GOPROXY"),
-			GOSUMDB:     os.Getenv("GOSUMDB"),
 			LocalPrefix: LocalPrefix,
 		},
 		AllErrors:  opt.AllErrors,

internal/gocommand/invoke.go

@@ -185,7 +185,6 @@ func (i *Invocation) run(ctx context.Context, stdout, stderr io.Writer) error {
 		cmd.Env = append(cmd.Env, "PWD="+i.WorkingDir)
 		cmd.Dir = i.WorkingDir
 	}
-
 	defer func(start time.Time) { log("%s for %v", time.Since(start), cmdDebugStr(cmd)) }(time.Now())
 
 	return runCmdContext(ctx, cmd)

internal/imports/fix.go

@@ -7,6 +7,7 @@ package imports
 import (
 	"bytes"
 	"context"
+	"encoding/json"
 	"fmt"
 	"go/ast"
 	"go/build"
@@ -598,7 +599,7 @@ func getCandidatePkgs(ctx context.Context, wrappedCallback *scanCallback, filena
 	// Start off with the standard library.
 	for importPath, exports := range stdlib {
 		p := &pkg{
-			dir:             filepath.Join(env.GOROOT, "src", importPath),
+			dir:             filepath.Join(env.goroot(), "src", importPath),
 			importPathShort: importPath,
 			packageName:     path.Base(importPath),
 			relevance:       MaxRelevance,
@@ -743,6 +744,8 @@ func getPackageExports(ctx context.Context, wrapped func(PackageExport), searchP
 	return getCandidatePkgs(ctx, callback, filename, filePkg, env)
 }
 
+var RequiredGoEnvVars = []string{"GO111MODULE", "GOFLAGS", "GOINSECURE", "GOMOD", "GOMODCACHE", "GONOPROXY", "GONOSUMDB", "GOPATH", "GOPROXY", "GOROOT", "GOSUMDB"}
+
 // ProcessEnv contains environment variables and settings that affect the use of
 // the go command, the go/build package, etc.
 type ProcessEnv struct {
@@ -752,10 +755,13 @@ type ProcessEnv struct {
 
 	BuildFlags []string
 
-	// If non-empty, these will be used instead of the
-	// process-wide values.
-	GOPATH, GOROOT, GO111MODULE, GOPROXY, GOFLAGS, GOSUMDB string
-	WorkingDir                                             string
+	// Env overrides the OS environment, and can be used to specify
+	// GOPROXY, GO111MODULE, etc. PATH cannot be set here, because
+	// exec.Command will not honor it.
+	// Specifying all of RequiredGoEnvVars avoids a call to `go env`.
+	Env map[string]string
+
+	WorkingDir string
 
 	// If Logf is non-nil, debug logging is enabled through this function.
 	Logf func(format string, args ...interface{})
@@ -763,6 +769,22 @@ type ProcessEnv struct {
 	resolver Resolver
 }
 
+func (e *ProcessEnv) goroot() string {
+	return e.mustGetEnv("GOROOT")
+}
+
+func (e *ProcessEnv) gopath() string {
+	return e.mustGetEnv("GOPATH")
+}
+
+func (e *ProcessEnv) mustGetEnv(k string) string {
+	v, ok := e.Env[k]
+	if !ok {
+		panic(fmt.Sprintf("%v not set in evaluated environment", k))
+	}
+	return v
+}
+
 // CopyConfig copies the env's configuration into a new env.
 func (e *ProcessEnv) CopyConfig() *ProcessEnv {
 	copy := *e
@@ -770,21 +792,40 @@ func (e *ProcessEnv) CopyConfig() *ProcessEnv {
 	return &copy
 }
 
-func (e *ProcessEnv) env() []string {
-	env := os.Environ()
-	add := func(k, v string) {
-		if v != "" {
-			env = append(env, k+"="+v)
+func (e *ProcessEnv) init() error {
+	foundAllRequired := true
+	for _, k := range RequiredGoEnvVars {
+		if _, ok := e.Env[k]; !ok {
+			foundAllRequired = false
+			break
 		}
 	}
-	add("GOPATH", e.GOPATH)
-	add("GOROOT", e.GOROOT)
-	add("GO111MODULE", e.GO111MODULE)
-	add("GOPROXY", e.GOPROXY)
-	add("GOFLAGS", e.GOFLAGS)
-	add("GOSUMDB", e.GOSUMDB)
-	if e.WorkingDir != "" {
-		add("PWD", e.WorkingDir)
+	if foundAllRequired {
+		return nil
+	}
+
+	if e.Env == nil {
+		e.Env = map[string]string{}
+	}
+
+	goEnv := map[string]string{}
+	stdout, err := e.invokeGo(context.TODO(), "env", append([]string{"-json"}, RequiredGoEnvVars...)...)
+	if err != nil {
+		return err
+	}
+	if err := json.Unmarshal(stdout.Bytes(), &goEnv); err != nil {
+		return err
+	}
+	for k, v := range goEnv {
+		e.Env[k] = v
+	}
+	return nil
+}
+
+func (e *ProcessEnv) env() []string {
+	var env []string // the gocommand package will prepend os.Environ.
+	for k, v := range e.Env {
+		env = append(env, k+"="+v)
 	}
 	return env
 }
@@ -793,8 +834,7 @@ func (e *ProcessEnv) GetResolver() Resolver {
 	if e.resolver != nil {
 		return e.resolver
 	}
-	out, err := e.invokeGo(context.TODO(), "env", "GOMOD")
-	if err != nil || len(bytes.TrimSpace(out.Bytes())) == 0 {
+	if len(e.Env["GOMOD"]) == 0 {
 		e.resolver = newGopathResolver(e)
 		return e.resolver
 	}
@@ -804,8 +844,8 @@ func (e *ProcessEnv) GetResolver() Resolver {
 
 func (e *ProcessEnv) buildContext() *build.Context {
 	ctx := build.Default
-	ctx.GOROOT = e.GOROOT
-	ctx.GOPATH = e.GOPATH
+	ctx.GOROOT = e.goroot()
+	ctx.GOPATH = e.gopath()
 
 	// As of Go 1.14, build.Context has a Dir field
 	// (see golang.org/issue/34860).
@@ -839,7 +879,7 @@ func (e *ProcessEnv) invokeGo(ctx context.Context, verb string, args ...string)
 func addStdlibCandidates(pass *pass, refs references) {
 	add := func(pkg string) {
 		// Prevent self-imports.
-		if path.Base(pkg) == pass.f.Name.Name && filepath.Join(pass.env.GOROOT, "src", pkg) == pass.srcDir {
+		if path.Base(pkg) == pass.f.Name.Name && filepath.Join(pass.env.goroot(), "src", pkg) == pass.srcDir {
 			return
 		}
 		exports := copyExports(stdlib[pkg])

internal/imports/fix_test.go

@@ -8,7 +8,6 @@ import (
 	"context"
 	"flag"
 	"fmt"
-	"go/build"
 	"log"
 	"path/filepath"
 	"reflect"
@@ -1562,12 +1561,13 @@ var _ = bytes.Buffer
 
 	testConfig{
 		module: packagestest.Module{
-			Name: "ignored.com",
+			Name:  "ignored.com",
+			Files: fm{"x.go": "package x"},
 		},
 	}.test(t, func(t *goimportTest) {
 		// Run in GOROOT/src so that the std module shows up in go list -m all.
-		t.env.WorkingDir = filepath.Join(t.env.GOROOT, "src")
-		got, err := t.processNonModule(filepath.Join(t.env.GOROOT, "src/x.go"), []byte(input), nil)
+		t.env.WorkingDir = filepath.Join(t.env.goroot(), "src")
+		got, err := t.processNonModule(filepath.Join(t.env.goroot(), "src/x.go"), []byte(input), nil)
 		if err != nil {
 			t.Fatalf("Process() = %v", err)
 		}
@@ -1585,10 +1585,11 @@ var _ = ecdsa.GenerateKey
 
 	testConfig{
 		module: packagestest.Module{
-			Name: "ignored.com",
+			Name:  "ignored.com",
+			Files: fm{"x.go": "package x"},
 		},
 	}.test(t, func(t *goimportTest) {
-		got, err := t.processNonModule(filepath.Join(t.env.GOROOT, "src/crypto/ecdsa/foo.go"), []byte(input), nil)
+		got, err := t.processNonModule(filepath.Join(t.env.goroot(), "src/crypto/ecdsa/foo.go"), []byte(input), nil)
 		if err != nil {
 			t.Fatalf("Process() = %v", err)
 		}
@@ -1623,33 +1624,26 @@ func (c testConfig) test(t *testing.T, fn func(*goimportTest)) {
 			exported := packagestest.Export(t, exporter, c.modules)
 			defer exported.Cleanup()
 
-			env := make(map[string]string)
+			env := map[string]string{}
 			for _, kv := range exported.Config.Env {
-				split := strings.Split(kv, "=")
-				k, v := split[0], split[1]
-				env[k] = v
+				split := strings.SplitN(kv, "=", 2)
+				env[split[0]] = split[1]
 			}
-
 			it := &goimportTest{
 				T: t,
 				env: &ProcessEnv{
-					GOROOT:      env["GOROOT"],
-					GOPATH:      env["GOPATH"],
-					GO111MODULE: env["GO111MODULE"],
-					GOSUMDB:     env["GOSUMDB"],
+					Env:         env,
 					WorkingDir:  exported.Config.Dir,
 					GocmdRunner: &gocommand.Runner{},
 				},
 				exported: exported,
 			}
+			if err := it.env.init(); err != nil {
+				t.Fatal(err)
+			}
 			if *testDebug {
 				it.env.Logf = log.Printf
 			}
-			if it.env.GOROOT == "" {
-				// packagestest clears out GOROOT to work around https://golang.org/issue/32849,
-				// which isn't relevant here. Fill it back in so we can find the standard library.
-				it.env.GOROOT = build.Default.GOROOT
-			}
 			fn(it)
 		})
 	}

internal/imports/imports.go

@@ -14,14 +14,12 @@ import (
 	"context"
 	"fmt"
 	"go/ast"
-	"go/build"
 	"go/format"
 	"go/parser"
 	"go/printer"
 	"go/token"
 	"io"
 	"io/ioutil"
-	"os"
 	"regexp"
 	"strconv"
 	"strings"
@@ -146,19 +144,16 @@ func initialize(filename string, src []byte, opt *Options) ([]byte, *Options, er
 
 	// Set the env if the user has not provided it.
 	if opt.Env == nil {
-		opt.Env = &ProcessEnv{
-			GOPATH:      build.Default.GOPATH,
-			GOROOT:      build.Default.GOROOT,
-			GOFLAGS:     os.Getenv("GOFLAGS"),
-			GO111MODULE: os.Getenv("GO111MODULE"),
-			GOPROXY:     os.Getenv("GOPROXY"),
-			GOSUMDB:     os.Getenv("GOSUMDB"),
-		}
+		opt.Env = &ProcessEnv{}
 	}
 	// Set the gocmdRunner if the user has not provided it.
 	if opt.Env.GocmdRunner == nil {
 		opt.Env.GocmdRunner = &gocommand.Runner{}
 	}
+	if err := opt.Env.init(); err != nil {
+		return nil, nil, err
+	}
+
 	if src == nil {
 		b, err := ioutil.ReadFile(filename)
 		if err != nil {

internal/imports/imports_test.go

@@ -1,7 +1,6 @@
 package imports
 
 import (
-	"go/build"
 	"os"
 	"testing"
 
@@ -32,8 +31,6 @@ func TestNilOpts(t *testing.T) {
 			name: "default",
 			opt: &Options{
 				Env: &ProcessEnv{
-					GOPATH:      build.Default.GOPATH,
-					GOROOT:      build.Default.GOROOT,
 					GocmdRunner: &gocommand.Runner{},
 				},
 				Comments:  true,

internal/imports/mod.go

@@ -79,7 +79,11 @@ func (r *ModuleResolver) init() error {
 		r.initAllMods()
 	}
 
-	r.moduleCacheDir = filepath.Join(filepath.SplitList(r.env.GOPATH)[0], "/pkg/mod")
+	if gmc := r.env.Env["GOMODCACHE"]; gmc != "" {
+		r.moduleCacheDir = gmc
+	} else {
+		r.moduleCacheDir = filepath.Join(filepath.SplitList(r.env.gopath())[0], "/pkg/mod")
+	}
 
 	sort.Slice(r.modsByModPath, func(i, j int) bool {
 		count := func(x int) int {
@@ -95,7 +99,7 @@ func (r *ModuleResolver) init() error {
 	})
 
 	r.roots = []gopathwalk.Root{
-		{filepath.Join(r.env.GOROOT, "/src"), gopathwalk.RootGOROOT},
+		{filepath.Join(r.env.goroot(), "/src"), gopathwalk.RootGOROOT},
 	}
 	if r.main != nil {
 		r.roots = append(r.roots, gopathwalk.Root{r.main.Dir, gopathwalk.RootCurrentModule})

internal/imports/mod_test.go

@@ -4,7 +4,6 @@ import (
 	"archive/zip"
 	"context"
 	"fmt"
-	"go/build"
 	"io/ioutil"
 	"log"
 	"os"
@@ -240,7 +239,7 @@ import _ "rsc.io/sampler"
 
 	// Clear out the resolver's cache, since we've changed the environment.
 	mt.resolver = newModuleResolver(mt.env)
-	mt.env.GOFLAGS = "-mod=vendor"
+	mt.env.Env["GOFLAGS"] = "-mod=vendor"
 	mt.assertModuleFoundInDir("rsc.io/sampler", "sampler", `/vendor/`)
 }
 
@@ -686,17 +685,21 @@ func setup(t *testing.T, main, wd string) *modTest {
 	}
 
 	env := &ProcessEnv{
-		GOROOT:      build.Default.GOROOT,
-		GOPATH:      filepath.Join(dir, "gopath"),
-		GO111MODULE: "on",
-		GOPROXY:     proxydir.ToURL(proxyDir),
-		GOSUMDB:     "off",
+		Env: map[string]string{
+			"GOPATH":      filepath.Join(dir, "gopath"),
+			"GO111MODULE": "on",
+			"GOSUMDB":     "off",
+			"GOPROXY":     proxydir.ToURL(proxyDir),
+		},
 		WorkingDir:  filepath.Join(mainDir, wd),
 		GocmdRunner: &gocommand.Runner{},
 	}
 	if *testDebug {
 		env.Logf = log.Printf
 	}
+	if err := env.init(); err != nil {
+		t.Fatal(err)
+	}
 
 	// go mod download gets mad if we don't have a go.mod, so make sure we do.
 	_, err = os.Stat(filepath.Join(mainDir, "go.mod"))
@@ -838,7 +841,7 @@ package x
 import _ "rsc.io/quote"
 `, "")
 	defer mt.cleanup()
-	want := filepath.Join(mt.resolver.env.GOPATH, "pkg/mod", "rsc.io/quote@v1.5.2")
+	want := filepath.Join(mt.resolver.env.gopath(), "pkg/mod", "rsc.io/quote@v1.5.2")
 
 	found := mt.assertScanFinds("rsc.io/quote", "quote")
 	modDir, _ := mt.resolver.modInfo(found.dir)
@@ -864,13 +867,17 @@ func TestInvalidModCache(t *testing.T) {
 		t.Fatal(err)
 	}
 	env := &ProcessEnv{
-		GOROOT:      build.Default.GOROOT,
-		GOPATH:      filepath.Join(dir, "gopath"),
-		GO111MODULE: "on",
-		GOSUMDB:     "off",
+		Env: map[string]string{
+			"GOPATH":      filepath.Join(dir, "gopath"),
+			"GO111MODULE": "on",
+			"GOSUMDB":     "off",
+		},
 		GocmdRunner: &gocommand.Runner{},
 		WorkingDir:  dir,
 	}
+	if err := env.init(); err != nil {
+		t.Fatal(err)
+	}
 	resolver := newModuleResolver(env)
 	scanToSlice(resolver, nil)
 }
@@ -938,11 +945,12 @@ import _ "rsc.io/quote"
 func BenchmarkScanModCache(b *testing.B) {
 	testenv.NeedsGo1Point(b, 11)
 	env := &ProcessEnv{
-		GOPATH:      build.Default.GOPATH,
-		GOROOT:      build.Default.GOROOT,
 		GocmdRunner: &gocommand.Runner{},
 		Logf:        log.Printf,
 	}
+	if err := env.init(); err != nil {
+		b.Fatal(err)
+	}
 	exclude := []gopathwalk.RootType{gopathwalk.RootGOROOT}
 	scanToSlice(env.GetResolver(), exclude)
 	b.ResetTimer()

internal/lsp/cache/view.go

@@ -114,8 +114,10 @@ type View struct {
 	// build system.
 	goCommand bool
 
-	// `go env` variables that need to be tracked.
-	gopath, gocache, goprivate string
+	// `go env` variables that need to be tracked by gopls.
+	gocache, gomodcache, gopath, goprivate string
+
+	goEnv map[string]string
 
 	// gocmdRunner guards go command calls from concurrency errors.
 	gocmdRunner *gocommand.Runner
@@ -464,13 +466,16 @@ func (v *View) populateProcessEnv(ctx context.Context, modFH, sumFH source.FileH
 	cleanup = func() {}
 
 	v.optionsMu.Lock()
-	env, buildFlags := v.envLocked()
+	_, buildFlags := v.envLocked()
 	localPrefix, verboseOutput := v.options.LocalPrefix, v.options.VerboseOutput
 	v.optionsMu.Unlock()
 
 	pe := v.processEnv
-
+	pe.LocalPrefix = localPrefix
+	pe.GocmdRunner = v.gocmdRunner
 	pe.BuildFlags = buildFlags
+	pe.Env = v.goEnv
+	pe.WorkingDir = v.folder.Filename()
 
 	// Add -modfile to the build flags, if we are using it.
 	if modFH != nil {
@@ -482,46 +487,18 @@ func (v *View) populateProcessEnv(ctx context.Context, modFH, sumFH source.FileH
 		pe.BuildFlags = append(pe.BuildFlags, fmt.Sprintf("-modfile=%s", tmpURI.Filename()))
 	}
 
-	pe.WorkingDir = v.folder.Filename()
-	pe.LocalPrefix = localPrefix
-	pe.GocmdRunner = v.gocmdRunner
-
 	if verboseOutput {
 		pe.Logf = func(format string, args ...interface{}) {
 			event.Log(ctx, fmt.Sprintf(format, args...))
 		}
 	}
-	for _, kv := range env {
-		split := strings.SplitN(kv, "=", 2)
-		if len(split) < 2 {
-			continue
-		}
-		switch split[0] {
-		case "GOPATH":
-			pe.GOPATH = split[1]
-		case "GOROOT":
-			pe.GOROOT = split[1]
-		case "GO111MODULE":
-			pe.GO111MODULE = split[1]
-		case "GOPROXY":
-			pe.GOPROXY = split[1]
-		case "GOFLAGS":
-			pe.GOFLAGS = split[1]
-		case "GOSUMDB":
-			pe.GOSUMDB = split[1]
-		}
-	}
-	if pe.GOPATH == "" {
-		return nil, fmt.Errorf("no GOPATH for view %s", v.folder)
-	}
 	return cleanup, nil
 }
 
 // envLocked returns the environment and build flags for the current view.
 // It assumes that the caller is holding the view's optionsMu.
 func (v *View) envLocked() ([]string, []string) {
-	env := []string{fmt.Sprintf("GOPATH=%s", v.gopath)}
-	env = append(env, v.options.Env...)
+	env := append([]string{}, v.options.Env...)
 	buildFlags := append([]string{}, v.options.BuildFlags...)
 	return env, buildFlags
 }
@@ -650,8 +627,7 @@ func (v *View) IgnoredFile(uri span.URI) bool {
 		}
 	} else {
 		mainMod := filepath.Dir(v.modURI.Filename())
-		modCache := filepath.Join(filepath.SplitList(v.gopath)[0], "/pkg/mod")
-		prefixes = []string{mainMod, modCache}
+		prefixes = []string{mainMod, v.gomodcache}
 	}
 
 	for _, prefix := range prefixes {
@@ -747,11 +723,10 @@ func (v *View) setBuildInformation(ctx context.Context, folder span.URI, env []s
 		return fmt.Errorf("invalid workspace configuration: %w", err)
 	}
 	// Make sure to get the `go env` before continuing with initialization.
-	gomod, err := v.getGoEnv(ctx, env)
+	modFile, err := v.setGoEnv(ctx, env)
 	if err != nil {
 		return err
 	}
-	modFile := strings.TrimSpace(gomod)
 	if modFile == os.DevNull {
 		return nil
 	}
@@ -813,80 +788,52 @@ func isSubdirectory(root, leaf string) bool {
 	return err == nil && !strings.HasPrefix(rel, "..")
 }
 
-// getGoEnv sets the view's build information's GOPATH, GOCACHE, GOPRIVATE, and
-// GOPACKAGESDRIVER values.  It also returns the view's GOMOD value, which need
-// not be cached.
-func (v *View) getGoEnv(ctx context.Context, env []string) (string, error) {
-	var gocache, gopath, gopackagesdriver, goprivate bool
-	isGoCommand := func(gopackagesdriver string) bool {
-		return gopackagesdriver == "" || gopackagesdriver == "off"
+// setGoEnv sets the view's various GO* values. It also returns the view's
+// GOMOD value, which need not be cached.
+func (v *View) setGoEnv(ctx context.Context, configEnv []string) (string, error) {
+	var gomod string
+	vars := map[string]*string{
+		"GOCACHE":    &v.gocache,
+		"GOPATH":     &v.gopath,
+		"GOPRIVATE":  &v.goprivate,
+		"GOMODCACHE": &v.gomodcache,
+		"GOMOD":      &gomod,
 	}
-	for _, e := range env {
-		split := strings.Split(e, "=")
-		if len(split) != 2 {
-			continue
-		}
-		switch split[0] {
-		case "GOCACHE":
-			v.gocache = split[1]
-			gocache = true
-		case "GOPATH":
-			v.gopath = split[1]
-			gopath = true
-		case "GOPRIVATE":
-			v.goprivate = split[1]
-			goprivate = true
-		case "GOPACKAGESDRIVER":
-			v.goCommand = isGoCommand(split[1])
-			gopackagesdriver = true
-		}
+	// We can save ~200 ms by requesting only the variables we care about.
+	args := append([]string{"-json"}, imports.RequiredGoEnvVars...)
+	for k := range vars {
+		args = append(args, k)
+	}
+
+	inv := gocommand.Invocation{
+		Verb:       "env",
+		Args:       args,
+		Env:        configEnv,
+		WorkingDir: v.Folder().Filename(),
 	}
 	// Don't go through runGoCommand, as we don't need a temporary -modfile to
 	// run `go env`.
-	inv := gocommand.Invocation{
-		Verb:       "env",
-		Args:       []string{"-json"},
-		Env:        env,
-		WorkingDir: v.Folder().Filename(),
-	}
 	stdout, err := v.gocmdRunner.Run(ctx, inv)
 	if err != nil {
 		return "", err
 	}
-	envMap := make(map[string]string)
-	decoder := json.NewDecoder(stdout)
-	if err := decoder.Decode(&envMap); err != nil {
+	if err := json.Unmarshal(stdout.Bytes(), &v.goEnv); err != nil {
 		return "", err
 	}
-	if !gopath {
-		if gopath, ok := envMap["GOPATH"]; ok {
-			v.gopath = gopath
-		} else {
-			return "", errors.New("unable to determine GOPATH")
-		}
-	}
-	if !gocache {
-		if gocache, ok := envMap["GOCACHE"]; ok {
-			v.gocache = gocache
-		} else {
-			return "", errors.New("unable to determine GOCACHE")
-		}
-	}
-	if !goprivate {
-		if goprivate, ok := envMap["GOPRIVATE"]; ok {
-			v.goprivate = goprivate
-		}
-		// No error here: GOPRIVATE is not essential.
-	}
-	// The value of GOPACKAGESDRIVER is not returned through the go command.
-	if !gopackagesdriver {
-		v.goCommand = isGoCommand(os.Getenv("GOPACKAGESDRIVER"))
-	}
-	if gomod, ok := envMap["GOMOD"]; ok {
-		return gomod, nil
-	}
-	return "", nil
 
+	for key, ptr := range vars {
+		*ptr = v.goEnv[key]
+	}
+
+	// Old versions of Go don't have GOMODCACHE, so emulate it.
+	if v.gomodcache == "" && v.gopath != "" {
+		v.gomodcache = filepath.Join(filepath.SplitList(v.gopath)[0], "pkg/mod")
+	}
+
+	// The value of GOPACKAGESDRIVER is not returned through the go command.
+	gopackagesdriver := os.Getenv("GOPACKAGESDRIVER")
+	v.goCommand = gopackagesdriver == "" || gopackagesdriver == "off"
+	return gomod, nil
 }
 
 func (v *View) IsGoPrivatePath(target string) bool {

internal/lsp/regtest/diagnostics_test.go

@@ -418,7 +418,7 @@ func Hello() {
 	})
 }
 
-// Tests golang/go#37984.
+// Tests golang/go#37984: GOPATH should be read from the go command.
 func TestNoGOPATH_Issue37984(t *testing.T) {
 	const missingImport = `
 -- main.go --
@@ -431,9 +431,8 @@ func _() {
 	runner.Run(t, missingImport, func(t *testing.T, env *Env) {
 		env.OpenFile("main.go")
 		env.Await(env.DiagnosticAtRegexp("main.go", "fmt"))
-		if err := env.Editor.OrganizeImports(env.Ctx, "main.go"); err == nil {
-			t.Fatalf("organize imports should fail with an empty GOPATH")
-		}
+		env.SaveBuffer("main.go")
+		env.Await(EmptyDiagnostics("main.go"))
 	}, WithEditorConfig(fake.EditorConfig{Env: []string{"GOPATH="}}))
 }
 

internal/lsp/regtest/imports_test.go

@@ -1,10 +1,19 @@
 package regtest
 
 import (
+	"io/ioutil"
+	"os"
+	"path/filepath"
+	"strings"
 	"testing"
+	"time"
+
+	"golang.org/x/tools/internal/lsp/fake"
+	"golang.org/x/tools/internal/testenv"
 )
 
-const needs = `
+func TestIssue38815(t *testing.T) {
+	const needs = `
 -- go.mod --
 module foo
 
@@ -12,12 +21,12 @@ module foo
 package main
 func f() {}
 `
-const ntest = `package main
+	const ntest = `package main
 func TestZ(t *testing.T) {
 	f()
 }
 `
-const want = `package main
+	const want = `package main
 
 import "testing"
 
@@ -26,7 +35,6 @@ func TestZ(t *testing.T) {
 }
 `
 
-func TestIssue38815(t *testing.T) {
 	// it was returning
 	// "package main\nimport \"testing\"\npackage main..."
 	runner.Run(t, needs, func(t *testing.T, env *Env) {
@@ -39,7 +47,8 @@ func TestIssue38815(t *testing.T) {
 	})
 }
 
-const vim1 = `package main
+func TestVim1(t *testing.T) {
+	const vim1 = `package main
 
 import "fmt"
 
@@ -53,10 +62,9 @@ func main() {
 }
 `
 
-func TestVim1(t *testing.T) {
 	// The file remains unchanged, but if there are any CodeActions returned, they confuse vim.
 	// Therefore check for no CodeActions
-	runner.Run(t, vim1, func(t *testing.T, env *Env) {
+	runner.Run(t, "", func(t *testing.T, env *Env) {
 		env.CreateBuffer("main.go", vim1)
 		env.OrganizeImports("main.go")
 		actions := env.CodeAction("main.go")
@@ -73,7 +81,8 @@ func TestVim1(t *testing.T) {
 	})
 }
 
-const vim2 = `package main
+func TestVim2(t *testing.T) {
+	const vim2 = `package main
 
 import (
 	"fmt"
@@ -88,8 +97,7 @@ func main() {
 }
 `
 
-func TestVim2(t *testing.T) {
-	runner.Run(t, vim1, func(t *testing.T, env *Env) {
+	runner.Run(t, "", func(t *testing.T, env *Env) {
 		env.CreateBuffer("main.go", vim2)
 		env.OrganizeImports("main.go")
 		actions := env.CodeAction("main.go")
@@ -98,3 +106,51 @@ func TestVim2(t *testing.T) {
 		}
 	})
 }
+
+func TestGOMODCACHE(t *testing.T) {
+	const proxy = `
+-- example.com@v1.2.3/go.mod --
+module example.com
+
+go 1.12
+-- example.com@v1.2.3/x/x.go --
+package x
+
+const X = 1
+-- example.com@v1.2.3/y/y.go --
+package y
+
+const Y = 2
+`
+	const ws = `
+-- go.mod --
+module mod.com
+
+require example.com v1.2.3
+
+-- main.go --
+package main
+
+import "example.com/x"
+
+var _, _ = x.X, y.Y
+`
+	testenv.NeedsGo1Point(t, 15)
+
+	modcache, err := ioutil.TempDir("", "TestGOMODCACHE-modcache")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.RemoveAll(modcache)
+
+	runner.Run(t, ws, func(t *testing.T, env *Env) {
+		env.OpenFile("main.go")
+		env.Await(env.DiagnosticAtRegexp("main.go", `y.Y`))
+		env.SaveBuffer("main.go")
+		env.Await(EmptyDiagnostics("main.go"))
+		path, _ := env.GoToDefinition("main.go", env.RegexpSearch("main.go", `y.(Y)`))
+		if !strings.HasPrefix(path, filepath.ToSlash(modcache)) {
+			t.Errorf("found module dependency outside of GOMODCACHE: got %v, wanted subdir of %v", path, filepath.ToSlash(modcache))
+		}
+	}, WithProxy(proxy), WithEditorConfig(fake.EditorConfig{Env: []string{"GOMODCACHE=" + modcache}}), WithTimeout(5*time.Second))
+}

internal/lsp/regtest/modfile_test.go

@@ -45,7 +45,7 @@ func main() {
 			env.DiagnosticAtRegexp("main.go", "\"example.com/blah\""),
 		)
 		if got := env.ReadWorkspaceFile("go.mod"); got != goModContent {
-			t.Fatalf("go.mod changed on disk:\n%s", tests.Diff(want, got))
+			t.Fatalf("go.mod changed on disk:\n%s", tests.Diff(goModContent, got))
 		}
 		// Save the buffer, which will format and organize imports.
 		// Confirm that the go.mod file still does not change.
@@ -54,7 +54,7 @@ func main() {
 			env.DiagnosticAtRegexp("main.go", "\"example.com/blah\""),
 		)
 		if got := env.ReadWorkspaceFile("go.mod"); got != goModContent {
-			t.Fatalf("go.mod changed on disk:\n%s", tests.Diff(want, got))
+			t.Fatalf("go.mod changed on disk:\n%s", tests.Diff(goModContent, got))
 		}
 	}, WithProxy(proxy))
 }