Don't panic when a Stringer field value is nil

This commit ensures that zap.Stringer won't cause a panic when the
given field value is nil or a typed nil[1]. In fact any panic
caused by the Stringer implementation would be recovered and
printed. For panic("foo") for a field k we would see kError=foo
within the log. A panic with an error would result in the error
message in kError. Other kinds of panic values are simply printed
as "<unknown>" since trying to convert the value itself could
cause a panic again which would trigger the panic within recover
panic.

[1]: https://golang.org/doc/faq#nil_error

zapcore/field.go

@@ -22,6 +22,7 @@ package zapcore
 
 import (
 	"bytes"
+	"errors"
 	"fmt"
 	"math"
 	"reflect"
@@ -160,7 +161,7 @@ func (f Field) AddTo(enc ObjectEncoder) {
 	case NamespaceType:
 		enc.OpenNamespace(f.Key)
 	case StringerType:
-		enc.AddString(f.Key, f.Interface.(fmt.Stringer).String())
+		err = encodeStringer(f.Key, f.Interface, enc)
 	case ErrorType:
 		encodeError(f.Key, f.Interface.(error), enc)
 	case SkipType:
@@ -199,3 +200,19 @@ func addFields(enc ObjectEncoder, fields []Field) {
 		fields[i].AddTo(enc)
 	}
 }
+
+func encodeStringer(key string, stringer interface{}, enc ObjectEncoder) (err error) {
+	defer func() {
+		if v := recover(); v != nil {
+			switch v := v.(type) {
+			case error:
+				err = v
+			default:
+				err = errors.New(fmt.Sprintf("PANIC=%v", v))
+			}
+		}
+	}()
+
+	enc.AddString(key, stringer.(fmt.Stringer).String())
+	return
+}

zapcore/field_test.go

@@ -24,13 +24,12 @@ import (
 	"errors"
 	"fmt"
 	"math"
+	"net/url"
 	"testing"
 	"time"
 
-	"go.uber.org/zap"
-
 	"github.com/stretchr/testify/assert"
-
+	"go.uber.org/zap"
 	. "go.uber.org/zap/zapcore"
 )
 
@@ -58,6 +57,28 @@ func (u users) MarshalLogArray(enc ArrayEncoder) error {
 	return nil
 }
 
+type obj struct {
+	kind int
+}
+
+func (o *obj) String() string {
+	if o == nil {
+		return "nil obj"
+	}
+
+	if o.kind == 1 {
+		panic("panic with string")
+	} else if o.kind == 2 {
+		panic(errors.New("panic with error"))
+	} else if o.kind == 3 {
+		// panic with an arbitrary object that causes a panic itself
+		// when being converted to a string
+		panic((*url.URL)(nil))
+	}
+
+	return "obj"
+}
+
 func TestUnknownFieldType(t *testing.T) {
 	unknown := Field{Key: "k", String: "foo"}
 	assert.Equal(t, UnknownType, unknown.Type, "Expected zero value of FieldType to be UnknownType.")
@@ -67,19 +88,27 @@ func TestUnknownFieldType(t *testing.T) {
 }
 
 func TestFieldAddingError(t *testing.T) {
+	var empty interface{}
 	tests := []struct {
-		t    FieldType
-		want interface{}
+		t     FieldType
+		iface interface{}
+		want  interface{}
+		err   string
 	}{
-		{ArrayMarshalerType, []interface{}{}},
-		{ObjectMarshalerType, map[string]interface{}{}},
+		{t: ArrayMarshalerType, iface: users(-1), want: []interface{}{}, err: "too few users"},
+		{t: ObjectMarshalerType, iface: users(-1), want: map[string]interface{}{}, err: "too few users"},
+		{t: StringerType, iface: obj{}, want: empty, err: "interface conversion: zapcore_test.obj is not fmt.Stringer: missing method String"},
+		{t: StringerType, iface: &obj{1}, want: empty, err: "PANIC=panic with string"},
+		{t: StringerType, iface: &obj{2}, want: empty, err: "panic with error"},
+		{t: StringerType, iface: &obj{3}, want: empty, err: "PANIC=<nil>"},
+		{t: StringerType, iface: (*url.URL)(nil), want: empty, err: "runtime error: invalid memory address or nil pointer dereference"},
 	}
 	for _, tt := range tests {
-		f := Field{Key: "k", Interface: users(-1), Type: tt.t}
+		f := Field{Key: "k", Interface: tt.iface, Type: tt.t}
 		enc := NewMapObjectEncoder()
 		assert.NotPanics(t, func() { f.AddTo(enc) }, "Unexpected panic when adding fields returns an error.")
 		assert.Equal(t, tt.want, enc.Fields["k"], "On error, expected zero value in field.Key.")
-		assert.Equal(t, "too few users", enc.Fields["kError"], "Expected error message in log context.")
+		assert.Equal(t, tt.err, enc.Fields["kError"], "Expected error message in log context.")
 	}
 }
 
@@ -116,6 +145,8 @@ func TestFields(t *testing.T) {
 		{t: ReflectType, iface: users(2), want: users(2)},
 		{t: NamespaceType, want: map[string]interface{}{}},
 		{t: StringerType, iface: users(2), want: "2 users"},
+		{t: StringerType, iface: &obj{}, want: "obj"},
+		{t: StringerType, iface: (*obj)(nil), want: "nil obj"},
 		{t: SkipType, want: interface{}(nil)},
 	}