Flag default assignments should be lazily applied

[briandealwis]

Re: GoogleContainerTools/skaffold#4129, spf13/cobra#1047

In Skaffold we use cobra and pflags to define our CLI. We have the flags toggle values directly in an options object. We have two commands that take the same flags but with different defaults. The following snippet demonstrates the issue:

var blowup bool

cmd1 := cobra.Command{}
cmd1.Flags().BoolVar(&blowup, "blowup", false, "When set to false, blowup")

cmd2 := cobra.Command{}
cmd2.Flags().BoolVar(&blowup, "blowup", true, "When set to false, blowup")

cmd1.ParseFlags(os.Args[1:])
fmt.Printf("blowup = %v\n", blowup)

So even though we have two FlagSets, because they reference the same value, the default value for the last flag defined "wins" unless the option is explicitly defined on the command-line.

The issue here is that pflags applies the default value immediately. BoolVar() just delegates BoolVarP(), and BoolVarP() uses newBoolValue():

pflag/bool.go

Lines 47 to 57 in 81378bb

	// BoolVar defines a bool flag with specified name, default value, and usage string.
	// The argument p points to a bool variable in which to store the value of the flag.
	func (f *FlagSet) BoolVar(p *bool, name string, value bool, usage string) {
	f.BoolVarP(p, name, "", value, usage)
	}
	// BoolVarP is like BoolVar, but accepts a shorthand letter that can be used after a single dash.
	func (f *FlagSet) BoolVarP(p *bool, name, shorthand string, value bool, usage string) {
	flag := f.VarPF(newBoolValue(value, p), name, shorthand, usage)
	flag.NoOptDefVal = "true"
	}

newBoolValue() allocates a container to map the provider pointer to the default value, but it immediately assigns the value:

pflag/bool.go

Lines 15 to 18 in 81378bb

	func newBoolValue(val bool, p *bool) *boolValue {
	*p = val
	return (*boolValue)(p)
	}

[yufeiminds]

I also need a lazily applied default value in my command-line tool.

Now I write default value as zero value and fill them at runtime, then write default to flag usage manually.

Is there any way to resolve it?

[pasamio]

I ran into this as well re-using an variable with two commands setting different defaults for the same variable as a convenience for the end user. I ended up isolating the variables and doing a small refactoring to make it work but it caught me off guard that a default value from a flag on a different command would be used based on essentially which one is configured last.

[cornfeedhobo]

I think this is user error. I would suggest using a lookup instead, and stop relying on passing a pointer around.

If someone can present some other use cases for lazy parsing, I'd maybe take a look at the issue on my fork.

[pasamio]

It could be classed as user error, I'm coming to this via Cobra that provided examples using this and I kept following that pattern. I think for someone running the code in two different commands via Cobra this provides a hard to understand and debug behaviour. It doesn't do what I expected and provides a surprise because the challenge is that this can influence the behaviour of another part of the program which is additionally confusing.

I ran into this twice before really realising what was going on. Using lookup instead is an alternative but the library provides a feature, I don't think it's an unreasonable expectation that when paired with Cobra that defaults are lazily applied when their commands are actually invoked. The current behaviour is in a sense hard to decipher because it isn't apparent that the two commands are interacting to change the default value which isn't entirely deterministic to the programmer at the time of writing. For people who know better they won't hit it however this is also a trap that at least a few people seem to have run into and I personally feel that in terms of being user friendly it doesn't seem wrong to defer setting the default value whilst removing a pitfall for a developer new to the library.

[cornfeedhobo]

@pasamio my point is that this is fundamental to golang, not an error with the package. You are passing around a pointer and expecting it to not be overwritten.

[pasamio]

I'm passing a pointer to a variable expecting that it will be filled with the value of my default argument for the flag that is currently being evaluated rather than it being filled with a value from a flag that is not currently being evaluated that is in another part of my Cobra application. I don't think it's an unreasonable expectation that when I hand the library a pointer to my variable that when my flags are evaluated that the variable is either populated with the value the user provided on the command line to the application or the default value I set for the flag in that command.

I think the challenge is the expectation between what pflag does and how that is used in Cobra.

[outofforest]

I digged into the source code of pflag to fix this but the implementation is so strange that solving this would require rewriting of entire library

[outofforest]

Assumption that value of pointer is overwritten immediately when flag is defined, not when Parse is called on FlagSet (here for example: https://github.com/spf13/pflag/blob/master/bool.go#L15) is really counter-intuitive. Pointer should be touched just before reading flags from CLI, not earlier.

I started working on fixing this by storing default value separately and applying it in Parse and ParseAll before reading values from CLI. But Value interface (https://github.com/spf13/pflag/blob/master/flag.go#L187) has no method to copy current value from one Value to another. I thought that doing sth like value.Set(defaultValue.String()) may work but due to reaaaaaalllyyy bad design of pflag library it doesn't work this way for all the types (built-in and custom ones). It would require adding new method Copy to all the types which would break compatibility.

I have to say that: During my research I found that pflag is soo poorly designed that it should be reimplemented completely. Take logic of this function as an example: https://github.com/spf13/pflag/blob/master/flag.go#L538