cmd/go: fix race applying fixes in fix and vet -fix modes

Previously, the cmd/fix tool, which is analogous to a compiler
in a "go fix" or "go vet -fix" build, applied its fixes directly
to source files during the build. However, this led to races
since the edits may in some cases occur concurrently with other
build steps that are still reading those source file.

This change separates the computation of the fixes, which
happens during the build, and applying the fixes, which happens
in a phase after the build.

The unitchecker now accepts a FixArchive file name (see CL 726940).
If it is non-empty, the unitchecker will write the fixed files
into an archive instead of updating them directly.

The vet build sets this option, then reads the produced zip
files in the second phase. The files are saved in the cache;
some care is required to sequence the various cache operations
so that a cache hit has all-or-nothing semantics.

The tweak to vet_basic.txt is a sign that there was a latent
bug, inadvertently fixed by this change: because the old approach
relied on side effects of cmd/fix to mutate files, rather than
the current pure-functional approach of computing fixes which
are then applied as a second pass, a cache hit would cause some
edits not to be applied. Now they are applied.

Fixes #71859

Change-Id: Ib8e70644ec246dcdb20a90794c11ea6fd420247d
Reviewed-on: https://go-review.googlesource.com/c/go/+/727000
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Matloob <matloob@google.com>

Author: adonovan

src/cmd/go/internal/test/test.go

@@ -1371,7 +1371,7 @@ func addTestVet(loaderstate *modload.State, b *work.Builder, p *load.Package, ru
 		return
 	}
 
-	vet := b.VetAction(loaderstate, work.ModeBuild, work.ModeBuild, p)
+	vet := b.VetAction(loaderstate, work.ModeBuild, work.ModeBuild, false, p)
 	runAction.Deps = append(runAction.Deps, vet)
 	// Install will clean the build directory.
 	// Make sure vet runs first.

src/cmd/go/internal/vet/vet.go

