flag: panic if a flag is defined after being set

As part of developing #57411, we ran into cases where a flag was
defined in one package init and Set in another package init, and there
was no init ordering implied by the spec between those two
packages. Changes in initialization ordering as part of #57411 caused
a Set to happen before the definition, which makes the Set silently
fail.

This CL makes the Set fail loudly in that situation.

Currently Set *does* fail kinda quietly in that situation, in that it
returns an error. (It seems that no one checks the error from Set,
at least for string flags.) Ian suggsted that instead we panic at
the definition site if there was previously a Set called on that
(at the time undefined) flag.

So Set on an undefined flag is ok and returns an error (as before),
but defining a flag which has already been Set causes a panic.  (The
API for flag definition has no way to return an error, and does
already panic in some situations like a duplicate definition.)

Update #57411

Change-Id: I39b5a49006f9469de0b7f3fe092afe3a352e4fcb
Reviewed-on: https://go-review.googlesource.com/c/go/+/480215
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@google.com>

diff --git a/src/flag/flag.go b/src/flag/flag.go
index 45928b01906f26ef281a8bc427a2c80a77a90019..9d3e8d32a5a4074786d325324985dbc318393758 100644 (file)
--- a/src/flag/flag.go
+++ b/src/flag/flag.go
@@ -89,6 +89,7 @@ import (
        "io"
        "os"
        "reflect"
+       "runtime"
        "sort"
        "strconv"
        "strings"
@@ -399,7 +400,8 @@ type FlagSet struct {
        formal        map[string]*Flag
        args          []string // arguments after flags
        errorHandling ErrorHandling
-       output        io.Writer // nil means stderr; use Output() accessor
+       output        io.Writer         // nil means stderr; use Output() accessor
+       undef         map[string]string // flags which didn't exist at the time of Set
 }
 
 // A Flag represents the state of a flag.
@@ -490,8 +492,29 @@ func Lookup(name string) *Flag {
 
 // Set sets the value of the named flag.
 func (f *FlagSet) Set(name, value string) error {
+       return f.set(name, value)
+}
+func (f *FlagSet) set(name, value string) error {
        flag, ok := f.formal[name]
        if !ok {
+               // Remember that a flag that isn't defined is being set.
+               // We return an error in this case, but in addition if
+               // subsequently that flag is defined, we want to panic
+               // at the definition point.
+               // This is a problem which occurs if both the definition
+               // and the Set call are in init code and for whatever
+               // reason the init code changes evaluation order.
+               // See issue 57411.
+               _, file, line, ok := runtime.Caller(2)
+               if !ok {
+                       file = "?"
+                       line = 0
+               }
+               if f.undef == nil {
+                       f.undef = map[string]string{}
+               }
+               f.undef[name] = fmt.Sprintf("%s:%d", file, line)
+
                return fmt.Errorf("no such flag -%v", name)
        }
        err := flag.Value.Set(value)
@@ -507,7 +530,7 @@ func (f *FlagSet) Set(name, value string) error {
 
 // Set sets the value of the named command-line flag.
 func Set(name, value string) error {
-       return CommandLine.Set(name, value)
+       return CommandLine.set(name, value)
 }
 
 // isZeroValue determines whether the string represents the zero
@@ -1004,6 +1027,9 @@ func (f *FlagSet) Var(value Value, name string, usage string) {
                }
                panic(msg) // Happens only if flags are declared with identical names
        }
+       if pos := f.undef[name]; pos != "" {
+               panic(fmt.Sprintf("flag %s set at %s before being defined", name, pos))
+       }
        if f.formal == nil {
                f.formal = make(map[string]*Flag)
        }

diff --git a/src/flag/flag_test.go b/src/flag/flag_test.go
index 14d199d6e950a80f05274402e2c216c8dc7ecbd4..57c88f009f1c4f6da7d80fb54181e5c879f0836e 100644 (file)
--- a/src/flag/flag_test.go
+++ b/src/flag/flag_test.go
@@ -12,6 +12,7 @@ import (
        "io"
        "os"
        "os/exec"
+       "regexp"
        "runtime"
        "sort"
        "strconv"
@@ -726,7 +727,7 @@ func mustPanic(t *testing.T, testName string, expected string, f func()) {
                case nil:
                        t.Errorf("%s\n: expected panic(%q), but did not panic", testName, expected)
                case string:
-                       if msg != expected {
+                       if ok, _ := regexp.MatchString(expected, msg); !ok {
                                t.Errorf("%s\n: expected panic(%q), but got panic(%q)", testName, expected, msg)
                        }
                default:
@@ -844,3 +845,14 @@ func TestUserDefinedBoolFunc(t *testing.T) {
                t.Errorf(`got %q; error should contain "test error"`, errMsg)
        }
 }
+
+func TestDefineAfterSet(t *testing.T) {
+       flags := NewFlagSet("test", ContinueOnError)
+       // Set by itself doesn't panic.
+       flags.Set("myFlag", "value")
+
+       // Define-after-set panics.
+       mustPanic(t, "DefineAfterSet", "flag myFlag set at .*/flag_test.go:.* before being defined", func() {
+               _ = flags.String("myFlag", "default", "usage")
+       })
+}