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