cmd/go: refactor -mod flag parsing

Jay Conrod · Jay Conrod · commit 6e3df749b105 · 2020-09-11T14:22:17.000Z
Keep track of whether the -mod flag was set explicitly. When -mod=readonly is the default, we'll want to adjust our error messages if it's set explicitly. Also, register the -mod, -modcacherw, and -modfile flags in functions in internal/base instead of internal/work. 'go mod' commands that don't load packages shouldn't depend on internal/work. For #40728 Change-Id: I272aea9e19908ba37e151baac4ea8630e90f241f Reviewed-on: https://go-review.googlesource.com/c/go/+/253744 Run-TryBot: Jay Conrod <jayconrod@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Michael Matloob <matloob@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com>
diff --git a/src/cmd/go/internal/base/flag.go b/src/cmd/go/internal/base/flag.go
@@ -28,13 +28,42 @@ func (v *StringsFlag) String() string {
 	return "<StringsFlag>"
 }
 
+// explicitStringFlag is like a regular string flag, but it also tracks whether
+// the string was set explicitly to a non-empty value.
+type explicitStringFlag struct {
+	value    *string
+	explicit *bool
+}
+
+func (f explicitStringFlag) String() string {
+	if f.value == nil {
+		return ""
+	}
+	return *f.value
+}
+
+func (f explicitStringFlag) Set(v string) error {
+	*f.value = v
+	if v != "" {
+		*f.explicit = true
+	}
+	return nil
+}
+
 // AddBuildFlagsNX adds the -n and -x build flags to the flag set.
 func AddBuildFlagsNX(flags *flag.FlagSet) {
 	flags.BoolVar(&cfg.BuildN, "n", false, "")
 	flags.BoolVar(&cfg.BuildX, "x", false, "")
 }
 
