Pointer Receivers on Error() and String() methods

[roger peppe]

I'm afraid I'm not convinced. Declaring the Error method on the
pointer receiver is idiomatic (just grep for ' Error\(' in the Go source)
and is a useful indicator that the error value is always intended
to be a pointer.

There's a much simpler fix for this: let the errors package
recover from this itself. We can just make Err.Error call fmt.Sprint
to get the error message (a one line change)

Then a wrapped nil error will print <nil> just like normal nil
errors.


On 20 August 2015 at 03:45, Nate Finch <nate.finch at canonical.com> wrote:
> tl;dr:  Don't.  Use a value receiver.  99% of the time you can just delete
> the * on the receiver and it'll still work.  If you really must use a
> pointer, please handle the case where you're called with a nil receiver.
>
> I spent most of today trying to understand why my new hook command was
> producing this output:
>
> error: %!v(PANIC=runtime error: invalid memory address or nil pointer
> dereference)
>
> It took me a while to figure out that this is what fmt.Printf("error: %v\n",
> err) outputs when err's Error() method panics.  If you're using %s or %v to
> print a value (or use Println which implicitly uses %v), then fmt will look
> for a String() method or Error() method on the value to call, and use the
> output of that for the value's string output.  If that method panics, fmt
> prints the panic in the way you see above (everything after the PANIC=).
>
> Of course, the problem here is that there's no type being written, and since
> it was an error interface, it could almost anything.  Using %#v skips
> calling the Error/String methods and prints out the values in a go format,
> which told me this was a juju/errors.Err value, wrapping an API params Error
> value which was a nil pointer.  When we call Error() on an errors.Err, we
> call Error() on the cause explicitly, which was panicking.
>
> Here's a minimal reproduction http://play.golang.org/p/ncNVrza-hn (you'll
> have to copy it to a local file and go run it, since the playground won't
> run code external to the stdlib).
>
> So what's sort of interesting is that printing the error before it gets
> Traced works fine, but after the trace it is not fine.  The errors.Err's
> Error() function looks like it's explicitly calling the Error() method on
> the wrapped Cause error, which is probably the problem.  fmt.Printf must use
> some reflection magic to avoid doing that.
>
> Now, the root cause of this particular bug is actually my own mistake.  Line
> 21 should check if orig is nil and then assign nil explicitly to err if it
> is.  Then errors.Trace would be able to tell that the error is nil and would
> just return nil itself, instead of thinking it's a valid error and wrapping
> it.
>
> However, you can sidestep this entirely by doing one of two things:  either
> just make the Error() (or String()) method use a value receiver.. in which
> case this code would produce this output:
>
> %!v(PANIC=value method main.MyError.Error called using nil *MyError pointer)
>
> (you can try it with the repro code I linked to)
>
> This printout is a lot more helpful and useful and obvious than the other
> "nil pointer" printout.
>
> OR
>
> Just handle a nil receiver:
>
> func (e *MyError) Error() string {
>     if e == nil {
>         return "<nil MyError>"
>     }
>     return e.Message
> }
>
> (note that it is dereferencing the pointer to e to access the Message field
> which causes the panic. Calling a method on a nil pointer is totally fine
> and will not panic if the code inside does not try to derefence the pointer
> to get to a field).
>
> Grepping through our code, I see a lot of pointer receivers on Error and
> String methods (45 and 77 respectively).  I think we should at least change
> all of these to be value methods (unless that's not possible.   That's a
> trivial change, and gives a much more useful printout when the pointer is
> nil.
>
> -Nate
>
> --
> Juju-dev mailing list
> Juju-dev at lists.ubuntu.com
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/juju-dev
>