cmd/go: refactor {Allow,Disallow}WriteGoMod to ExplicitWriteGoMod

Subcommands may now set the global flag modload.ExplicitWriteGoMod
instead of calling {Allow,Disallow}WriteGoMod.

When ExplicitWriteGoMod is false (default), modload.LoadPackages and
ListModules will either update go.mod and go.sum or report an error if
they need to be updated, depending on cfg.BuildMod.

When ExplicitWriteGoMod is true, commands must explicitly call
modload.WriteGoMod to update go.mod and go.sum or report an
error. Commands that perform some operation after loading the build
list (like downloading zips or building packages) and commands that
load packages multiple times should set this. For now, only 'go get'
and 'go mod download' set this.

This CL is a pure refactor: no change in behavior is expected.
There are some other minor changes in here, too: commitRequirements no
longer sets the global requirements: that should be done separately first.

Change-Id: I69942a808bb177faf7904a53aaf2d4ac68500e82
Reviewed-on: https://go-review.googlesource.com/c/go/+/349600
Trust: Jay Conrod <jayconrod@google.com>
Reviewed-by: Bryan C. Mills <bcmills@google.com>

src/cmd/go/internal/modcmd/download.go

@@ -86,6 +86,7 @@ func runDownload(ctx context.Context, cmd *base.Command, args []string) {
 
 	// Check whether modules are enabled and whether we're in a module.
 	modload.ForceUseModules = true
+	modload.ExplicitWriteGoMod = true
 	if !modload.HasModRoot() && len(args) == 0 {
 		base.Fatalf("go: no modules specified (see 'go help mod download')")
 	}
@@ -149,13 +150,16 @@ func runDownload(ctx context.Context, cmd *base.Command, args []string) {
 	if !haveExplicitArgs {
 		// 'go mod download' is sometimes run without arguments to pre-populate the
 		// module cache. It may fetch modules that aren't needed to build packages
-		// in the main mdoule. This is usually not intended, so don't save sums for
-		// downloaded modules (golang.org/issue/45332).
-		// TODO(golang.org/issue/45551): For now, in ListModules, save sums needed
-		// to load the build list (same as 1.15 behavior). In the future, report an
-		// error if go.mod or go.sum need to be updated after loading the build
-		// list.
-		modload.DisallowWriteGoMod()
+		// in the main module. This is usually not intended, so don't save sums for
+		// downloaded modules (golang.org/issue/45332). We do still fix
+		// inconsistencies in go.mod though.
+		//
+		// TODO(#45551): In the future, report an error if go.mod or go.sum need to
+		// be updated after loading the build list. This may require setting
+		// the mode to "mod" or "readonly" depending on haveExplicitArgs.
+		if err := modload.WriteGoMod(ctx); err != nil {
+			base.Fatalf("go: %v", err)
+		}
 	}
 
 	for _, info := range infos {
@@ -215,7 +219,9 @@ func runDownload(ctx context.Context, cmd *base.Command, args []string) {
 	//
 	// Don't save sums for 'go mod download' without arguments; see comment above.
 	if haveExplicitArgs {
-		modload.WriteGoMod(ctx)
+		if err := modload.WriteGoMod(ctx); err != nil {
+			base.Errorf("go: %v", err)
+		}
 	}
 
 	// If there was an error matching some of the requested packages, emit it now

src/cmd/go/internal/modcmd/why.go

@@ -68,6 +68,7 @@ func runWhy(ctx context.Context, cmd *base.Command, args []string) {
 	modload.InitWorkfile()
 	modload.ForceUseModules = true
 	modload.RootMode = modload.NeedRoot
+	modload.ExplicitWriteGoMod = true // don't write go.mod in ListModules
 
 	loadOpts := modload.PackageOpts{
 		Tags:                     imports.AnyTags(),

src/cmd/go/internal/modget/get.go

@@ -281,7 +281,7 @@ func runGet(ctx context.Context, cmd *base.Command, args []string) {
 	// Do not allow any updating of go.mod until we've applied
 	// all the requested changes and checked that the result matches
 	// what was requested.
-	modload.DisallowWriteGoMod()
+	modload.ExplicitWriteGoMod = true
 
 	// Allow looking up modules for import paths when outside of a module.
 	// 'go get' is expected to do this, unlike other commands.
@@ -423,9 +423,9 @@ func runGet(ctx context.Context, cmd *base.Command, args []string) {
 	// Everything succeeded. Update go.mod.
 	oldReqs := reqsFromGoMod(modload.ModFile())
 
-	modload.AllowWriteGoMod()
-	modload.WriteGoMod(ctx)
-	modload.DisallowWriteGoMod()
+	if err := modload.WriteGoMod(ctx); err != nil {
+		base.Fatalf("go: %v", err)
+	}
 
 	newReqs := reqsFromGoMod(modload.ModFile())
 	r.reportChanges(oldReqs, newReqs)

src/cmd/go/internal/modload/buildlist.go

@@ -469,7 +469,8 @@ func LoadModGraph(ctx context.Context, goVersion string) *ModuleGraph {
 		base.Fatalf("go: %v", err)
 	}
 
-	commitRequirements(ctx, rs)
+	requirements = rs
+
 	return mg
 }
 
@@ -534,7 +535,7 @@ func EditBuildList(ctx context.Context, add, mustSelect []module.Version) (chang
 	if err != nil {
 		return false, err
 	}
-	commitRequirements(ctx, rs)
+	requirements = rs
 	return changed, err
 }
 

src/cmd/go/internal/modload/init.go

@@ -44,6 +44,16 @@ var (
 	ForceUseModules bool
 
 	allowMissingModuleImports bool
+
+	// ExplicitWriteGoMod prevents LoadPackages, ListModules, and other functions
+	// from updating go.mod and go.sum or reporting errors when updates are
+	// needed. A package should set this if it would cause go.mod to be written
+	// multiple times (for example, 'go get' calls LoadPackages multiple times) or
+	// if it needs some other operation to be successful before go.mod and go.sum
+	// can be written (for example, 'go mod download' must download modules before
+	// adding sums to go.sum). Packages that set this are responsible for calling
+	// WriteGoMod explicitly.
+	ExplicitWriteGoMod bool
 )
 
 func TODOWorkspaces(s string) error {
@@ -591,8 +601,9 @@ func loadWorkFile(path string) (goVersion string, modRoots []string, err error)
 // build list from its go.mod file.
 //
 // LoadModFile may make changes in memory, like adding a go directive and
-// ensuring requirements are consistent, and will write those changes back to
-// disk unless DisallowWriteGoMod is in effect.
+// ensuring requirements are consistent. The caller is responsible for ensuring
+// those changes are written to disk by calling LoadPackages or ListModules
+// (unless ExplicitWriteGoMod is set) or by calling WriteGoMod directly.
 //
 // As a side-effect, LoadModFile may change cfg.BuildMod to "vendor" if
 // -mod wasn't set explicitly and automatic vendoring should be enabled.
@@ -605,22 +616,8 @@ func loadWorkFile(path string) (goVersion string, modRoots []string, err error)
 // it for global consistency. Most callers outside of the modload package should
 // use LoadModGraph instead.
 func LoadModFile(ctx context.Context) *Requirements {
-	rs, needCommit := loadModFile(ctx)
-	if needCommit {
-		commitRequirements(ctx, rs)
-	}
-	return rs
-}
-
-// loadModFile is like LoadModFile, but does not implicitly commit the
-// requirements back to disk after fixing inconsistencies.
-//
-// If needCommit is true, after the caller makes any other needed changes to the
-// returned requirements they should invoke commitRequirements to fix any
-// inconsistencies that may be present in the on-disk go.mod file.
-func loadModFile(ctx context.Context) (rs *Requirements, needCommit bool) {
 	if requirements != nil {
-		return requirements, false
+		return requirements
 	}
 
 	Init()
@@ -631,7 +628,7 @@ func loadModFile(ctx context.Context) (rs *Requirements, needCommit bool) {
 		goVersion := LatestGoVersion()
 		rawGoVersion.Store(mainModule, goVersion)
 		requirements = newRequirements(pruningForGoVersion(goVersion), nil, nil)
-		return requirements, false
+		return requirements
 	}
 
 	var modFiles []*modfile.File
@@ -660,13 +657,13 @@ func loadModFile(ctx context.Context) (rs *Requirements, needCommit bool) {
 
 	MainModules = makeMainModules(mainModules, modRoots, modFiles, indices, workFileGoVersion)
 	setDefaultBuildMod() // possibly enable automatic vendoring
-	rs = requirementsFromModFiles(ctx, modFiles)
+	rs := requirementsFromModFiles(ctx, modFiles)
 
 	if inWorkspaceMode() {
 		// We don't need to do anything for vendor or update the mod file so
 		// return early.
-
-		return rs, true
+		requirements = rs
+		return rs
 	}
 
 	mainModule := MainModules.mustGetSingleMainModule()
@@ -712,7 +709,7 @@ func loadModFile(ctx context.Context) (rs *Requirements, needCommit bool) {
 	}
 
 	requirements = rs
-	return requirements, true
+	return requirements
 }
 
 // CreateModFile initializes a new module by creating a go.mod file.
@@ -777,7 +774,10 @@ func CreateModFile(ctx context.Context, modPath string) {
 	if err != nil {
 		base.Fatalf("go: %v", err)
 	}
-	commitRequirements(ctx, rs)
+	requirements = rs
+	if err := commitRequirements(ctx); err != nil {
+		base.Fatalf("go: %v", err)
+	}
 
 	// Suggest running 'go mod tidy' unless the project is empty. Even if we
 	// imported all the correct requirements above, we're probably missing
@@ -1003,7 +1003,6 @@ func setDefaultBuildMod() {
 		// go.mod is inconsistent. They're useful for debugging, and they need
 		// to work in buggy situations.
 		cfg.BuildMod = "mod"
-		allowWriteGoMod = false
 		return
 	case "mod vendor":
 		cfg.BuildMod = "readonly"
@@ -1309,62 +1308,44 @@ func findImportComment(file string) string {
 	return path
 }
 
-var allowWriteGoMod = true
-
-// DisallowWriteGoMod causes future calls to WriteGoMod to do nothing at all.
-func DisallowWriteGoMod() {
-	allowWriteGoMod = false
-}
-
-// AllowWriteGoMod undoes the effect of DisallowWriteGoMod:
-// future calls to WriteGoMod will update go.mod if needed.
-// Note that any past calls have been discarded, so typically
-// a call to AlowWriteGoMod should be followed by a call to WriteGoMod.
-func AllowWriteGoMod() {
-	allowWriteGoMod = true
-}
-
 // WriteGoMod writes the current build list back to go.mod.
-func WriteGoMod(ctx context.Context) {
-	if !allowWriteGoMod {
-		panic("WriteGoMod called while disallowed")
-	}
-	commitRequirements(ctx, LoadModFile(ctx))
+func WriteGoMod(ctx context.Context) error {
+	requirements = LoadModFile(ctx)
+	return commitRequirements(ctx)
 }
 
-// commitRequirements writes sets the global requirements variable to rs and
-// writes its contents back to the go.mod file on disk.
-// goVersion, if non-empty, is used to set the version on the go.mod file.
-func commitRequirements(ctx context.Context, rs *Requirements) {
-	requirements = rs
-
-	if !allowWriteGoMod {
-		// Some package outside of modload promised to update the go.mod file later.
-		return
-	}
-
+// commitRequirements ensures go.mod and go.sum are up to date with the current
+// requirements.
+//
+// In "mod" mode, commitRequirements writes changes to go.mod and go.sum.
+//
+// In "readonly" and "vendor" modes, commitRequirements returns an error if
+// go.mod or go.sum are out of date in a semantically significant way.
+//
+// In workspace mode, commitRequirements only writes changes to go.work.sum.
+func commitRequirements(ctx context.Context) (err error) {
 	if inWorkspaceMode() {
 		// go.mod files aren't updated in workspace mode, but we still want to
 		// update the go.work.sum file.
-		if err := modfetch.WriteGoSum(keepSums(ctx, loaded, rs, addBuildListZipSums), mustHaveCompleteRequirements()); err != nil {
-			base.Fatalf("go: %v", err)
-		}
-		return
+		return modfetch.WriteGoSum(keepSums(ctx, loaded, requirements, addBuildListZipSums), mustHaveCompleteRequirements())
 	}
-
 	if MainModules.Len() != 1 || MainModules.ModRoot(MainModules.Versions()[0]) == "" {
 		// We aren't in a module, so we don't have anywhere to write a go.mod file.
-		return
+		return nil
 	}
 	mainModule := MainModules.mustGetSingleMainModule()
-	modFilePath := modFilePath(MainModules.ModRoot(mainModule))
 	modFile := MainModules.ModFile(mainModule)
+	if modFile == nil {
+		// command-line-arguments has no .mod file to write.
+		return nil
+	}
+	modFilePath := modFilePath(MainModules.ModRoot(mainModule))
 
 	var list []*modfile.Require
-	for _, m := range rs.rootModules {
+	for _, m := range requirements.rootModules {
 		list = append(list, &modfile.Require{
 			Mod:      m,
-			Indirect: !rs.direct[m.Path],
+			Indirect: !requirements.direct[m.Path],
 		})
 	}
 	if modFile.Go == nil || modFile.Go.Version == "" {
@@ -1382,7 +1363,7 @@ func commitRequirements(ctx context.Context, rs *Requirements) {
 	if dirty && cfg.BuildMod != "mod" {
 		// If we're about to fail due to -mod=readonly,
 		// prefer to report a dirty go.mod over a dirty go.sum
-		base.Fatalf("go: %v", errGoModDirty)
+		return errGoModDirty
 	}
 
 	if !dirty && cfg.CmdName != "mod tidy" {
@@ -1391,22 +1372,22 @@ func commitRequirements(ctx context.Context, rs *Requirements) {
 		// Don't write go.mod, but write go.sum in case we added or trimmed sums.
 		// 'go mod init' shouldn't write go.sum, since it will be incomplete.
 		if cfg.CmdName != "mod init" {
-			if err := modfetch.WriteGoSum(keepSums(ctx, loaded, rs, addBuildListZipSums), mustHaveCompleteRequirements()); err != nil {
-				base.Fatalf("go: %v", err)
+			if err := modfetch.WriteGoSum(keepSums(ctx, loaded, requirements, addBuildListZipSums), mustHaveCompleteRequirements()); err != nil {
+				return err
 			}
 		}
-		return
+		return nil
 	}
 	if _, ok := fsys.OverlayPath(modFilePath); ok {
 		if dirty {
-			base.Fatalf("go: updates to go.mod needed, but go.mod is part of the overlay specified with -overlay")
+			return errors.New("updates to go.mod needed, but go.mod is part of the overlay specified with -overlay")
 		}
-		return
+		return nil
 	}
 
 	new, err := modFile.Format()
 	if err != nil {
-		base.Fatalf("go: %v", err)
+		return err
 	}
 	defer func() {
 		// At this point we have determined to make the go.mod file on disk equal to new.
@@ -1415,8 +1396,8 @@ func commitRequirements(ctx context.Context, rs *Requirements) {
 		// Update go.sum after releasing the side lock and refreshing the index.
 		// 'go mod init' shouldn't write go.sum, since it will be incomplete.
 		if cfg.CmdName != "mod init" {
-			if err := modfetch.WriteGoSum(keepSums(ctx, loaded, rs, addBuildListZipSums), mustHaveCompleteRequirements()); err != nil {
-				base.Fatalf("go: %v", err)
+			if err == nil {
+				err = modfetch.WriteGoSum(keepSums(ctx, loaded, requirements, addBuildListZipSums), mustHaveCompleteRequirements())
 			}
 		}
 	}()
@@ -1450,8 +1431,9 @@ func commitRequirements(ctx context.Context, rs *Requirements) {
 	})
 
 	if err != nil && err != errNoChange {
-		base.Fatalf("go: updating go.mod: %v", err)
+		return fmt.Errorf("updating go.mod: %w", err)
 	}
+	return nil
 }
 
 // keepSums returns the set of modules (and go.mod file entries) for which

src/cmd/go/internal/modload/list.go

@@ -72,7 +72,10 @@ func ListModules(ctx context.Context, args []string, mode ListMode) ([]*modinfo.
 	}
 
 	if err == nil {
-		commitRequirements(ctx, rs)
+		requirements = rs
+		if !ExplicitWriteGoMod {
+			err = commitRequirements(ctx)
+		}
 	}
 	return mods, err
 }

src/cmd/go/internal/modload/load.go

@@ -327,7 +327,7 @@ func LoadPackages(ctx context.Context, opts PackageOpts, patterns ...string) (ma
 		}
 	}
 
-	initialRS, _ := loadModFile(ctx) // Ignore needCommit — we're going to commit at the end regardless.
+	initialRS := LoadModFile(ctx)
 
 	ld := loadFromRoots(ctx, loaderParams{
 		PackageOpts:  opts,
@@ -396,7 +396,7 @@ func LoadPackages(ctx context.Context, opts PackageOpts, patterns ...string) (ma
 			}
 		}
 
-		if allowWriteGoMod {
+		if !ExplicitWriteGoMod {
 			modfetch.TrimGoSum(keep)
 
 			// commitRequirements below will also call WriteGoSum, but the "keep" map
@@ -417,8 +417,11 @@ func LoadPackages(ctx context.Context, opts PackageOpts, patterns ...string) (ma
 	}
 
 	// Success! Update go.mod and go.sum (if needed) and return the results.
+	// We'll skip updating if ExplicitWriteGoMod is true (the caller has opted
+	// to call WriteGoMod itself) or if ResolveMissingImports is false (the
+	// command wants to examine the package graph as-is).
 	loaded = ld
-	commitRequirements(ctx, loaded.requirements)
+	requirements = loaded.requirements
 
 	for _, pkg := range ld.pkgs {
 		if !pkg.isTest() {
@@ -426,6 +429,13 @@ func LoadPackages(ctx context.Context, opts PackageOpts, patterns ...string) (ma
 		}
 	}
 	sort.Strings(loadedPackages)
+
+	if !ExplicitWriteGoMod && opts.ResolveMissingImports {
+		if err := commitRequirements(ctx); err != nil {
+			base.Fatalf("go: %v", err)
+		}
+	}
+
 	return matches, loadedPackages
 }
 
@@ -685,7 +695,7 @@ func ImportFromFiles(ctx context.Context, gofiles []string) {
 			return roots
 		},
 	})
-	commitRequirements(ctx, loaded.requirements)
+	requirements = loaded.requirements
 }
 
 // DirImportPath returns the effective import path for dir,