-// AddLoadFlags adds the -mod build flag to the flag set.
-func AddLoadFlags(flags *flag.FlagSet) {
-	flags.StringVar(&cfg.BuildMod, "mod", "", "")
+// AddModFlag adds the -mod build flag to the flag set.
+func AddModFlag(flags *flag.FlagSet) {
+	flags.Var(explicitStringFlag{value: &cfg.BuildMod, explicit: &cfg.BuildModExplicit}, "mod", "")
+}
+
+// AddModCommonFlags adds the module-related flags common to build commands
+// and 'go mod' subcommands.
+func AddModCommonFlags(flags *flag.FlagSet) {
+	flags.BoolVar(&cfg.ModCacheRW, "modcacherw", false, "")
+	flags.StringVar(&cfg.ModFile, "modfile", "", "")
 }
diff --git a/src/cmd/go/internal/cfg/cfg.go b/src/cmd/go/internal/cfg/cfg.go
@@ -27,7 +27,8 @@ var (
 	BuildBuildmode         string // -buildmode flag
 	BuildContext           = defaultContext()
 	BuildMod               string             // -mod flag
-	BuildModReason         string             // reason -mod flag is set, if set by default
+	BuildModExplicit       bool               // whether -mod was set explicitly
+	BuildModReason         string             // reason -mod was set, if set by default
 	BuildI                 bool               // -i flag
 	BuildLinkshared        bool               // -linkshared flag
 	BuildMSan              bool               // -msan flag
diff --git a/src/cmd/go/internal/fmtcmd/fmt.go b/src/cmd/go/internal/fmtcmd/fmt.go
@@ -23,7 +23,8 @@ import (
 
 func init() {
 	base.AddBuildFlagsNX(&CmdFmt.Flag)
-	base.AddLoadFlags(&CmdFmt.Flag)
+	base.AddModFlag(&CmdFmt.Flag)
+	base.AddModCommonFlags(&CmdFmt.Flag)
 }
 
 var CmdFmt = &base.Command{
diff --git a/src/cmd/go/internal/modcmd/download.go b/src/cmd/go/internal/modcmd/download.go
@@ -14,7 +14,6 @@ import (
 	"cmd/go/internal/cfg"
 	"cmd/go/internal/modfetch"
 	"cmd/go/internal/modload"
-	"cmd/go/internal/work"
 
 	"golang.org/x/mod/module"
 )
@@ -64,7 +63,7 @@ func init() {
 
 	// TODO(jayconrod): https://golang.org/issue/35849 Apply -x to other 'go mod' commands.
 	cmdDownload.Flag.BoolVar(&cfg.BuildX, "x", false, "")
-	work.AddModCommonFlags(cmdDownload)
+	base.AddModCommonFlags(&cmdDownload.Flag)
 }
 
 type moduleJSON struct {
diff --git a/src/cmd/go/internal/modcmd/edit.go b/src/cmd/go/internal/modcmd/edit.go
@@ -19,7 +19,6 @@ import (
 	"cmd/go/internal/lockedfile"
 	"cmd/go/internal/modfetch"
 	"cmd/go/internal/modload"
-	"cmd/go/internal/work"
 
 	"golang.org/x/mod/modfile"
 	"golang.org/x/mod/module"
@@ -154,7 +153,7 @@ func init() {
 	cmdEdit.Flag.Var(flagFunc(flagRetract), "retract", "")
 	cmdEdit.Flag.Var(flagFunc(flagDropRetract), "dropretract", "")
 
-	work.AddModCommonFlags(cmdEdit)
+	base.AddModCommonFlags(&cmdEdit.Flag)
 	base.AddBuildFlagsNX(&cmdEdit.Flag)
 }
 
diff --git a/src/cmd/go/internal/modcmd/graph.go b/src/cmd/go/internal/modcmd/graph.go
@@ -15,7 +15,6 @@ import (
 	"cmd/go/internal/base"
 	"cmd/go/internal/cfg"
 	"cmd/go/internal/modload"
-	"cmd/go/internal/work"
 
 	"golang.org/x/mod/module"
 )
@@ -33,7 +32,7 @@ path@version, except for the main module, which has no @version suffix.
 }
 
 func init() {
-	work.AddModCommonFlags(cmdGraph)
+	base.AddModCommonFlags(&cmdGraph.Flag)
 }
 
 func runGraph(ctx context.Context, cmd *base.Command, args []string) {
diff --git a/src/cmd/go/internal/modcmd/init.go b/src/cmd/go/internal/modcmd/init.go
@@ -9,7 +9,6 @@ package modcmd
 import (
 	"cmd/go/internal/base"
 	"cmd/go/internal/modload"
-	"cmd/go/internal/work"
 	"context"
 	"os"
 	"strings"
@@ -30,7 +29,7 @@ To override this guess, supply the module path as an argument.
 }
 
 func init() {
-	work.AddModCommonFlags(cmdInit)
+	base.AddModCommonFlags(&cmdInit.Flag)
 }
 
 func runInit(ctx context.Context, cmd *base.Command, args []string) {
diff --git a/src/cmd/go/internal/modcmd/tidy.go b/src/cmd/go/internal/modcmd/tidy.go
@@ -10,7 +10,6 @@ import (
 	"cmd/go/internal/base"
 	"cmd/go/internal/cfg"
 	"cmd/go/internal/modload"
-	"cmd/go/internal/work"
 	"context"
 )
 
@@ -32,7 +31,7 @@ to standard error.
 func init() {
 	cmdTidy.Run = runTidy // break init cycle
 	cmdTidy.Flag.BoolVar(&cfg.BuildV, "v", false, "")
-	work.AddModCommonFlags(cmdTidy)
+	base.AddModCommonFlags(&cmdTidy.Flag)
 }
 
 func runTidy(ctx context.Context, cmd *base.Command, args []string) {
diff --git a/src/cmd/go/internal/modcmd/vendor.go b/src/cmd/go/internal/modcmd/vendor.go
@@ -19,7 +19,6 @@ import (
 	"cmd/go/internal/cfg"
 	"cmd/go/internal/imports"
 	"cmd/go/internal/modload"
-	"cmd/go/internal/work"
 
 	"golang.org/x/mod/module"
 	"golang.org/x/mod/semver"
@@ -41,7 +40,7 @@ modules and packages to standard error.
 
 func init() {
 	cmdVendor.Flag.BoolVar(&cfg.BuildV, "v", false, "")
-	work.AddModCommonFlags(cmdVendor)
+	base.AddModCommonFlags(&cmdVendor.Flag)
 }
 
 func runVendor(ctx context.Context, cmd *base.Command, args []string) {
diff --git a/src/cmd/go/internal/modcmd/verify.go b/src/cmd/go/internal/modcmd/verify.go
@@ -17,7 +17,6 @@ import (
 	"cmd/go/internal/cfg"
 	"cmd/go/internal/modfetch"
 	"cmd/go/internal/modload"
-	"cmd/go/internal/work"
 
 	"golang.org/x/mod/module"
 	"golang.org/x/mod/sumdb/dirhash"
@@ -38,7 +37,7 @@ non-zero status.
 }
 
 func init() {
-	work.AddModCommonFlags(cmdVerify)
+	base.AddModCommonFlags(&cmdVerify.Flag)
 }
 
 func runVerify(ctx context.Context, cmd *base.Command, args []string) {
diff --git a/src/cmd/go/internal/modcmd/why.go b/src/cmd/go/internal/modcmd/why.go
@@ -11,7 +11,6 @@ import (
 
 	"cmd/go/internal/base"
 	"cmd/go/internal/modload"
-	"cmd/go/internal/work"
 
 	"golang.org/x/mod/module"
 )
@@ -58,7 +57,7 @@ var (
 
 func init() {
 	cmdWhy.Run = runWhy // break init cycle
-	work.AddModCommonFlags(cmdWhy)
+	base.AddModCommonFlags(&cmdWhy.Flag)
 }
 
 func runWhy(ctx context.Context, cmd *base.Command, args []string) {
diff --git a/src/cmd/go/internal/modload/init.go b/src/cmd/go/internal/modload/init.go
@@ -518,17 +518,20 @@ func modFileToBuildList() {
 // setDefaultBuildMod sets a default value for cfg.BuildMod
 // if it is currently empty.
 func setDefaultBuildMod() {
-	if cfg.BuildMod != "" {
+	if cfg.BuildModExplicit {
 		// Don't override an explicit '-mod=' argument.
 		return
 	}
-	cfg.BuildMod = "mod"
+
 	if cfg.CmdName == "get" || strings.HasPrefix(cfg.CmdName, "mod ") {
-		// Don't set -mod implicitly for commands whose purpose is to
-		// manipulate the build list.
+		// 'get' and 'go mod' commands may update go.mod automatically.
+		// TODO(jayconrod): should this narrower? Should 'go mod download' or
+		// 'go mod graph' update go.mod by default?
+		cfg.BuildMod = "mod"
 		return
 	}
 	if modRoot == "" {
+		cfg.BuildMod = "mod"
 		return
 	}
 
@@ -546,18 +549,18 @@ func setDefaultBuildMod() {
 			}
 		}
 
-		// Since a vendor directory exists, we have a non-trivial reason for
-		// choosing -mod=mod, although it probably won't be used for anything.
-		// Record the reason anyway for consistency.
-		// It may be overridden if we switch to mod=readonly below.
-		cfg.BuildModReason = fmt.Sprintf("Go version in go.mod is %s.", modGo)
+		// Since a vendor directory exists, we should record why we didn't use it.
+		// This message won't normally be shown, but it may appear with import errors.
+		cfg.BuildModReason = fmt.Sprintf("Go version in go.mod is %s, so vendor directory was not used.", modGo)
 	}
 
 	p := ModFilePath()
 	if fi, err := os.Stat(p); err == nil && !hasWritePerm(p, fi) {
 		cfg.BuildMod = "readonly"
 		cfg.BuildModReason = "go.mod file is read-only."
+		return
 	}
+	cfg.BuildMod = "mod"
 }
 
 func legacyModInit() {
@@ -857,7 +860,7 @@ func WriteGoMod() {
 		// prefer to report a dirty go.mod over a dirty go.sum
 		if cfg.BuildModReason != "" {
 			base.Fatalf("go: updates to go.mod needed, disabled by -mod=readonly\n\t(%s)", cfg.BuildModReason)
-		} else {
+		} else if cfg.BuildModExplicit {
 			base.Fatalf("go: updates to go.mod needed, disabled by -mod=readonly")
 		}
 	}
diff --git a/src/cmd/go/internal/work/build.go b/src/cmd/go/internal/work/build.go
@@ -240,24 +240,23 @@ const (
 // AddBuildFlags adds the flags common to the build, clean, get,
 // install, list, run, and test commands.
 func AddBuildFlags(cmd *base.Command, mask BuildFlagMask) {
+	base.AddBuildFlagsNX(&cmd.Flag)
 	cmd.Flag.BoolVar(&cfg.BuildA, "a", false, "")
-	cmd.Flag.BoolVar(&cfg.BuildN, "n", false, "")
 	cmd.Flag.IntVar(&cfg.BuildP, "p", cfg.BuildP, "")
 	if mask&OmitVFlag == 0 {
 		cmd.Flag.BoolVar(&cfg.BuildV, "v", false, "")
 	}
-	cmd.Flag.BoolVar(&cfg.BuildX, "x", false, "")
 
 	cmd.Flag.Var(&load.BuildAsmflags, "asmflags", "")
 	cmd.Flag.Var(buildCompiler{}, "compiler", "")
 	cmd.Flag.StringVar(&cfg.BuildBuildmode, "buildmode", "default", "")
 	cmd.Flag.Var(&load.BuildGcflags, "gcflags", "")
 	cmd.Flag.Var(&load.BuildGccgoflags, "gccgoflags", "")
 	if mask&OmitModFlag == 0 {
-		cmd.Flag.StringVar(&cfg.BuildMod, "mod", "", "")
+		base.AddModFlag(&cmd.Flag)
 	}
 	if mask&OmitModCommonFlags == 0 {
-		AddModCommonFlags(cmd)
+		base.AddModCommonFlags(&cmd.Flag)
 	}
 	cmd.Flag.StringVar(&cfg.BuildContext.InstallSuffix, "installsuffix", "", "")
 	cmd.Flag.Var(&load.BuildLdflags, "ldflags", "")
@@ -275,13 +274,6 @@ func AddBuildFlags(cmd *base.Command, mask BuildFlagMask) {
 	cmd.Flag.StringVar(&cfg.DebugTrace, "debug-trace", "", "")
 }
 
-// AddModCommonFlags adds the module-related flags common to build commands
-// and 'go mod' subcommands.
-func AddModCommonFlags(cmd *base.Command) {
-	cmd.Flag.BoolVar(&cfg.ModCacheRW, "modcacherw", false, "")
-	cmd.Flag.StringVar(&cfg.ModFile, "modfile", "", "")
-}
-
 // tagsFlag is the implementation of the -tags flag.
 type tagsFlag []string
 
diff --git a/src/cmd/go/internal/work/init.go b/src/cmd/go/internal/work/init.go
@@ -252,7 +252,7 @@ func buildModeInit() {
 
 	switch cfg.BuildMod {
 	case "":
-		// ok
+		// Behavior will be determined automatically, as if no flag were passed.
 	case "readonly", "vendor", "mod":
 		if !cfg.ModulesEnabled && !inGOFLAGS("-mod") {
 			base.Fatalf("build flag -mod=%s only valid when using modules", cfg.BuildMod)

Original file line number	Diff line number	Diff line change
`@@ -23,7 +23,8 @@ import (`
`23`	`23`
`24`	`24`	`func init() {`
`25`	`25`	`base.AddBuildFlagsNX(&CmdFmt.Flag)`
`26`		`- base.AddLoadFlags(&CmdFmt.Flag)`
	`26`	`+ base.AddModFlag(&CmdFmt.Flag)`
	`27`	`+ base.AddModCommonFlags(&CmdFmt.Flag)`
`27`	`28`	`}`
`28`	`29`
`29`	`30`	`var CmdFmt = &base.Command{`
Original file line number	Diff line number	Diff line change
`@@ -14,7 +14,6 @@ import (`
`14`	`14`	`"cmd/go/internal/cfg"`
`15`	`15`	`"cmd/go/internal/modfetch"`
`16`	`16`	`"cmd/go/internal/modload"`
`17`		`- "cmd/go/internal/work"`
`18`	`17`
`19`	`18`	`"golang.org/x/mod/module"`
`20`	`19`	`)`
`@@ -64,7 +63,7 @@ func init() {`
`64`	`63`
`65`	`64`	`// TODO(jayconrod): https://golang.org/issue/35849 Apply -x to other 'go mod' commands.`
`66`	`65`	`cmdDownload.Flag.BoolVar(&cfg.BuildX, "x", false, "")`
`67`		`- work.AddModCommonFlags(cmdDownload)`
	`66`	`+ base.AddModCommonFlags(&cmdDownload.Flag)`
`68`	`67`	`}`
`69`	`68`
`70`	`69`	`type moduleJSON struct {`
Original file line number	Diff line number	Diff line change
`@@ -15,7 +15,6 @@ import (`
`15`	`15`	`"cmd/go/internal/base"`
`16`	`16`	`"cmd/go/internal/cfg"`
`17`	`17`	`"cmd/go/internal/modload"`
`18`		`- "cmd/go/internal/work"`
`19`	`18`
`20`	`19`	`"golang.org/x/mod/module"`
`21`	`20`	`)`
`@@ -33,7 +32,7 @@ path@version, except for the main module, which has no @version suffix.`
`33`	`32`	`}`
`34`	`33`
`35`	`34`	`func init() {`
`36`		`- work.AddModCommonFlags(cmdGraph)`
	`35`	`+ base.AddModCommonFlags(&cmdGraph.Flag)`
`37`	`36`	`}`
`38`	`37`
`39`	`38`	`func runGraph(ctx context.Context, cmd *base.Command, args []string) {`
Original file line number	Diff line number	Diff line change
`@@ -9,7 +9,6 @@ package modcmd`
`9`	`9`	`import (`
`10`	`10`	`"cmd/go/internal/base"`
`11`	`11`	`"cmd/go/internal/modload"`
`12`		`- "cmd/go/internal/work"`
`13`	`12`	`"context"`
`14`	`13`	`"os"`
`15`	`14`	`"strings"`
`@@ -30,7 +29,7 @@ To override this guess, supply the module path as an argument.`
`30`	`29`	`}`
`31`	`30`
`32`	`31`	`func init() {`
`33`		`- work.AddModCommonFlags(cmdInit)`
	`32`	`+ base.AddModCommonFlags(&cmdInit.Flag)`
`34`	`33`	`}`
`35`	`34`
`36`	`35`	`func runInit(ctx context.Context, cmd *base.Command, args []string) {`
Original file line number	Diff line number	Diff line change
`@@ -10,7 +10,6 @@ import (`
`10`	`10`	`"cmd/go/internal/base"`
`11`	`11`	`"cmd/go/internal/cfg"`
`12`	`12`	`"cmd/go/internal/modload"`
`13`		`- "cmd/go/internal/work"`
`14`	`13`	`"context"`
`15`	`14`	`)`
`16`	`15`
`@@ -32,7 +31,7 @@ to standard error.`
`32`	`31`	`func init() {`
`33`	`32`	`cmdTidy.Run = runTidy // break init cycle`
`34`	`33`	`cmdTidy.Flag.BoolVar(&cfg.BuildV, "v", false, "")`
`35`		`- work.AddModCommonFlags(cmdTidy)`
	`34`	`+ base.AddModCommonFlags(&cmdTidy.Flag)`
`36`	`35`	`}`
`37`	`36`
`38`	`37`	`func runTidy(ctx context.Context, cmd *base.Command, args []string) {`
Original file line number	Diff line number	Diff line change
`@@ -17,7 +17,6 @@ import (`
`17`	`17`	`"cmd/go/internal/cfg"`
`18`	`18`	`"cmd/go/internal/modfetch"`
`19`	`19`	`"cmd/go/internal/modload"`
`20`		`- "cmd/go/internal/work"`
`21`	`20`
`22`	`21`	`"golang.org/x/mod/module"`
`23`	`22`	`"golang.org/x/mod/sumdb/dirhash"`
`@@ -38,7 +37,7 @@ non-zero status.`
`38`	`37`	`}`
`39`	`38`
`40`	`39`	`func init() {`
`41`		`- work.AddModCommonFlags(cmdVerify)`
	`40`	`+ base.AddModCommonFlags(&cmdVerify.Flag)`
`42`	`41`	`}`
`43`	`42`
`44`	`43`	`func runVerify(ctx context.Context, cmd *base.Command, args []string) {`