From be2be86550401f20bc7d8ece1969a07150cb46b7 Mon Sep 17 00:00:00 2001 From: Prashant Varanasi Date: Fri, 28 Aug 2020 14:11:23 -0700 Subject: [PATCH] Update Stringer panic check to look like stdlib (#857) There's no behaviour changes, but there are a couple of refactorings: * Name the named return error `retErr`, and use explicit return values. The only purpose of the named return is for the panic handling. * Make the panic handling look more similar to the standard library and add a reference to the stdlib code in fmt that does the same checks. --- zapcore/field.go | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/zapcore/field.go b/zapcore/field.go index ef46656be..7e255d63e 100644 --- a/zapcore/field.go +++ b/zapcore/field.go @@ -205,18 +205,23 @@ func addFields(enc ObjectEncoder, fields []Field) { } } -func encodeStringer(key string, stringer interface{}, enc ObjectEncoder) (err error) { +func encodeStringer(key string, stringer interface{}, enc ObjectEncoder) (retErr error) { + // Try to capture panics (from nil references or otherwise) when calling + // the String() method, similar to https://golang.org/src/fmt/print.go#L540 defer func() { - if v := recover(); v != nil { - val := reflect.ValueOf(stringer) - if val.Kind() == reflect.Ptr && val.IsNil() { + if err := recover(); err != nil { + // If it's a nil pointer, just say "". The likeliest causes are a + // Stringer that fails to guard against nil or a nil pointer for a + // value receiver, and in either case, "" is a nice result. + if v := reflect.ValueOf(stringer); v.Kind() == reflect.Ptr && v.IsNil() { enc.AddString(key, "") - } else { - err = fmt.Errorf("PANIC=%v", v) + return } + + retErr = fmt.Errorf("PANIC=%v", err) } }() enc.AddString(key, stringer.(fmt.Stringer).String()) - return + return nil }