New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

flag: PrintDefaults panics trying to ascertain whether a flag was set #28667

Open
alandonovan opened this Issue Nov 8, 2018 · 15 comments

Comments

Projects
None yet
7 participants
@alandonovan
Contributor

alandonovan commented Nov 8, 2018

(*flag.FlagSet).PrintDefaults assumes that it is safe to call .String() on the zero value of a flag type, or if that flag type is a pointer, a new instance of the variable. Consequently, a program that uses indirect data structures in its flags will panic when a user sets the -h flag.

$ go version
go version devel +7da1f7addf Thu Nov 8 08:19:48 2018 +0000 linux/amd64
$ cat a.go
package main

import "flag"

func main() {
        f := &myFlag{x: new(int)}
        flag.CommandLine.Var(f, "f", "usage")
        flag.CommandLine.PrintDefaults()
}

type myFlag struct {
        x *int
}

func (f *myFlag) Set(s string) error { return nil }
func (f *myFlag) String() string     { _ = *f.x; return "" }

$ go run a.go
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x491878]

goroutine 1 [running]:
main.(*myFlag).String(0xc00007c030, 0x4ab080, 0xc00007c030)
        /usr/local/google/home/adonovan/got/src/golang.org/x/tools/a.go:16 +0x8
flag.isZeroValue(0xc000086000, 0x0, 0x0, 0x4c49e4)
        /usr/local/google/home/adonovan/goroot/src/flag/flag.go:457 +0xff
flag.(*FlagSet).PrintDefaults.func1(0xc000086000)
        /usr/local/google/home/adonovan/goroot/src/flag/flag.go:520 +0x202
flag.(*FlagSet).VisitAll(0xc00007e120, 0xc000092f28)
        /usr/local/google/home/adonovan/goroot/src/flag/flag.go:387 +0x61
flag.(*FlagSet).PrintDefaults(0xc00007e120)
        /usr/local/google/home/adonovan/goroot/src/flag/flag.go:503 +0x4e
main.main()
        /usr/local/google/home/adonovan/got/src/golang.org/x/tools/a.go:8 +0xbc
exit status 2

I don't think the flag package should be making assumptions about the concrete representation of the flag. That's the point of the flag.Value interface.

@bontibon

This comment has been minimized.

Contributor

bontibon commented Nov 8, 2018

Seems like this is covered in #24439.

Note that this functionality is documented:

The flag package may call the String method with a zero-valued receiver, such as a nil pointer.

@alandonovan

This comment has been minimized.

Contributor

alandonovan commented Nov 8, 2018

Yes, the isses are substantially similar. The documentation is incorrect though: it should say that flag may call .String() on a zero value of the flag type, or if the flag type is a pointer, on a pointer to a new variable. Not a precondition that exactly rolls off the tongue; frankly I'm surprised this wasn't considered a breaking semantic change to a published interface.

@robpike

This comment has been minimized.

Contributor

robpike commented Nov 8, 2018

What is "this" that was a breaking change?

@alandonovan

This comment has been minimized.

Contributor

alandonovan commented Nov 9, 2018

@robpike

This comment has been minimized.

Contributor

robpike commented Nov 9, 2018

Happy to see that rolled back. I didn't like it at all at first, and only a little bit later on.

@jimmyfrasche

This comment has been minimized.

Member

jimmyfrasche commented Nov 9, 2018

The simplest thing would be to print DefValue whenever it was not the empty string. Then isZeroValue can just be deleted.

@bcmills

This comment has been minimized.

Member

bcmills commented Nov 19, 2018

@jimmyfrasche, or we could record whether the default value is the zero value of the type when we first set DefValue, rather than trying to infer it after-the-fact.

(We could even do that without changing the definition of *flag.Flag, by either storing it in a parallel map or wrapping the Flag in a larger, unexported struct.)

@bcmills bcmills added this to the Go1.13 milestone Nov 19, 2018

@jimmyfrasche

