Re: Pointer Receivers on Error() and String() methods
On 24 August 2015 at 19:22, Nate Finch nate.fi...@canonical.com 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 david.che...@canonical.com
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 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
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 david.che...@canonical.com
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 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
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 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
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 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
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 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
