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 <[email protected]> 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 > [email protected] > Modify settings or unsubscribe at: > https://lists.ubuntu.com/mailman/listinfo/juju-dev > -- Juju-dev mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
