gopls/internal/cache: simplify usage of snapshot.GoCommandInvocation

The Snapshot.GoCommandInvocation API is a source of confusion, because
it passes in a gocommand.Invocation, performs arbitrary mutation, and
then passes it back to the caller (which may perform mutation on top).
Aside from the readability problems of this indirectness, this API can
lead to real bugs, for example if one of the mutations is incompatible
with another.

Furthermore, the fact that the GoCommandInvocation is the golden source
of information about the Go command environment leads to awkwardness and
redundant work. For example, to get the packages.Config we called
Snapshot.GoCommandInvocation, just to read env, working dir, and build
flags (and the now irrelevant ModFile and ModFlag). But
GoCommandInvocation wrote overlays, so we'd end up writing overlays
twice: once within the gopls layer, and then again in the go/packages go
list driver.

Simplify as follows:
- Pass in dir, verb, args, and env to GoCommandInvocation, to avoid
  passing around and mutating an Invocation.
- Extract the View.Env, which is a useful concept, and apply invocation
  env on top. Surveying existing use cases that this was correct, as all
  call sites expected their env not to be overwritten.
- Move Snapshot.config to load.go, where it belongs, and simplify not to
  depend on GoCommandInvocation.

Also add an additional test case for BenchmarkReload.

Change-Id: I8ae7a13a033360e0e7b0b24ff718b5a22123e99c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/626715
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Robert Findley <rfindley@google.com>

Author: findleyr

gopls/internal/cache/load.go

@@ -9,6 +9,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"go/types"
 	"path/filepath"
 	"slices"
 	"sort"
@@ -25,8 +26,8 @@ import (
 	"golang.org/x/tools/gopls/internal/util/immutable"
 	"golang.org/x/tools/gopls/internal/util/pathutil"
 	"golang.org/x/tools/internal/event"
-	"golang.org/x/tools/internal/gocommand"
 	"golang.org/x/tools/internal/packagesinternal"
+	"golang.org/x/tools/internal/typesinternal"
 	"golang.org/x/tools/internal/xcontext"
 )
 
