all: use explicit -mod, -modfile fields for gocommand.Invocation

Build flags, -mod, and -modfile are all accepted by disjoint go command
verbs. Mixing them together had the effect of forcing gocommand users to
figure out which went where themselves, which was annoying and
error-prone. Add ModFlag and ModFile fields to Invocation and update all
uses to use them.

go/packages has a BuildFlags field that's bad for the same reason. Add
private modFlag and modFile fields that forward to the corresponding
Invocation fields.

imports.ProcessEnv gets the same treatment.

Fixes golang/go#41826.

Change-Id: If74d19146e9e62930d7c34f40859c27c25566c4e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/263213
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>

go/internal/packagesdriver/sizes.go

@@ -39,7 +39,12 @@ func GetSizes(ctx context.Context, buildFlags, env []string, gocmdRunner *gocomm
 	}
 
 	if tool == "off" {
-		return GetSizesGolist(ctx, buildFlags, env, gocmdRunner, dir)
+		inv := gocommand.Invocation{
+			BuildFlags: buildFlags,
+			Env:        env,
+			WorkingDir: dir,
+		}
+		return GetSizesGolist(ctx, inv, gocmdRunner)
 	}
 
 	req, err := json.Marshal(struct {
@@ -75,26 +80,17 @@ func GetSizes(ctx context.Context, buildFlags, env []string, gocmdRunner *gocomm
 	return response.Sizes, nil
 }
 
-func GetSizesGolist(ctx context.Context, buildFlags, env []string, gocmdRunner *gocommand.Runner, dir string) (types.Sizes, error) {
-	inv := gocommand.Invocation{
-		Verb:       "list",
-		Args:       []string{"-f", "{{context.GOARCH}} {{context.Compiler}}", "--", "unsafe"},
-		Env:        env,
-		BuildFlags: buildFlags,
-		WorkingDir: dir,
-	}
+func GetSizesGolist(ctx context.Context, inv gocommand.Invocation, gocmdRunner *gocommand.Runner) (types.Sizes, error) {
+	inv.Verb = "list"
+	inv.Args = []string{"-f", "{{context.GOARCH}} {{context.Compiler}}", "--", "unsafe"}
 	stdout, stderr, friendlyErr, rawErr := gocmdRunner.RunRaw(ctx, inv)
 	var goarch, compiler string
 	if rawErr != nil {
 		if strings.Contains(rawErr.Error(), "cannot find main module") {
 			// User's running outside of a module. All bets are off. Get GOARCH and guess compiler is gc.
 			// TODO(matloob): Is this a problem in practice?
-			inv := gocommand.Invocation{
-				Verb:       "env",
-				Args:       []string{"GOARCH"},
-				Env:        env,
-				WorkingDir: dir,
-			}
+			inv.Verb = "env"
+			inv.Args = []string{"GOARCH"}
 			envout, enverr := gocmdRunner.Run(ctx, inv)
 			if enverr != nil {
 				return nil, enverr

go/packages/golist.go

@@ -139,6 +139,12 @@ func goListDriver(cfg *Config, patterns ...string) (*driverResponse, error) {
 
 	response := newDeduper()
 
+	state := &golistState{
+		cfg:        cfg,
+		ctx:        ctx,
+		vendorDirs: map[string]bool{},
+	}
+
 	// Fill in response.Sizes asynchronously if necessary.
 	var sizeserr error
 	var sizeswg sync.WaitGroup
@@ -146,19 +152,13 @@ func goListDriver(cfg *Config, patterns ...string) (*driverResponse, error) {
 		sizeswg.Add(1)
 		go func() {
 			var sizes types.Sizes
-			sizes, sizeserr = packagesdriver.GetSizesGolist(ctx, cfg.BuildFlags, cfg.Env, cfg.gocmdRunner, cfg.Dir)
+			sizes, sizeserr = packagesdriver.GetSizesGolist(ctx, state.cfgInvocation(), cfg.gocmdRunner)
 			// types.SizesFor always returns nil or a *types.StdSizes.
 			response.dr.Sizes, _ = sizes.(*types.StdSizes)
 			sizeswg.Done()
 		}()
 	}
 
-	state := &golistState{
-		cfg:        cfg,
-		ctx:        ctx,
-		vendorDirs: map[string]bool{},
-	}
-
 	// Determine files requested in contains patterns
 	var containFiles []string
 	restPatterns := make([]string, 0, len(patterns))
@@ -839,18 +839,26 @@ func golistargs(cfg *Config, words []string) []string {
 	return fullargs
 }
 
-// invokeGo returns the stdout of a go command invocation.
-func (state *golistState) invokeGo(verb string, args ...string) (*bytes.Buffer, error) {
+// cfgInvocation returns an Invocation that reflects cfg's settings.
+func (state *golistState) cfgInvocation() gocommand.Invocation {
 	cfg := state.cfg
-
-	inv := gocommand.Invocation{
-		Verb:       verb,
-		Args:       args,
+	return gocommand.Invocation{
 		BuildFlags: cfg.BuildFlags,
+		ModFile:    cfg.modFile,
+		ModFlag:    cfg.modFlag,
 		Env:        cfg.Env,
 		Logf:       cfg.Logf,
 		WorkingDir: cfg.Dir,
 	}
+}
+
+// invokeGo returns the stdout of a go command invocation.
+func (state *golistState) invokeGo(verb string, args ...string) (*bytes.Buffer, error) {
+	cfg := state.cfg
+
+	inv := state.cfgInvocation()
+	inv.Verb = verb
+	inv.Args = args
 	gocmdRunner := cfg.gocmdRunner
 	if gocmdRunner == nil {
 		gocmdRunner = &gocommand.Runner{}

go/packages/packages.go

@@ -144,6 +144,12 @@ type Config struct {
 	// the build system's query tool.
 	BuildFlags []string
 
+	// modFile will be used for -modfile in go command invocations.
+	modFile string
+
+	// modFlag will be used for -modfile in go command invocations.
+	modFlag string
+
 	// Fset provides source position information for syntax trees and types.
 	// If Fset is nil, Load will use a new fileset, but preserve Fset's value.
 	Fset *token.FileSet
@@ -366,6 +372,12 @@ func init() {
 	packagesinternal.SetGoCmdRunner = func(config interface{}, runner *gocommand.Runner) {
 		config.(*Config).gocmdRunner = runner
 	}
+	packagesinternal.SetModFile = func(config interface{}, value string) {
+		config.(*Config).modFile = value
+	}
+	packagesinternal.SetModFlag = func(config interface{}, value string) {
+		config.(*Config).modFlag = value
+	}
 	packagesinternal.TypecheckCgo = int(typecheckCgo)
 }
 

internal/gocommand/invoke.go

@@ -130,6 +130,8 @@ type Invocation struct {
 	Verb       string
 	Args       []string
 	BuildFlags []string
+	ModFlag    string
+	ModFile    string
 	Env        []string
 	WorkingDir string
 	Logf       func(format string, args ...interface{})
@@ -158,17 +160,35 @@ func (i *Invocation) run(ctx context.Context, stdout, stderr io.Writer) error {
 	}
 
 	goArgs := []string{i.Verb}
+
+	appendModFile := func() {
+		if i.ModFile != "" {
+			goArgs = append(goArgs, "-modfile="+i.ModFile)
+		}
+	}
+	appendModFlag := func() {
+		if i.ModFlag != "" {
+			goArgs = append(goArgs, "-mod="+i.ModFlag)
+		}
+	}
+
 	switch i.Verb {
-	case "mod":
-		// mod needs the sub-verb before build flags.
-		goArgs = append(goArgs, i.Args[0])
-		goArgs = append(goArgs, i.BuildFlags...)
-		goArgs = append(goArgs, i.Args[1:]...)
-	case "env":
-		// env doesn't take build flags.
+	case "env", "version":
 		goArgs = append(goArgs, i.Args...)
-	default:
+	case "mod":
+		// mod needs the sub-verb before flags.
+		goArgs = append(goArgs, i.Args[0])
+		appendModFile()
+		goArgs = append(goArgs, i.Args[1:]...)
+	case "get":
 		goArgs = append(goArgs, i.BuildFlags...)
+		appendModFile()
+		goArgs = append(goArgs, i.Args...)
+
+	default: // notably list and build.
+		goArgs = append(goArgs, i.BuildFlags...)
+		appendModFile()
+		appendModFlag()
 		goArgs = append(goArgs, i.Args...)
 	}
 	cmd := exec.Command("go", goArgs...)

internal/imports/fix.go

@@ -804,6 +804,8 @@ type ProcessEnv struct {
 	GocmdRunner *gocommand.Runner
 
 	BuildFlags []string
+	ModFlag    string
+	ModFile    string
 
 	// Env overrides the OS environment, and can be used to specify
 	// GOPROXY, GO111MODULE, etc. PATH cannot be set here, because

internal/imports/mod.go

@@ -59,6 +59,8 @@ func (r *ModuleResolver) init() error {
 	}
 	inv := gocommand.Invocation{
 		BuildFlags: r.env.BuildFlags,
+		ModFlag:    r.env.ModFlag,
+		ModFile:    r.env.ModFile,
 		Env:        r.env.env(),
 		Logf:       r.env.Logf,
 		WorkingDir: r.env.WorkingDir,

internal/lsp/cache/imports.go

@@ -160,9 +160,7 @@ func (s *importsState) populateProcessEnv(ctx context.Context, snapshot *snapsho
 		return cleanup, err
 	}
 	if modmod {
-		// -mod isn't really a build flag, but we can get away with it given
-		// the set of commands that goimports wants to run.
-		pe.BuildFlags = append([]string{"-mod=mod"}, pe.BuildFlags...)
+		pe.ModFlag = "mod"
 	}
 
 	// Add -modfile to the build flags, if we are using it.
@@ -172,7 +170,7 @@ func (s *importsState) populateProcessEnv(ctx context.Context, snapshot *snapsho
 		if err != nil {
 			return nil, err
 		}
-		pe.BuildFlags = append(pe.BuildFlags, fmt.Sprintf("-modfile=%s", tmpURI.Filename()))
+		pe.ModFile = tmpURI.Filename()
 	}
 
 	return cleanup, nil

internal/lsp/cache/load.go

@@ -92,7 +92,7 @@ func (s *snapshot) load(ctx context.Context, scopes ...interface{}) error {
 	cleanup := func() {}
 	wdir := s.view.rootURI.Filename()
 
-	var buildFlags []string
+	var modFile string
 	var modURI span.URI
 	var modContent []byte
 	switch {
@@ -141,18 +141,17 @@ func (s *snapshot) load(ctx context.Context, scopes ...interface{}) error {
 		if err != nil {
 			return err
 		}
-		buildFlags = append(buildFlags, fmt.Sprintf("-modfile=%s", tmpURI.Filename()))
+		modFile = tmpURI.Filename()
 	}
 
 	cfg := s.config(ctx, wdir)
-	cfg.BuildFlags = append(cfg.BuildFlags, buildFlags...)
-
+	packagesinternal.SetModFile(cfg, modFile)
 	modMod, err := s.needsModEqualsMod(ctx, modURI, modContent)
 	if err != nil {
 		return err
 	}
 	if modMod {
-		cfg.BuildFlags = append([]string{"-mod=mod"}, cfg.BuildFlags...)
+		packagesinternal.SetModFlag(cfg, "mod")
 	}
 
 	pkgs, err := packages.Load(cfg, query...)

internal/lsp/cache/mod.go

@@ -23,6 +23,7 @@ import (
 	"golang.org/x/tools/internal/lsp/protocol"
 	"golang.org/x/tools/internal/lsp/source"
 	"golang.org/x/tools/internal/memoize"
+	"golang.org/x/tools/internal/packagesinternal"
 	"golang.org/x/tools/internal/span"
 	errors "golang.org/x/xerrors"
 )
@@ -329,7 +330,7 @@ func (s *snapshot) ModUpgrade(ctx context.Context, fh source.FileHandle) (map[st
 		if s.workspaceMode()&tempModfile == 0 || containsVendor(fh.URI()) {
 			// Use -mod=readonly if the module contains a vendor directory
 			// (see golang/go#38711).
-			args = append([]string{"-mod", "readonly"}, args...)
+			packagesinternal.SetModFlag(cfg, "readonly")
 		}
 		stdout, err := snapshot.runGoCommandWithConfig(ctx, cfg, "list", args)
 		if err != nil {

internal/lsp/cache/snapshot.go

@@ -255,13 +255,14 @@ func (s *snapshot) RunGoCommandPiped(ctx context.Context, wd, verb string, args
 func (s *snapshot) goCommandInvocation(ctx context.Context, cfg *packages.Config, allowTempModfile bool, verb string, args []string) (tmpURI span.URI, runner *gocommand.Runner, inv *gocommand.Invocation, cleanup func(), err error) {
 	cleanup = func() {} // fallback
 	modURI := s.GoModForFile(ctx, span.URIFromPath(cfg.Dir))
-	var buildFlags []string
-	// `go mod`, `go env`, and `go version` don't take build flags.
-	switch verb {
-	case "mod", "env", "version":
-	default:
-		buildFlags = append(cfg.BuildFlags, buildFlags...)
+
+	inv = &gocommand.Invocation{
+		Verb:       verb,
+		Args:       args,
+		Env:        cfg.Env,
+		WorkingDir: cfg.Dir,
 	}
+
 	if allowTempModfile && s.workspaceMode()&tempModfile != 0 {
 		if modURI == "" {
 			return "", nil, nil, cleanup, fmt.Errorf("no go.mod file found in %s", cfg.Dir)
@@ -277,40 +278,30 @@ func (s *snapshot) goCommandInvocation(ctx context.Context, cfg *packages.Config
 		if err != nil {
 			return "", nil, nil, cleanup, err
 		}
-		buildFlags = append(buildFlags, fmt.Sprintf("-modfile=%s", tmpURI.Filename()))
+		inv.ModFile = tmpURI.Filename()
 	}
-	// TODO(rstambler): Remove this when golang/go#41826 is resolved.
-	// Don't add -mod=mod for `go mod` or `go get`.
-	switch verb {
-	case "mod", "get":
-	default:
-		var modContent []byte
-		if modURI != "" {
-			modFH, err := s.GetFile(ctx, modURI)
-			if err != nil {
-				return "", nil, nil, cleanup, err
-			}
-			modContent, err = modFH.Read()
-			if err != nil {
-				return "", nil, nil, nil, err
-			}
-		}
-		modMod, err := s.needsModEqualsMod(ctx, modURI, modContent)
+
+	var modContent []byte
+	if modURI != "" {
+		modFH, err := s.GetFile(ctx, modURI)
 		if err != nil {
 			return "", nil, nil, cleanup, err
 		}
-		if modMod {
-			buildFlags = append([]string{"-mod=mod"}, buildFlags...)
+		modContent, err = modFH.Read()
+		if err != nil {
+			return "", nil, nil, nil, err
 		}
 	}
+	modMod, err := s.needsModEqualsMod(ctx, modURI, modContent)
+	if err != nil {
+		return "", nil, nil, cleanup, err
+	}
+	if modMod {
+		inv.ModFlag = "mod"
+	}
+
 	runner = packagesinternal.GetGoCmdRunner(cfg)
-	return tmpURI, runner, &gocommand.Invocation{
-		Verb:       verb,
-		Args:       args,
-		Env:        cfg.Env,
-		BuildFlags: buildFlags,
-		WorkingDir: cfg.Dir,
-	}, cleanup, nil
+	return tmpURI, runner, inv, cleanup, nil
 }
 
 func (s *snapshot) buildOverlay() map[string][]byte {

internal/packagesinternal/packages.go

@@ -12,3 +12,6 @@ var GetGoCmdRunner = func(config interface{}) *gocommand.Runner { return nil }
 var SetGoCmdRunner = func(config interface{}, runner *gocommand.Runner) {}
 
 var TypecheckCgo int
+
+var SetModFlag = func(config interface{}, value string) {}
+var SetModFile = func(config interface{}, value string) {}