Yup, I agree with Rog. This is fixing the problem in the wrong place.
On Mon, Aug 24, 2015 at 5:14 PM, roger peppe <roger.pe...@canonical.com> wrote:
> 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.fi...@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@lists.ubuntu.com
>> Modify settings or unsubscribe at:
>> https://lists.ubuntu.com/mailman/listinfo/juju-dev
>>
>
> --
> Juju-dev mailing list
> Juju-dev@lists.ubuntu.com
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/juju-dev
--
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/juju-dev