@@ -57,6 +58,7 @@ func (s *Snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc
 	// Keep track of module query -> module path so that we can later correlate query
 	// errors with errors.
 	moduleQueries := make(map[string]string)
+
 	for _, scope := range scopes {
 		switch scope := scope.(type) {
 		case packageLoadScope:
@@ -118,21 +120,13 @@ func (s *Snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc
 
 	startTime := time.Now()
 
-	inv, cleanupInvocation, err := s.GoCommandInvocation(allowNetwork, &gocommand.Invocation{
-		WorkingDir: s.view.root.Path(),
-	})
-	if err != nil {
-		return err
-	}
-	defer cleanupInvocation()
-
 	// Set a last resort deadline on packages.Load since it calls the go
 	// command, which may hang indefinitely if it has a bug. golang/go#42132
 	// and golang/go#42255 have more context.
 	ctx, cancel := context.WithTimeout(ctx, 10*time.Minute)
 	defer cancel()
 
-	cfg := s.config(ctx, inv)
+	cfg := s.config(ctx, allowNetwork)
 	pkgs, err := packages.Load(cfg, query...)
 
 	// If the context was canceled, return early. Otherwise, we might be
@@ -278,7 +272,7 @@ func (s *Snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc
 		if allFilesExcluded(pkg.GoFiles, filterFunc) {
 			continue
 		}
-		buildMetadata(newMetadata, pkg, cfg.Dir, standalone, s.view.typ != GoPackagesDriverView)
+		buildMetadata(newMetadata, cfg.Dir, standalone, pkg)
 	}
 
 	s.mu.Lock()
@@ -362,14 +356,56 @@ func (m *moduleErrorMap) Error() string {
 	return buf.String()
 }
 
+// config returns the configuration used for the snapshot's interaction with
+// the go/packages API. It uses the given working directory.
+//
+// TODO(rstambler): go/packages requires that we do not provide overlays for
+// multiple modules in one config, so buildOverlay needs to filter overlays by
+// module.
+// TODO(rfindley): ^^ is this still true?
+func (s *Snapshot) config(ctx context.Context, allowNetwork bool) *packages.Config {
+	cfg := &packages.Config{
+		Context:    ctx,
+		Dir:        s.view.root.Path(),
+		Env:        s.view.Env(),
+		BuildFlags: slices.Clone(s.view.folder.Options.BuildFlags),
+		Mode: packages.NeedName |
+			packages.NeedFiles |
+			packages.NeedCompiledGoFiles |
+			packages.NeedImports |
+			packages.NeedDeps |
+			packages.NeedTypesSizes |
+			packages.NeedModule |
+			packages.NeedEmbedFiles |
+			packages.LoadMode(packagesinternal.DepsErrors) |
+			packages.NeedForTest,
+		Fset:    nil, // we do our own parsing
+		Overlay: s.buildOverlays(),
+		Logf: func(format string, args ...interface{}) {
+			if s.view.folder.Options.VerboseOutput {
+				event.Log(ctx, fmt.Sprintf(format, args...))
+			}
+		},
+		Tests: true,
+	}
+	if !allowNetwork {
+		cfg.Env = append(cfg.Env, "GOPROXY=off")
+	}
+	// We want to type check cgo code if go/types supports it.
+	if typesinternal.SetUsesCgo(&types.Config{}) {
+		cfg.Mode |= packages.LoadMode(packagesinternal.TypecheckCgo)
+	}
+	return cfg
+}
+
 // buildMetadata populates the updates map with metadata updates to
 // apply, based on the given pkg. It recurs through pkg.Imports to ensure that
 // metadata exists for all dependencies.
 //
 // Returns the metadata.Package that was built (or which was already present in
 // updates), or nil if the package could not be built. Notably, the resulting
 // metadata.Package may have an ID that differs from pkg.ID.
-func buildMetadata(updates map[PackageID]*metadata.Package, pkg *packages.Package, loadDir string, standalone, goListView bool) *metadata.Package {
+func buildMetadata(updates map[PackageID]*metadata.Package, loadDir string, standalone bool, pkg *packages.Package) *metadata.Package {
 	// Allow for multiple ad-hoc packages in the workspace (see #47584).
 	pkgPath := PackagePath(pkg.PkgPath)
 	id := PackageID(pkg.ID)
@@ -524,7 +560,7 @@ func buildMetadata(updates map[PackageID]*metadata.Package, pkg *packages.Packag
 			continue
 		}
 
-		dep := buildMetadata(updates, imported, loadDir, false, goListView) // only top level packages can be standalone
+		dep := buildMetadata(updates, loadDir, false, imported) // only top level packages can be standalone
 
 		// Don't record edges to packages with no name, as they cause trouble for
 		// the importer (golang/go#60952).

gopls/internal/cache/mod.go

@@ -19,7 +19,6 @@ import (
 	"golang.org/x/tools/gopls/internal/protocol"
 	"golang.org/x/tools/gopls/internal/protocol/command"
 	"golang.org/x/tools/internal/event"
-	"golang.org/x/tools/internal/gocommand"
 	"golang.org/x/tools/internal/memoize"
 )
 
@@ -252,11 +251,7 @@ func modWhyImpl(ctx context.Context, snapshot *Snapshot, fh file.Handle) (map[st
 	for _, req := range pm.File.Require {
 		args = append(args, req.Mod.Path)
 	}
-	inv, cleanupInvocation, err := snapshot.GoCommandInvocation(false, &gocommand.Invocation{
-		Verb:       "mod",
-		Args:       args,
-		WorkingDir: filepath.Dir(fh.URI().Path()),
-	})
+	inv, cleanupInvocation, err := snapshot.GoCommandInvocation(false, filepath.Dir(fh.URI().Path()), "mod", args)
 	if err != nil {
 		return nil, err
 	}

gopls/internal/cache/mod_tidy.go

@@ -23,7 +23,6 @@ import (
 	"golang.org/x/tools/gopls/internal/protocol/command"
 	"golang.org/x/tools/internal/diff"
 	"golang.org/x/tools/internal/event"
-	"golang.org/x/tools/internal/gocommand"
 	"golang.org/x/tools/internal/memoize"
 )
 
@@ -108,12 +107,13 @@ func modTidyImpl(ctx context.Context, snapshot *Snapshot, pm *ParsedModule) (*Ti
 	}
 	defer cleanup()
 
-	inv, cleanupInvocation, err := snapshot.GoCommandInvocation(false, &gocommand.Invocation{
-		Verb:       "mod",
-		Args:       []string{"tidy", "-modfile=" + filepath.Join(tempDir, "go.mod")},
-		Env:        []string{"GOWORK=off"},
-		WorkingDir: pm.URI.Dir().Path(),
-	})
+	inv, cleanupInvocation, err := snapshot.GoCommandInvocation(
+		false,
+		pm.URI.Dir().Path(),
+		"mod",
+		[]string{"tidy", "-modfile=" + filepath.Join(tempDir, "go.mod")},
+		"GOWORK=off",
+	)
 	if err != nil {
 		return nil, err
 	}

gopls/internal/cache/snapshot.go

@@ -13,7 +13,6 @@ import (
 	"go/build/constraint"
 	"go/parser"
 	"go/token"
-	"go/types"
 	"os"
 	"path"
 	"path/filepath"
@@ -26,7 +25,6 @@ import (
 	"sync"
 
 	"golang.org/x/sync/errgroup"
-	"golang.org/x/tools/go/packages"
 	"golang.org/x/tools/go/types/objectpath"
 	"golang.org/x/tools/gopls/internal/cache/metadata"
 	"golang.org/x/tools/gopls/internal/cache/methodsets"
@@ -49,8 +47,6 @@ import (
 	"golang.org/x/tools/internal/event/label"
 	"golang.org/x/tools/internal/gocommand"
 	"golang.org/x/tools/internal/memoize"
-	"golang.org/x/tools/internal/packagesinternal"
-	"golang.org/x/tools/internal/typesinternal"
 )
 
 // A Snapshot represents the current state for a given view.
@@ -363,50 +359,6 @@ func (s *Snapshot) Templates() map[protocol.DocumentURI]file.Handle {
 	return tmpls
 }
 
-// config returns the configuration used for the snapshot's interaction with
-// the go/packages API. It uses the given working directory.
-//
-// TODO(rstambler): go/packages requires that we do not provide overlays for
-// multiple modules in one config, so buildOverlay needs to filter overlays by
-// module.
-func (s *Snapshot) config(ctx context.Context, inv *gocommand.Invocation) *packages.Config {
-
-	cfg := &packages.Config{
-		Context:    ctx,
-		Dir:        inv.WorkingDir,
-		Env:        inv.Env,
-		BuildFlags: inv.BuildFlags,
-		Mode: packages.NeedName |
-			packages.NeedFiles |
-			packages.NeedCompiledGoFiles |
-			packages.NeedImports |
-			packages.NeedDeps |
-			packages.NeedTypesSizes |
-			packages.NeedModule |
-			packages.NeedEmbedFiles |
-			packages.LoadMode(packagesinternal.DepsErrors) |
-			packages.NeedForTest,
-		Fset:    nil, // we do our own parsing
-		Overlay: s.buildOverlays(),
-		ParseFile: func(*token.FileSet, string, []byte) (*ast.File, error) {
-			panic("go/packages must not be used to parse files")
-		},
-		Logf: func(format string, args ...interface{}) {
-			if s.Options().VerboseOutput {
-				event.Log(ctx, fmt.Sprintf(format, args...))
-			}
-		},
-		Tests: true,
-	}
-	packagesinternal.SetModFile(cfg, inv.ModFile)
-	packagesinternal.SetModFlag(cfg, inv.ModFlag)
-	// We want to type check cgo code if go/types supports it.
-	if typesinternal.SetUsesCgo(&types.Config{}) {
-		cfg.Mode |= packages.LoadMode(packagesinternal.TypecheckCgo)
-	}
-	return cfg
-}
-
 // RunGoModUpdateCommands runs a series of `go` commands that updates the go.mod
 // and go.sum file for wd, and returns their updated contents.
 //
@@ -423,16 +375,14 @@ func (s *Snapshot) RunGoModUpdateCommands(ctx context.Context, modURI protocol.D
 	// TODO(rfindley): we must use ModFlag and ModFile here (rather than simply
 	// setting Args), because without knowing the verb, we can't know whether
 	// ModFlag is appropriate. Refactor so that args can be set by the caller.
-	inv, cleanupInvocation, err := s.GoCommandInvocation(true, &gocommand.Invocation{
-		WorkingDir: modURI.Dir().Path(),
-		ModFlag:    "mod",
-		ModFile:    filepath.Join(tempDir, "go.mod"),
-		Env:        []string{"GOWORK=off"},
-	})
+	inv, cleanupInvocation, err := s.GoCommandInvocation(true, modURI.Dir().Path(), "", nil, "GOWORK=off")
 	if err != nil {
 		return nil, nil, err
 	}
 	defer cleanupInvocation()
+
+	inv.ModFlag = "mod"
+	inv.ModFile = filepath.Join(tempDir, "go.mod")
 	invoke := func(args ...string) (*bytes.Buffer, error) {
 		inv.Verb = args[0]
 		inv.Args = args[1:]
@@ -510,22 +460,16 @@ func TempModDir(ctx context.Context, fs file.Source, modURI protocol.DocumentURI
 // BuildFlags should be more clearly expressed in the API.
 //
 // If allowNetwork is set, do not set GOPROXY=off.
-func (s *Snapshot) GoCommandInvocation(allowNetwork bool, inv *gocommand.Invocation) (_ *gocommand.Invocation, cleanup func(), _ error) {
-	// TODO(rfindley): it's not clear that this is doing the right thing.
-	// Should inv.Env really overwrite view.options? Should s.view.envOverlay
-	// overwrite inv.Env? (Do we ever invoke this with a non-empty inv.Env?)
-	//
-	// We should survey existing uses and write down rules for how env is
-	// applied.
-	inv.Env = slices.Concat(
-		os.Environ(),
-		s.Options().EnvSlice(),
-		inv.Env,
-		[]string{"GO111MODULE=" + s.view.adjustedGO111MODULE()},
-		s.view.EnvOverlay(),
-	)
-	inv.BuildFlags = slices.Clone(s.Options().BuildFlags)
-
+//
+// TODO(rfindley): use a named boolean for allowNetwork.
+func (s *Snapshot) GoCommandInvocation(allowNetwork bool, dir, verb string, args []string, env ...string) (_ *gocommand.Invocation, cleanup func(), _ error) {
+	inv := &gocommand.Invocation{
+		Verb:       verb,
+		Args:       args,
+		WorkingDir: dir,
+		Env:        append(s.view.Env(), env...),
+		BuildFlags: slices.Clone(s.Options().BuildFlags),
+	}
 	if !allowNetwork {
 		inv.Env = append(inv.Env, "GOPROXY=off")
 	}

gopls/internal/cache/view.go

@@ -356,6 +356,16 @@ func (v *View) Folder() *Folder {
 	return v.folder
 }
 
+// Env returns the environment to use for running go commands in this view.
+func (v *View) Env() []string {
+	return slices.Concat(
+		os.Environ(),
+		v.folder.Options.EnvSlice(),
+		[]string{"GO111MODULE=" + v.adjustedGO111MODULE()},
+		v.EnvOverlay(),
+	)
+}
+
 // UpdateFolders updates the set of views for the new folders.
 //
 // Calling this causes each view to be reinitialized.

gopls/internal/golang/assembly.go

@@ -16,13 +16,11 @@ import (
 	"context"
 	"fmt"
 	"html"
-	"path/filepath"
 	"regexp"
 	"strconv"
 	"strings"
 
 	"golang.org/x/tools/gopls/internal/cache"
-	"golang.org/x/tools/internal/gocommand"
 )
 
 // AssemblyHTML returns an HTML document containing an assembly listing of the selected function.
@@ -32,11 +30,7 @@ import (
 // - cross-link jumps and block labels, like github.com/aclements/objbrowse.
 func AssemblyHTML(ctx context.Context, snapshot *cache.Snapshot, pkg *cache.Package, symbol string, web Web) ([]byte, error) {
 	// Compile the package with -S, and capture its stderr stream.
-	inv, cleanupInvocation, err := snapshot.GoCommandInvocation(false, &gocommand.Invocation{
-		Verb:       "build",
-		Args:       []string{"-gcflags=-S", "."},
-		WorkingDir: filepath.Dir(pkg.Metadata().CompiledGoFiles[0].Path()),
-	})
+	inv, cleanupInvocation, err := snapshot.GoCommandInvocation(false, pkg.Metadata().CompiledGoFiles[0].Dir().Path(), "build", []string{"-gcflags=-S", "."})
 	if err != nil {
 		return nil, err // e.g. failed to write overlays (rare)
 	}

gopls/internal/golang/gc_annotations.go

@@ -18,7 +18,6 @@ import (
 	"golang.org/x/tools/gopls/internal/protocol"
 	"golang.org/x/tools/gopls/internal/settings"
 	"golang.org/x/tools/internal/event"
-	"golang.org/x/tools/internal/gocommand"
 )
 
 // GCOptimizationDetails invokes the Go compiler on the specified
@@ -57,14 +56,10 @@ func GCOptimizationDetails(ctx context.Context, snapshot *cache.Snapshot, mp *me
 	if !strings.HasPrefix(outDir, "/") {
 		outDirURI = protocol.DocumentURI(strings.Replace(string(outDirURI), "file:///", "file://", 1))
 	}
-	inv, cleanupInvocation, err := snapshot.GoCommandInvocation(false, &gocommand.Invocation{
-		Verb: "build",
-		Args: []string{
-			fmt.Sprintf("-gcflags=-json=0,%s", outDirURI),
-			fmt.Sprintf("-o=%s", tmpFile.Name()),
-			".",
-		},
-		WorkingDir: pkgDir,
+	inv, cleanupInvocation, err := snapshot.GoCommandInvocation(false, pkgDir, "build", []string{
+		fmt.Sprintf("-gcflags=-json=0,%s", outDirURI),
+		fmt.Sprintf("-o=%s", tmpFile.Name()),
+		".",
 	})
 	if err != nil {
 		return nil, err