@@ -6,6 +6,8 @@
 package vet
 
 import (
+	"archive/zip"
+	"bytes"
 	"context"
 	"encoding/json"
 	"errors"
@@ -176,6 +178,7 @@ func run(ctx context.Context, cmd *base.Command, args []string) {
 
 	work.VetExplicit = len(toolFlags) > 0
 
+	applyFixes := false
 	if cmd.Name() == "fix" || *vetFixFlag {
 		// fix mode: 'go fix' or 'go vet -fix'
 		if jsonFlag {
@@ -186,6 +189,8 @@ func run(ctx context.Context, cmd *base.Command, args []string) {
 			toolFlags = append(toolFlags, "-fix")
 			if diffFlag {
 				toolFlags = append(toolFlags, "-diff")
+			} else {
+				applyFixes = true
 			}
 		}
 		if contextFlag != -1 {
@@ -226,13 +231,21 @@ func run(ctx context.Context, cmd *base.Command, args []string) {
 		base.Fatalf("no packages to %s", cmd.Name())
 	}
 
+	// Build action graph.
 	b := work.NewBuilder("", moduleLoaderState.VendorDirOrEmpty)
 	defer func() {
 		if err := b.Close(); err != nil {
 			base.Fatal(err)
 		}
 	}()
 
+	root := &work.Action{Mode: "go " + cmd.Name()}
+
+	addVetAction := func(p *load.Package) {
+		act := b.VetAction(moduleLoaderState, work.ModeBuild, work.ModeBuild, applyFixes, p)
+		root.Deps = append(root.Deps, act)
+	}
+
 	// To avoid file corruption from duplicate application of
 	// fixes (in fix mode), and duplicate reporting of diagnostics
 	// (in vet mode), we must run the tool only once for each
@@ -248,8 +261,6 @@ func run(ctx context.Context, cmd *base.Command, args []string) {
 	// We needn't worry about intermediate test variants, as they
 	// will only be executed in VetxOnly mode, for facts but not
 	// diagnostics.
-
-	root := &work.Action{Mode: "go " + cmd.Name()}
 	for _, p := range pkgs {
 		_, ptest, pxtest, perr := load.TestPackagesFor(moduleLoaderState, ctx, pkgOpts, p, nil)
 		if perr != nil {
@@ -262,13 +273,65 @@ func run(ctx context.Context, cmd *base.Command, args []string) {
 		}
 		if len(ptest.GoFiles) > 0 || len(ptest.CgoFiles) > 0 {
 			// The test package includes all the files of primary package.
-			root.Deps = append(root.Deps, b.VetAction(moduleLoaderState, work.ModeBuild, work.ModeBuild, ptest))
+			addVetAction(ptest)
 		}
 		if pxtest != nil {
-			root.Deps = append(root.Deps, b.VetAction(moduleLoaderState, work.ModeBuild, work.ModeBuild, pxtest))
+			addVetAction(pxtest)
 		}
 	}
 	b.Do(ctx, root)
+
+	// Apply fixes.
+	//
+	// We do this as a separate phase after the build to avoid
+	// races between source file updates and reads of those same
+	// files by concurrent actions of the ongoing build.
+	//
+	// If a file is fixed by multiple actions, they must be consistent.
+	if applyFixes {
+		contents := make(map[string][]byte)
+		// Gather the fixes.
+		for _, act := range root.Deps {
+			if act.FixArchive != "" {
+				if err := readZip(act.FixArchive, contents); err != nil {
+					base.Errorf("reading archive of fixes: %v", err)
+					return
+				}
+			}
+		}
+		// Apply them.
+		for filename, content := range contents {
+			if err := os.WriteFile(filename, content, 0644); err != nil {
+				base.Errorf("applying fix: %v", err)
+			}
+		}
+	}
+}
+
+// readZip reads the zipfile entries into the provided map.
+// It reports an error if updating the map would change an existing entry.
+func readZip(zipfile string, out map[string][]byte) error {
+	r, err := zip.OpenReader(zipfile)
+	if err != nil {
+		return err
+	}
+	defer r.Close() // ignore error
+	for _, f := range r.File {
+		rc, err := f.Open()
+		if err != nil {
+			return err
+		}
+		content, err := io.ReadAll(rc)
+		rc.Close() // ignore error
+		if err != nil {
+			return err
+		}
+		if prev, ok := out[f.Name]; ok && !bytes.Equal(prev, content) {
+			return fmt.Errorf("inconsistent fixes to file %v", f.Name)
+		}
+		out[f.Name] = content
+	}
+	return nil
 }
 
 // printJSONDiagnostics parses JSON (from the tool's stdout) and

src/cmd/go/internal/work/action.go

@@ -109,11 +109,13 @@ type Action struct {
 	actionID         cache.ActionID // cache ID of action input
 	buildID          string         // build ID of action output
 
-	VetxOnly  bool       // Mode=="vet": only being called to supply info about dependencies
-	needVet   bool       // Mode=="build": need to fill in vet config
-	needBuild bool       // Mode=="build": need to do actual build (can be false if needVet is true)
-	vetCfg    *vetConfig // vet config
-	output    []byte     // output redirect buffer (nil means use b.Print)
+	VetxOnly   bool       // Mode=="vet": only being called to supply info about dependencies
+	needVet    bool       // Mode=="build": need to fill in vet config
+	needBuild  bool       // Mode=="build": need to do actual build (can be false if needVet is true)
+	needFix    bool       // Mode=="vet": need secondary target, a .zip file containing fixes
+	vetCfg     *vetConfig // vet config
+	FixArchive string     // the created .zip file containing fixes (if needFix)
+	output     []byte     // output redirect buffer (nil means use b.Print)
 
 	sh *Shell // lazily created per-Action shell; see Builder.Shell
 
@@ -869,9 +871,10 @@ func (b *Builder) cgoAction(p *load.Package, objdir string, deps []*Action, hasC
 // It depends on the action for compiling p.
 // If the caller may be causing p to be installed, it is up to the caller
 // to make sure that the install depends on (runs after) vet.
-func (b *Builder) VetAction(s *modload.State, mode, depMode BuildMode, p *load.Package) *Action {
+func (b *Builder) VetAction(s *modload.State, mode, depMode BuildMode, needFix bool, p *load.Package) *Action {
 	a := b.vetAction(s, mode, depMode, p)
 	a.VetxOnly = false
+	a.needFix = needFix
 	return a
 }
 

src/cmd/go/internal/work/exec.go

@@ -1187,6 +1187,7 @@ type vetConfig struct {
 	VetxOutput    string            // write vetx data to this output file
 	Stdout        string            // write stdout (JSON, unified diff) to this output file
 	GoVersion     string            // Go version for package
+	FixArchive    string            // write fixed files to this zip archive, if non-empty
 
 	SucceedOnTypecheckFailure bool // awful hack; see #18395 and below
 }
@@ -1308,6 +1309,9 @@ func (b *Builder) vet(ctx context.Context, a *Action) error {
 	vcfg.VetxOnly = a.VetxOnly
 	vcfg.VetxOutput = a.Objdir + "vet.out"
 	vcfg.Stdout = a.Objdir + "vet.stdout"
+	if a.needFix {
+		vcfg.FixArchive = a.Objdir + "vet.fix.zip"
+	}
 	vcfg.PackageVetx = make(map[string]string)
 
 	h := cache.NewHash("vet " + a.Package.ImportPath)
@@ -1368,31 +1372,60 @@ func (b *Builder) vet(ctx context.Context, a *Action) error {
 			vcfg.PackageVetx[a1.Package.ImportPath] = a1.built
 		}
 	}
-	vetxKey := cache.ActionID(h.Sum()) // for .vetx file
-
-	fmt.Fprintf(h, "stdout\n")
-	stdoutKey := cache.ActionID(h.Sum()) // for .stdout file
+	var (
+		id            = cache.ActionID(h.Sum())     // for .vetx file
+		stdoutKey     = cache.Subkey(id, "stdout")  // for .stdout file
+		fixArchiveKey = cache.Subkey(id, "fix.zip") // for .fix.zip file
+	)
 
 	// Check the cache; -a forces a rebuild.
 	if !cfg.BuildA {
 		c := cache.Default()
-		if vcfg.VetxOnly {
-			if file, _, err := cache.GetFile(c, vetxKey); err == nil {
-				a.built = file
-				return nil
+
+		// There may be multiple artifacts in the cache.
+		// We need to retrieve them all, or none:
+		// the effect must be transactional.
+		var (
+			vetxFile   string                           // name of cached .vetx file
+			fixArchive string                           // name of cached .fix.zip file
+			stdout     io.Reader = bytes.NewReader(nil) // cached stdout stream
+		)
+
+		// Obtain location of cached .vetx file.
+		vetxFile, _, err := cache.GetFile(c, id)
+		if err != nil {
+			goto cachemiss
+		}
+
+		// Obtain location of cached .fix.zip file (if needed).
+		if a.needFix {
+			file, _, err := cache.GetFile(c, fixArchiveKey)
+			if err != nil {
+				goto cachemiss
 			}
-		} else {
-			// Copy cached vet.std files to stdout.
-			if file, _, err := cache.GetFile(c, stdoutKey); err == nil {
-				f, err := os.Open(file)
-				if err != nil {
-					return err
-				}
-				defer f.Close() // ignore error (can't fail)
-				return VetHandleStdout(f)
+			fixArchive = file
+		}
+
+		// Copy cached .stdout file to stdout.
+		if file, _, err := cache.GetFile(c, stdoutKey); err == nil {
+			f, err := os.Open(file)
+			if err != nil {
+				goto cachemiss
 			}
+			defer f.Close() // ignore error (can't fail)
+			stdout = f
+		}
+
+		// Cache hit: commit transaction.
+		a.built = vetxFile
+		a.FixArchive = fixArchive
+		if err := VetHandleStdout(stdout); err != nil {
+			return err // internal error (don't fall through to cachemiss)
 		}
+
+		return nil
 	}
+cachemiss:
 
 	js, err := json.MarshalIndent(vcfg, "", "\t")
 	if err != nil {
@@ -1419,13 +1452,23 @@ func (b *Builder) vet(ctx context.Context, a *Action) error {
 		return err
 	}
 
-	// Vet tool succeeded, possibly with facts and JSON stdout. Save both in cache.
+	// Vet tool succeeded, possibly with facts, fixes, or JSON stdout.
+	// Save all in cache.
 
-	// Save facts
+	// Save facts.
 	if f, err := os.Open(vcfg.VetxOutput); err == nil {
 		defer f.Close() // ignore error
 		a.built = vcfg.VetxOutput
-		cache.Default().Put(vetxKey, f) // ignore error
+		cache.Default().Put(id, f) // ignore error
+	}
+
+	// Save fix archive (if any).
+	if a.needFix {
+		if f, err := os.Open(vcfg.FixArchive); err == nil {
+			defer f.Close() // ignore error
+			a.FixArchive = vcfg.FixArchive
+			cache.Default().Put(fixArchiveKey, f) // ignore error
+		}
 	}
 
 	// Save stdout.

src/cmd/go/testdata/script/vet_basic.txt

@@ -91,6 +91,7 @@ stderr '-json and -diff cannot be used together'
 # Legacy way of selecting fixers is a no-op.
 go fix -fix=old1,old2 example.com/x
 stderr 'go fix: the -fix=old1,old2 flag is obsolete and has no effect'
+cp x.go.bak x.go
 
 # -c=n flag shows n lines of context.
 ! go vet -c=2 -printf example.com/x