This comment has been minimized.

Member

jimmyfrasche commented Nov 19, 2018

@bcmills that would work reliably with all flags satisfying the optional Get() interface{} method.

For flag.Value without a Get method, it would have to assume that the Value is the actual value of the flag. But it could also be a nontrivial container for the value—like some nonzero parsing state but a zero default value.

It would be much superior to the status quo in that it would never explode. It would probably do the right thing most of the time. A nontrivial Value could implement Get() to provide the desired behavior, but that's an awkward enough subtlety to justify documentation, imo.

@robpike

This comment has been minimized.

Contributor

robpike commented Nov 19, 2018

Why don't we just roll back the change?

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Nov 19, 2018

The change in question (https://golang.org/cl/23581) fixes #15904. Of course we can decide to not fix that, though I'll note that it was reported more than once.

@jimmyfrasche

This comment has been minimized.

Member

jimmyfrasche commented Nov 19, 2018

Checking against a whitelist of zero values is simpler to maintain, but it has false positives and negatives for user-defined flags. In addition to not recognizing "0s" there was #23543 where a string flag with "0" was being falsely recognized as a zero value.

Only checking against the empty string is even simpler. It's also uniform in that it works the same for any flag, regardless of how or where it is defined. It checks against the shell's concept of a zero value rather than Go's, so it may be of greater value to an end user unaware of Go's concept of zero value. Debatable.

Using reflect to see if it's the zero value is uniform and extensible but less simple to do right, empirically. Perhaps @bcmills' version would work if it were simplified to only run if there's a Get method and otherwise just check against the empty string.

Another option would be yet another optional method on Value like ShowDefault() bool but I don't think anyone wants to go down that road.

@robpike

This comment has been minimized.

Contributor

robpike commented Nov 19, 2018

@ianlancetaylor But the fix is problematic. We can roll back this fix and handle #15904 another way, rather than continuing to hack and redesign and complicate a fundamentally simple package.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Nov 19, 2018

If someone has a different fix that is certainly fine with me.

@jimmyfrasche

This comment has been minimized.

Member

jimmyfrasche commented Nov 20, 2018

I did a sketch of my simplification of @bcmills' check in FlagSet.Var: https://play.golang.org/p/Jsl2nSaJN8U

I'm guessing it would be roughly the same LOC as the current fix but it would be more reliable and less surprising.

@pam4

This comment has been minimized.

pam4 commented Dec 7, 2018

Issue #24439 already cover the documentation being incorrect, see here:

Here's my proposed fix:
// The flag package may call the String method with a zero-valued receiver,
// or with a pointer pointing to a zero value for the base type (in case of
// a pointer receiver).

This is the same as @alandonovan said above, just from the String method perspective.
Yes, it doesn't roll off the tongue either, but unless a code change is coming soon, I think fixing the docs would be nice, especially because it is contradicted by the example (missing check for nil pointer in String method).

The mentioned breaking change broke one of my flag.Value implementations too, at that time, causing panic.
My use case was to use boolean flags to select one of many exclusive modes, so that the last one takes effect.
E.g. -slow/-normal/-fast (to the same effect of something like -mode=slow/-mode=normal/-mode=fast).

type myFlag struct {
    name string  // either "slow", "normal" or "fast"
    ptr  *string // shared pointer to the selected mode
}

func (f myFlag) String() string {
    // return strconv.FormatBool(*f.ptr == f.name) // panic
    return strconv.FormatBool(f.ptr != nil && *f.ptr == f.name) // fixed
}

I made a zero-valued myFlag (which was not supposed to happen) produce "false" to match the normally occurring falses. If I had returned something else instead, say "invalid", it wouldn't have worked.
In fact, issue #24439 is also proposing to clarify such subtlety in the docs.

I think the fix proposed here is simpler and easier to document. Users would have to make adjustments to their code again to avoid printing redundant defaults, but that's a minor issue compared to the panic (in my case I would have to add the Get method).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment