go/packages: do not mutate Config

This CL establishes the documented (but previously false)
invariant that Load does not mutate Config.

The stateful fields have been moved:
- gocmdRunner is moved to golistState;
- goListOverlayFile is now a parameter of driver.
  In principle a non-standard driver might use
  this field (as suggested by the Config.Overlay
  documentation) but today none does.
  Nonetheless WriteOverlays is now called before
  the external driver.

Fixes golang/go#67702

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

go/packages/external.go

@@ -79,7 +79,7 @@ type DriverResponse struct {
 
 // driver is the type for functions that query the build system for the
 // packages named by the patterns.
-type driver func(cfg *Config, patterns ...string) (*DriverResponse, error)
+type driver func(cfg *Config, patterns []string) (*DriverResponse, error)
 
 // findExternalDriver returns the file path of a tool that supplies
 // the build system package structure, or "" if not found.
@@ -103,7 +103,7 @@ func findExternalDriver(cfg *Config) driver {
 			return nil
 		}
 	}
-	return func(cfg *Config, words ...string) (*DriverResponse, error) {
+	return func(cfg *Config, patterns []string) (*DriverResponse, error) {
 		req, err := json.Marshal(DriverRequest{
 			Mode:       cfg.Mode,
 			Env:        cfg.Env,
@@ -117,7 +117,7 @@ func findExternalDriver(cfg *Config) driver {
 
 		buf := new(bytes.Buffer)
 		stderr := new(bytes.Buffer)
-		cmd := exec.CommandContext(cfg.Context, tool, words...)
+		cmd := exec.CommandContext(cfg.Context, tool, patterns...)
 		cmd.Dir = cfg.Dir
 		// The cwd gets resolved to the real path. On Darwin, where
 		// /tmp is a symlink, this breaks anything that expects the

go/packages/golist.go

@@ -80,6 +80,12 @@ type golistState struct {
 	cfg *Config
 	ctx context.Context
 
+	runner *gocommand.Runner
+
+	// overlay is the JSON file that encodes the Config.Overlay
+	// mapping, used by 'go list -overlay=...'.
+	overlay string
+
 	envOnce    sync.Once
 	goEnvError error
 	goEnv      map[string]string
@@ -127,7 +133,10 @@ func (state *golistState) mustGetEnv() map[string]string {
 // goListDriver uses the go list command to interpret the patterns and produce
 // the build system package structure.
 // See driver for more details.
-func goListDriver(cfg *Config, patterns ...string) (_ *DriverResponse, err error) {
+//
+// overlay is the JSON file that encodes the cfg.Overlay
+// mapping, used by 'go list -overlay=...'
+func goListDriver(cfg *Config, runner *gocommand.Runner, overlay string, patterns []string) (_ *DriverResponse, err error) {
 	// Make sure that any asynchronous go commands are killed when we return.
 	parentCtx := cfg.Context
 	if parentCtx == nil {
@@ -142,13 +151,15 @@ func goListDriver(cfg *Config, patterns ...string) (_ *DriverResponse, err error
 		cfg:        cfg,
 		ctx:        ctx,
 		vendorDirs: map[string]bool{},
+		overlay:    overlay,
+		runner:     runner,
 	}
 
 	// Fill in response.Sizes asynchronously if necessary.
 	if cfg.Mode&NeedTypesSizes != 0 || cfg.Mode&(NeedTypes|NeedTypesInfo) != 0 {
 		errCh := make(chan error)
 		go func() {
-			compiler, arch, err := getSizesForArgs(ctx, state.cfgInvocation(), cfg.gocmdRunner)
+			compiler, arch, err := getSizesForArgs(ctx, state.cfgInvocation(), runner)
 			response.dr.Compiler = compiler
 			response.dr.Arch = arch
 			errCh <- err
@@ -681,7 +692,7 @@ func (state *golistState) shouldAddFilenameFromError(p *jsonPackage) bool {
 // getGoVersion returns the effective minor version of the go command.
 func (state *golistState) getGoVersion() (int, error) {
 	state.goVersionOnce.Do(func() {
-		state.goVersion, state.goVersionError = gocommand.GoVersion(state.ctx, state.cfgInvocation(), state.cfg.gocmdRunner)
+		state.goVersion, state.goVersionError = gocommand.GoVersion(state.ctx, state.cfgInvocation(), state.runner)
 	})
 	return state.goVersion, state.goVersionError
 }
@@ -840,7 +851,7 @@ func (state *golistState) cfgInvocation() gocommand.Invocation {
 		Env:        cfg.Env,
 		Logf:       cfg.Logf,
 		WorkingDir: cfg.Dir,
-		Overlay:    cfg.goListOverlayFile,
+		Overlay:    state.overlay,
 	}
 }
 
@@ -851,11 +862,8 @@ func (state *golistState) invokeGo(verb string, args ...string) (*bytes.Buffer,
 	inv := state.cfgInvocation()
 	inv.Verb = verb
 	inv.Args = args
-	gocmdRunner := cfg.gocmdRunner
-	if gocmdRunner == nil {
-		gocmdRunner = &gocommand.Runner{}
-	}
-	stdout, stderr, friendlyErr, err := gocmdRunner.RunRaw(cfg.Context, inv)
+
+	stdout, stderr, friendlyErr, err := state.runner.RunRaw(cfg.Context, inv)
 	if err != nil {
 		// Check for 'go' executable not being found.
 		if ee, ok := err.(*exec.Error); ok && ee.Err == exec.ErrNotFound {

go/packages/packages.go

@@ -144,13 +144,7 @@ const (
 // A Config specifies details about how packages should be loaded.
 // The zero value is a valid configuration.
 //
-// Calls to Load do not modify this struct.
-//
-// TODO(adonovan): #67702: this is currently false: in fact,
-// calls to [Load] do not modify the public fields of this struct, but
-// may modify hidden fields, so concurrent calls to [Load] must not
-// use the same Config. But perhaps we should reestablish the
-// documented invariant.
+// Calls to [Load] do not modify this struct.
 type Config struct {
 	// Mode controls the level of information returned for each package.
 	Mode LoadMode
@@ -181,19 +175,10 @@ type Config struct {
 	//
 	Env []string
 
-	// gocmdRunner guards go command calls from concurrency errors.
-	gocmdRunner *gocommand.Runner
-
 	// BuildFlags is a list of command-line flags to be passed through to
 	// 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
@@ -240,9 +225,13 @@ type Config struct {
 	// drivers may vary in their level of support for overlays.
 	Overlay map[string][]byte
 
-	// goListOverlayFile is the JSON file that encodes the Overlay
-	// mapping, used by 'go list -overlay=...'
-	goListOverlayFile string
+	// -- Hidden configuration fields only for use in x/tools --
+
+	// modFile will be used for -modfile in go command invocations.
+	modFile string
+
+	// modFlag will be used for -modfile in go command invocations.
+	modFlag string
 }
 
 // Load loads and returns the Go packages named by the given patterns.
@@ -333,21 +322,24 @@ func defaultDriver(cfg *Config, patterns ...string) (*DriverResponse, bool, erro
 		} else if !response.NotHandled {
 			return response, true, nil
 		}
-		// (fall through)
+		// not handled: fall through
 	}
 
 	// go list fallback
-	//
+
 	// Write overlays once, as there are many calls
 	// to 'go list' (one per chunk plus others too).
-	overlay, cleanupOverlay, err := gocommand.WriteOverlays(cfg.Overlay)
+	overlayFile, cleanupOverlay, err := gocommand.WriteOverlays(cfg.Overlay)
 	if err != nil {
 		return nil, false, err
 	}
 	defer cleanupOverlay()
-	cfg.goListOverlayFile = overlay
 
-	response, err := callDriverOnChunks(goListDriver, cfg, chunks)
+	var runner gocommand.Runner // (shared across many 'go list' calls)
+	driver := func(cfg *Config, patterns []string) (*DriverResponse, error) {
+		return goListDriver(cfg, &runner, overlayFile, patterns)
+	}
+	response, err := callDriverOnChunks(driver, cfg, chunks)
 	if err != nil {
 		return nil, false, err
 	}
@@ -385,16 +377,14 @@ func splitIntoChunks(patterns []string, argMax int) ([][]string, error) {
 
 func callDriverOnChunks(driver driver, cfg *Config, chunks [][]string) (*DriverResponse, error) {
 	if len(chunks) == 0 {
-		return driver(cfg)
+		return driver(cfg, nil)
 	}
 	responses := make([]*DriverResponse, len(chunks))
 	errNotHandled := errors.New("driver returned NotHandled")
 	var g errgroup.Group
 	for i, chunk := range chunks {
-		i := i
-		chunk := chunk
 		g.Go(func() (err error) {
-			responses[i], err = driver(cfg, chunk...)
+			responses[i], err = driver(cfg, chunk)
 			if responses[i] != nil && responses[i].NotHandled {
 				err = errNotHandled
 			}
@@ -749,9 +739,6 @@ func newLoader(cfg *Config) *loader {
 	if ld.Config.Env == nil {
 		ld.Config.Env = os.Environ()
 	}
-	if ld.Config.gocmdRunner == nil {
-		ld.Config.gocmdRunner = &gocommand.Runner{}
-	}
 	if ld.Context == nil {
 		ld.Context = context.Background()
 	}