flag: disallow setting flags multiple times

This is a day 1 error in the flag package: It did not check
that a flag was set at most once on the command line.
Because user-defined flags may have more general
properties, the check applies only to the standard flag
types in this package: bool, string, etc.

Fixes issue 8960.

LGTM=bradfitz
R=golang-codereviews, bradfitz
CC=golang-codereviews
https://codereview.appspot.com/156390043

2 files changed, 90 insertions, 0 deletions

diff --git a/src/flag/flag.go b/src/flag/flag.go
index 323e452a8..56860fc9d 100644
--- a/src/flag/flag.go
+++ b/src/flag/flag.go
@@ -235,6 +235,9 @@ func (d *durationValue) String() string { return (*time.Duration)(d).String() }
 // If a Value has an IsBoolFlag() bool method returning true,
 // the command-line parser makes -name equivalent to -name=true
 // rather than using the next command-line argument.
+//
+// It is the implementer's responsibility to verify, if required, that the
+// flag has been set only once.
 type Value interface {
 	String() string
 	Set(string) error
@@ -270,6 +273,7 @@ type FlagSet struct {
 	parsed        bool
 	actual        map[string]*Flag
 	formal        map[string]*Flag
+	set           map[*Flag]bool
 	args          []string // arguments after flags
 	errorHandling ErrorHandling
 	output        io.Writer // nil means stderr; use out() accessor
@@ -717,6 +721,25 @@ func (f *FlagSet) usage() {
 	}
 }
 
+// alreadySet reports whether the flag has already been set during parsing.
+// Because user-defined flags may legally be set multiple times, it only
+// complains about flags defined in this package.
+func (f *FlagSet) alreadySet(flag *Flag) bool {
+	if f.set == nil {
+		f.set = make(map[*Flag]bool)
+	}
+	switch flag.Value.(type) {
+	case *boolValue, *intValue, *int64Value, *uintValue, *uint64Value, *stringValue, *float64Value, *durationValue:
+	default:
+		return false // Not one of ours.
+	}
+	if f.set[flag] {
+		return true
+	}
+	f.set[flag] = true
+	return false
+}
+
 // parseOne parses one flag. It reports whether a flag was seen.
 func (f *FlagSet) parseOne() (bool, error) {
 	if len(f.args) == 0 {
@@ -760,6 +783,12 @@ func (f *FlagSet) parseOne() (bool, error) {
 		}
 		return false, f.failf("flag provided but not defined: -%s", name)
 	}
+
+	// has it already been set?
+	if f.alreadySet(flag) {
+		return false, f.failf("flag set multiple times: -%s", name)
+	}
+
 	if fv, ok := flag.Value.(boolFlag); ok && fv.IsBoolFlag() { // special case: doesn't need an arg
 		if has_value {
 			if err := fv.Set(value); err != nil {
@@ -795,6 +824,7 @@ func (f *FlagSet) parseOne() (bool, error) {
 // The return value will be ErrHelp if -help or -h were set but not defined.
 func (f *FlagSet) Parse(arguments []string) error {
 	f.parsed = true
+	f.set = nil
 	f.args = arguments
 	for {
 		seen, err := f.parseOne()
diff --git a/src/flag/flag_test.go b/src/flag/flag_test.go
index 8c88c8c27..80d92a5e7 100644
--- a/src/flag/flag_test.go
+++ b/src/flag/flag_test.go
@@ -377,3 +377,63 @@ func TestHelp(t *testing.T) {
 		t.Fatal("help was called; should not have been for defined help flag")
 	}
 }
+
+// Test that a standard flag can be set only once. Need to verify every flag type.
+// User-defined flags can be set multiple times, and this is verified in the tests above.
+func TestErrorOnMultipleSettings(t *testing.T) {
+	check := func(typ string, err error) {
+		if err == nil {
+			t.Errorf("%s flag can be set multiple times")
+		} else if !strings.Contains(err.Error(), "flag set multiple") {
+			t.Fatalf("expected multiple setting error, got %q", err)
+		}
+	}
+	{
+		var flags FlagSet
+		flags.Init("test", ContinueOnError)
+		flags.Bool("v", false, "usage")
+		check("bool", flags.Parse([]string{"-v", "-v"}))
+	}
+	{
+		var flags FlagSet
+		flags.Init("test", ContinueOnError)
+		flags.Int("v", 0, "usage")
+		check("int", flags.Parse([]string{"-v", "1", "-v", "2"}))
+	}
+	{
+		var flags FlagSet
+		flags.Init("test", ContinueOnError)
+		flags.Int64("v", 0, "usage")
+		check("int64", flags.Parse([]string{"-v", "1", "-v", "2"}))
+	}
+	{
+		var flags FlagSet
+		flags.Init("test", ContinueOnError)
+		flags.Uint("v", 0, "usage")
+		check("uint", flags.Parse([]string{"-v", "1", "-v", "2"}))
+	}
+	{
+		var flags FlagSet
+		flags.Init("test", ContinueOnError)
+		flags.Uint64("v", 0, "usage")
+		check("uint64", flags.Parse([]string{"-v", "1", "-v", "2"}))
+	}
+	{
+		var flags FlagSet
+		flags.Init("test", ContinueOnError)
+		flags.String("v", "", "usage")
+		check("string", flags.Parse([]string{"-v", "1", "-v", "2"}))
+	}
+	{
+		var flags FlagSet
+		flags.Init("test", ContinueOnError)
+		flags.Float64("v", 0, "usage")
+		check("float64", flags.Parse([]string{"-v", "1", "-v", "2"}))
+	}
+	{
+		var flags FlagSet
+		flags.Init("test", ContinueOnError)
+		flags.Duration("v", 0, "usage")
+		check("duration", flags.Parse([]string{"-v", "1s", "-v", "2s"}))
+	}
+}