Re: Pointer Receivers on Error() and String() methods
On 24 August 2015 at 19:22, Nate Finch wrote:
> That's fair. I wasn't sure if recovering the panic in errors would be a
> good idea, since it could hide programmer error (like what I'd done). But
> I'm fine with that solution.
To be fair, I'm not entirely sure that panic recovery is a good idea
either, but fmt sets the
precedent to follow here.
> On Mon, Aug 24, 2015 at 3:22 AM David Cheney
> wrote:
>>
>> 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
>> 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 just like normal nil
>> > errors.
>> >
>> >
>> > On 20 August 2015 at 03:45, Nate Finch 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
Re: Pointer Receivers on Error() and String() methods
That's fair. I wasn't sure if recovering the panic in errors would be a
good idea, since it could hide programmer error (like what I'd done). But
I'm fine with that solution.
On Mon, Aug 24, 2015 at 3:22 AM David Cheney
wrote:
> 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
> 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 just like normal nil
> > errors.
> >
> >
> > On 20 August 2015 at 03:45, Nate Finch 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 ""
> >> }
> >> 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
> f
Re: Pointer Receivers on Error() and String() methods
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 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 just like normal nil
> errors.
>
>
> On 20 August 2015 at 03:45, Nate Finch 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 ""
>> }
>> 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
Re: Pointer Receivers on Error() and String() methods
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 just like normal nil
errors.
On 20 August 2015 at 03:45, Nate Finch 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 ""
> }
> 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
Pointer Receivers on Error() and String() methods
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 ""
}
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
