funcr: Handle nil Stringer, Marshaler, error

Produce a useful error rather than panic when we can be sure that the
panic was a nil-ptr.  Otherwise pass the panic on.

funcr/funcr.go

@@ -351,15 +351,15 @@ func (f Formatter) prettyWithFlags(value interface{}, flags uint32, depth int) s
 	if v, ok := value.(logr.Marshaler); ok {
 		// Replace the value with what the type wants to get logged.
 		// That then gets handled below via reflection.
-		value = v.MarshalLog()
+		value = invokeMarshaler(v)
 	}
 
 	// Handle types that want to format themselves.
 	switch v := value.(type) {
 	case fmt.Stringer:
-		value = v.String()
+		value = invokeStringer(v)
 	case error:
-		value = v.Error()
+		value = invokeError(v)
 	}
 
 	// Handling the most common types without reflect is a small perf win.
@@ -596,6 +596,56 @@ func isEmpty(v reflect.Value) bool {
 	return false
 }
 
+func invokeMarshaler(m logr.Marshaler) (ret interface{}) {
+	defer func() {
+		if err := recover(); err != nil {
+			if isNil(m) {
+				ret = "<nil-logr-marshaler>"
+			} else {
+				panic(err)
+			}
+		}
+	}()
+	return m.MarshalLog()
+}
+
+func invokeStringer(s fmt.Stringer) (ret string) {
+	defer func() {
+		if err := recover(); err != nil {
+			if isNil(s) {
+				ret = "<nil-fmt-stringer>"
+			} else {
+				panic(err)
+			}
+		}
+	}()
+	return s.String()
+}
+
+func invokeError(e error) (ret string) {
+	defer func() {
+		if isNil(e) {
+			if err := recover(); err != nil {
+				ret = "<nil-error>"
+			} else {
+				panic(err)
+			}
+		}
+	}()
+	return e.Error()
+}
+
+func isNil(i interface{}) bool {
+	if i == nil {
+		return true
+	}
+	switch reflect.TypeOf(i).Kind() {
+	case reflect.Ptr, reflect.Map, reflect.Array, reflect.Chan, reflect.Slice:
+		return reflect.ValueOf(i).IsNil()
+	}
+	return false
+}
+
 // Caller represents the original call site for a log line, after considering
 // logr.Logger.WithCallDepth and logr.Logger.WithCallStackHelper.  The File and
 // Line fields will always be provided, while the Func field is optional.

funcr/funcr_test.go

@@ -52,7 +52,7 @@ func (p pointErr) MarshalText() ([]byte, error) {
 }
 
 // Logging this should result in the MarshalLog() value.
-type Tmarshaler string
+type Tmarshaler struct{ val string }
 
 func (t Tmarshaler) MarshalLog() interface{} {
 	return struct{ Inner string }{"I am a logr.Marshaler"}
@@ -66,8 +66,15 @@ func (t Tmarshaler) Error() string {
 	return "Error(): you should not see this"
 }
 
+// Logging this should result in a panic.
+type Tmarshalerpanic struct{ val string }
+
+func (t Tmarshalerpanic) MarshalLog() interface{} {
+	panic("Tmarshalerpanic")
+}
+
 // Logging this should result in the String() value.
-type Tstringer string
+type Tstringer struct{ val string }
 
 func (t Tstringer) String() string {
 	return "I am a fmt.Stringer"
@@ -77,6 +84,27 @@ func (t Tstringer) Error() string {
 	return "Error(): you should not see this"
 }
 
+// Logging this should result in a panic.
+type Tstringerpanic struct{ val string }
+
+func (t Tstringerpanic) String() string {
+	panic("Tstringerpanic")
+}
+
+// Logging this should result in the Error() value.
+type Terror struct{ val string }
+
+func (t Terror) Error() string {
+	return "I am an error"
+}
+
+// Logging this should result in a panic.
+type Terrorpanic struct{ val string }
+
+func (t Terrorpanic) Error() string {
+	panic("Terrorpanic")
+}
+
 type TjsontagsString struct {
 	String1 string `json:"string1"`           // renamed
 	String2 string `json:"-"`                 // ignored
@@ -209,8 +237,9 @@ func TestPretty(t *testing.T) {
 	}
 
 	cases := []struct {
-		val interface{}
-		exp string // used in cases where JSON can't handle it
+		val  interface{}
+		exp  string // used in cases where JSON can't handle it
+		pmsg string // used to test panic cases
 	}{
 		{val: "strval"},
 		{val: "strval\nwith\t\"escapes\""},
@@ -351,16 +380,52 @@ func TestPretty(t *testing.T) {
 			},
 		},
 		{
-			val: Tmarshaler("foobar"),
+			val: Tmarshaler{"foobar"},
+			exp: `{"Inner":"I am a logr.Marshaler"}`,
+		},
+		{
+			val: &Tmarshaler{"foobar"},
 			exp: `{"Inner":"I am a logr.Marshaler"}`,
 		},
 		{
-			val: Tstringer("foobar"),
+			val: (*Tmarshaler)(nil),
+			exp: `"<nil-logr-marshaler>"`,
+		},
+		{
+			val:  Tmarshalerpanic{"foobar"},
+			pmsg: "Tmarshalerpanic",
+		},
+		{
+			val: Tstringer{"foobar"},
+			exp: `"I am a fmt.Stringer"`,
+		},
+		{
+			val: &Tstringer{"foobar"},
 			exp: `"I am a fmt.Stringer"`,
 		},
 		{
-			val: fmt.Errorf("error"),
-			exp: `"error"`,
+			val: (*Tstringer)(nil),
+			exp: `"<nil-fmt-stringer>"`,
+		},
+		{
+			val:  Tstringerpanic{"foobar"},
+			pmsg: "Tstringerpanic",
+		},
+		{
+			val: Terror{"foobar"},
+			exp: `"I am an error"`,
+		},
+		{
+			val: &Terror{"foobar"},
+			exp: `"I am an error"`,
+		},
+		{
+			val: (*Terror)(nil),
+			exp: `"<nil-error>"`,
+		},
+		{
+			val:  Terrorpanic{"foobar"},
+			pmsg: "Terrorpanic",
 		},
 		{
 			val: TjsontagsString{
@@ -514,6 +579,21 @@ func TestPretty(t *testing.T) {
 
 	f := NewFormatter(Options{})
 	for i, tc := range cases {
+		// Handle cases that are supposed to panic.
+		if tc.pmsg != "" {
+			func() {
+				defer func() {
+					r := recover()
+					if r == nil {
+						t.Errorf("[%d]: expected panic(%q)", i, tc.pmsg)
+					} else if fmt.Sprintf("%v", r) != tc.pmsg {
+						t.Errorf("[%d]: expected panic(%q), got %q", i, tc.pmsg, r)
+					}
+				}()
+				_ = f.pretty(tc.val)
+			}()
+			continue
+		}
 		ours := f.pretty(tc.val)
 		want := ""
 		if tc.exp != "" {