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: Value description is inaccurate about zero-values #24439

Open
hubslave opened this Issue Mar 18, 2018 · 8 comments

Comments

Projects
None yet
6 participants
@hubslave

hubslave commented Mar 18, 2018

// The flag package may call the String method with a zero-valued receiver,
// such as a nil pointer.
type Value interface {
    String() string
    Set(string) error
}

But in the example we have:

// String is the method to format the flag's value, part of the flag.Value interface.
// The String method's output will be used in diagnostics.
func (i *interval) String() string {
    return fmt.Sprint(*i)
}

Which would panic if called with a nil pointer receiver.

Actually that code should be fine because String is never called with a nil pointer receiver (flag.isZeroValue uses reflect.New if the receiver is a pointer), but it is still contradictory.

In fact, from the current phrasing you could assume that doing something like this is fine, which is not:

func (x *mytype) String() string {
    if x == nil { // guard for flag.Value requirement
        return "foo"
    }
    return *x.ptr // my constructor guarantees this is fine
}

Moreover from the flag.Value description it is not outright clear why the caller has to prepare to handle a value that he never produced.
Yes, the flag.PrintDefaults description answers that, but maybe it would be helpful to mention in the flag.Value description too, that the zero-value string representation is just used to turn off displaying the default (either because it is an unused value meaning off, or because it is an obvious default).
For instance if someone writes a flag.Value implementation whose zero-value (not the nil pointer) represents an inconsistent state (like I did), he would have to figure out what his String method is supposed to return in such a case and why it matters.

@mvdan

This comment has been minimized.

Member

mvdan commented Mar 18, 2018

doing something like this is fine, which is not

Why is it not fine? Because the nil check is redundant in practice?

/cc @robpike

@mvdan mvdan added the NeedsDecision label Mar 18, 2018

@mvdan mvdan added this to the Go1.11 milestone Mar 18, 2018

@mvdan mvdan changed the title from flag.Value description is inaccurate about zero-values to flag: Value description is inaccurate about zero-values Mar 18, 2018

@hubslave

This comment has been minimized.

hubslave commented Mar 18, 2018

Why is it not fine? Because the nil check is redundant in practice?

And because it panics on the *x.ptr dereferencing.
If your flag.Value's are pointers, the flag package uses reflect.New to produce a new one, and calls String on it. Therefore the receiver itself is not zero-valued, but the value it points to is.

@robpike

This comment has been minimized.

Contributor

robpike commented Mar 19, 2018

If there's anything to do here, and I'm not sure there is, it would be to adjust the example to be more careful:

// String is the method to format the flag's value, part of the flag.Value interface.
// The String method's output will be used in diagnostics.
func (i *interval) String() string {
    if i == nil {
        return "[]"
    }
    return fmt.Sprint(*i)
}
@hubslave

This comment has been minimized.

hubslave commented Mar 19, 2018

I would be happy to be proven wrong, but I think it deserves a little more justification than that.
I'm pointing out 2 issues here, and I consider both important:

  1. The String method in the example obviously fails to fulfill a stated requirement (the flag package says it may do something that would make it panic).
    Please clarify why this is not a problem.

  2. See my example in the 3rd code block: I would expect it to work just fine, but it panics (when called by flag.isZeroValue).
    The doc says that the receiver itself can be zero-valued, not the value it points to. This is specially reinforced by the "such as a nil pointer" bit.
    I would conclude that a String implementation is allowed to check for a nil receiver and otherwise just assume that the receiver was made with its own constructor.
    Obviously you don't agree with that, otherwise you would see the point. Can you explain why?

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 what actually happens in the code.
String is never called with a nil pointer, though I have no objection if the doc says it may do more than what it actually does, as long as it doesn't do more than what it actually says!

@rsc

This comment has been minimized.

Contributor

rsc commented Mar 27, 2018

In any event it seems like the flag package only ever uses Values that clients pass in.

@rsc rsc added the NeedsFix label Mar 27, 2018

@rsc

This comment has been minimized.

Contributor

rsc commented Mar 27, 2018

Assigning to @robpike to fix one way or another.

@hubslave

This comment has been minimized.

hubslave commented Mar 27, 2018

In any event it seems like the flag package only ever uses Values that clients pass in.

@rsc I'm not sure what you mean.
The flag.isZeroValue function makes new Values from scratch with reflect and calls String on them.
Otherwise we would not have needed a constraint for the String method in the docs in the first place.
Is the problem with my example in the 3rd code block clear to you?
https://play.golang.org/p/-VO6paFNd6H
I could give you more realistic (and more complex) examples.

(Other Values not passed in by clients include the flag package's own private types which are used for all non-custom flags, but they work fine.)

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Dec 12, 2018

See also #28667.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.12, Go1.13 Dec 12, 2018

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