Catch panics when calling GoString like fmt %#v does

[benhoyt]

This handles a few cases (similar to how fmt %#v does):

1. A GoString method on a value receiver, called with a nil pointer
2. A GoString method on a pointer receiver that doesn't check for nil
3. A GoString method that panics in some other way

Because Go 1.17 added a method Time.GoString with value receiver, this
broke structs that had *time.Time fields with nil values (which is
common!).

Also added a bunch of tests for these cases.

This should address kr#77

[barrettj12]

Looks good. Would just like to check that it works on the example given in the original issue, as well as the logging in the Juju CAAS package.

[jameinel]

I feel like we definitely want the defer p.catchPanic() to handle any sort of panic while calling GoString.
I'm not sure that we want to handle nil pointers there, vs having it as part of the standard (non-exceptional) path.

[benhoyt]

@jameinel (Copying from my Mattermost message.) I don't think we can let the nil pointer case fall through. This block is after we already know the thing has a GoString method, and it may be a pointer receiver, or may not. If it's a pointer receiver and the pointer is nil, we still want to call GoString in case that method has special handling for receiver == nil. So I think this is correct (and probably why fmt does it that way).

[benhoyt]

@barrettj12 FYI, I've also just tested this against the example program at kr#77, and it works fine too.

[barrettj12]

I added a test for a wrapper around ValueGoString - this is more in the character of the original error (the panic was coming from a nil *metav1.Time which is a wrapper around time.Time). The difference is, you get

"%!v(PANIC=Format method: runtime error: invalid memory address or nil pointer dereference)"

instead of

%!v(PANIC=Format method: value method github.com/kr/pretty.ValueGoString.GoString called using nil *ValueGoString pointer)

(also snuck in a .gitignore addition)

I've tested these changes against the original example in the issue, as well as our logs in the Juju CAAS provider. Both format correctly now. Therefore, I am happy for these changes to go through. @jameinel if you still have some issues, let's resolve those before merging.

[jameinel]

One problem I have with:

"%!v(PANIC=Format method: runtime error: invalid memory address or nil pointer dereference)"

Is that it is very hard for me to realize that Format is the name of the method that was called.
While I agree that the previous one is a bit verbose:

%!v(PANIC=Format method: value method github.com/kr/pretty.ValueGoString.GoString called using nil *ValueGoString pointer)

It does at least point me towards something I could do about it. I think one problem is that "Format method" sounds like a poorly formed sentence telling me that somehow a method was formatted incorrectly. Compare that to something like:

"%!v(PANIC=called method "Format": runtime error: invalid memory address or nil pointer dereference)"

However, even with that, I still don't have any idea what Format method is invalid. Is it somehow otherwise available from other context? What pointer is nil? How do I actually debug and fix that?
Maybe it would be available from a panic() and a stack trace (though we're intentionally recovering, so all you end up with in the log is that some Format method was invalid).

[jameinel]

I'm ok with the changes, but I did want to propose a tweak to the error message, at least to think through what we want to do. We can do exactly the same as fmt.Print (though in that case, we shouldn't be giving you the type either because fmt does not)

I don't think your average developer gets panics in their fmt.Format so often that they have internalized the structure of the error message, and thus changing it to be more obvious about what method is being called would be useful. If you feel consistency is better, then we can land it as is.

[jameinel]

So it looks like to my earlier comment, you do at least get the type here, which points you in the right direction.
It also does match what Format does, though I struggled a lot to get a failure there:

package main

import (
    "fmt"
    "time"

    "github.com/kr/pretty"
)

type Value struct {}
func (Value) Format(f fmt.State, verb rune) { f.Write([]byte("formatted"))}
func (Value) GoString() string { return "gostringed" }

type Panic struct {}
func (Panic) Format(f fmt.State, verb rune) { panic("failed")}
func main() {
    now := time.Now()
    var t *time.Time
    // t = &now
    _ = now
    fmt.Println(pretty.Sprint(t))
    fmt.Println("done")
    var val Value
    fmt.Println(val)
    var ptr *Value
    fmt.Println(ptr)
    var pnc Panic
    fmt.Println(pnc)
}

ends up outputting:

$ go run main.go
%!v(PANIC=Format method: value method time.Time.GoString called using nil *Time pointer)
done
formatted
<nil>
%!v(PANIC=Format method: failed)

Its a bit of a shame that the standard is so hard to parse. I'm willing to go along with established precedent, but I do feel that:

"(*pretty.PanicGoString)(PANIC=calling method "GoString": oops!)"}

Is easier for a human to understand and realize what they need to fix.

Thoughts?

[benhoyt]

Thanks @jameinel. I agree that calling method "GoString": ... is easier to read and understand, and that the error message being exactly the same as fmt's is not necessary. Updated!