fix: rendering time values

This is inspired by stretchr#1829,
but we proceed differently, not checking for a string type but for
type convertibility to a time instead.

Added more tests to check how embedded types, pointers etc actually render and don't cause
panic.

From the original issues:

* fixes stretchr#1078
* fixes stretchr#1079

Signed-off-by: Frederic BIDON <fredbi@yahoo.com>

Author: fredbi

internal/spew/common.go

@@ -98,47 +98,99 @@ func handleMethods(cs *ConfigState, w io.Writer, v reflect.Value) (handled bool)
 		v = unsafeReflectValue(v)
 	}
 
+	defer catchPanic(w, v)
+	handled, continued := handleErrorOrStringer(cs, w, v)
+	if handled {
+		// short-circuit: if we can handle directly without trying to convert to a pointer receiver, we're done.
+		// This allows avoiding to call unsafeReflectValue() or retrieving the value's address when not necessary.
+		return true
+	}
+	if continued {
+		// printed, and wants to return now to continue digging
+		return false
+	}
+
 	// Choose whether or not to do error and Stringer interface lookups against
 	// the base type or a pointer to the base type depending on settings.
+	//
 	// Technically calling one of these methods with a pointer receiver can
-	// mutate the value, however, types which choose to satisify an error or
+	// mutate the value, however, types which choose to satisfy an error or
 	// Stringer interface with a pointer receiver should not be mutating their
 	// state inside these interface methods.
 	if !cs.DisablePointerMethods && !UnsafeDisabled && !v.CanAddr() {
+		// since this is unsafe, there are a few edge cases where it doesn't work well
 		v = unsafeReflectValue(v)
 	}
+
 	if v.CanAddr() {
 		v = v.Addr()
 	}
 
+	handled, _ = handleErrorOrStringer(cs, w, v)
+
+	return handled
+}
+
+// handleErrorOrString is only called when v.CanInterface().
+//
+// NOTE: we should prove that unsafReflectValue doesn't alter this property.
+func handleErrorOrStringer(cs *ConfigState, w io.Writer, v reflect.Value) (handled, continued bool) {
 	// Is it an error or Stringer?
 	switch iface := v.Interface().(type) {
 	case error:
-		defer catchPanic(w, v)
 		if cs.ContinueOnMethod {
 			w.Write(openParenBytes)
 			w.Write([]byte(iface.Error()))
 			w.Write(closeParenBytes)
 			w.Write(spaceBytes)
-			return false
+			return false, true
 		}
 
 		w.Write([]byte(iface.Error()))
-		return true
+		return true, false
 
 	case fmt.Stringer:
-		defer catchPanic(w, v)
 		if cs.ContinueOnMethod {
 			w.Write(openParenBytes)
 			w.Write([]byte(iface.String()))
 			w.Write(closeParenBytes)
 			w.Write(spaceBytes)
-			return false
+			return false, true
 		}
+
 		w.Write([]byte(iface.String()))
-		return true
+		return true, false
+
+	default:
+		// is it convertible to time.Time (or *time.Time)?
+		converted, ok := isConvertibleToTime(v)
+		if !ok {
+			// can't handle this value
+			return false, false
+		}
+
+		if !converted.CanInterface() {
+			return false, false // safeguard
+		}
+
+		timeIface := converted.Interface()
+		stringer, ok := timeIface.(fmt.Stringer)
+		if !ok {
+			return false, false // safeguard
+		}
+
+		if cs.ContinueOnMethod {
+			_, _ = w.Write(openParenBytes)
+			_, _ = w.Write([]byte(stringer.String()))
+			_, _ = w.Write(closeParenBytes)
+			_, _ = w.Write(spaceBytes)
+			return false, true
+		}
+
+		_, _ = w.Write([]byte(stringer.String()))
+
+		return true, false
 	}
-	return false
 }
 
 // printBool outputs a boolean value as true or false to Writer w.
@@ -242,12 +294,14 @@ func newValuesSorter(values []reflect.Value, cs *ConfigState) sort.Interface {
 			vs.strings[i] = b.String()
 		}
 	}
+
 	if vs.strings == nil && cs.SpewKeys {
 		vs.strings = make([]string, len(values))
 		for i := range vs.values {
 			vs.strings[i] = Sprintf("%#v", vs.values[i].Interface())
 		}
 	}
+
 	return vs
 }
 

internal/spew/config.go

@@ -53,6 +53,12 @@ type ConfigState struct {
 	// invoked for types that implement them.
 	DisableMethods bool
 
+	// EnableTimeStringer specifies whether to invoke the Stringer interface on
+	// time.Time values even when DisableMethods is true. This is useful to get
+	// human-readable output for time.Time values while keeping method calls
+	// disabled for other types.
+	EnableTimeStringer bool
+
 	// DisablePointerMethods specifies whether or not to check for and invoke
 	// error and Stringer interfaces on types which only accept a pointer
 	// receiver when the current type is not a pointer.
@@ -102,7 +108,10 @@ type ConfigState struct {
 
 // Config is the active configuration of the top-level functions.
 // The configuration can be changed by modifying the contents of spew.Config.
-var Config = ConfigState{Indent: " "}
+var Config = ConfigState{ //nolint:gochecknoglobals // this is global configuration and we leave it for backward-compatibility
+	Indent:             " ",
+	EnableTimeStringer: true,
+}
 
 // Errorf is a wrapper for fmt.Errorf that treats each argument as if it were
 // passed with a Formatter interface returned by c.NewFormatter.  It returns

internal/spew/dump.go

@@ -303,7 +303,7 @@ func (d *dumpState) dump(v reflect.Value) {
 
 	// Call Stringer/error interfaces if they exist and the handle methods flag
 	// is enabled
-	if !d.cs.DisableMethods {
+	if !d.cs.DisableMethods || (d.cs.EnableTimeStringer && isTime(v)) {
 		if (kind != reflect.Invalid) && (kind != reflect.Interface) {
 			if handled := handleMethods(d.cs, d.w, v); handled {
 				return

internal/spew/dump_test.go

@@ -66,6 +66,7 @@ import (
 	"fmt"
 	"strconv"
 	"testing"
+	"time"
 	"unsafe"
 
 	"github.com/go-openapi/testify/v2/internal/spew"
@@ -957,6 +958,91 @@ func addErrorDumpTests() {
 	addDumpTest(nv, "(*"+vt+")(<nil>)\n")
 }
 
+type (
+	embeddedTime struct {
+		time.Time
+	}
+	embeddedTimePtr struct { // this type is an example where the unsafeReflectValue does not work well
+		*time.Time
+	}
+	redeclaredTime    time.Time
+	redeclaredTimePtr *time.Time
+	aliasedTime       = time.Time
+)
+
+func addTimeDumpTests() {
+	ts := time.Date(2006, time.January, 2, 15, 4, 5, 999999999, time.UTC)
+	alias := aliasedTime(ts) //nolint:unconvert // we want to prove here that aliased types don't matter
+	tsAddr := fmt.Sprintf("%p", &ts)
+	es := embeddedTime{
+		Time: ts,
+	}
+	ps := embeddedTimePtr{
+		Time: &ts,
+	}
+	var tptr *time.Time
+	panick := embeddedTimePtr{
+		Time: nil,
+	}
+	rtptr := redeclaredTimePtr(&ts)
+	ppts := ptr(ptr(ts))
+	ppAddr := fmt.Sprintf("%p", ppts)
+	ppIAddr := fmt.Sprintf("%p", *ppts)
+
+	addDumpTest(
+		// simple time.Time
+		ts,
+		"(time.Time) 2006-01-02 15:04:05.999999999 +0000 UTC\n",
+	)
+	addDumpTest(
+		// aliases are ignored at runtime
+		alias,
+		"(time.Time) 2006-01-02 15:04:05.999999999 +0000 UTC\n",
+	)
+	addDumpTest(
+		// pointer to time.Time
+		&ts,
+		"(*time.Time)("+tsAddr+")(2006-01-02 15:04:05.999999999 +0000 UTC)\n",
+	)
+	addDumpTest(
+		// struct with embedded time.Time
+		es,
+		"(spew_test.embeddedTime) 2006-01-02 15:04:05.999999999 +0000 UTC\n",
+	)
+	addDumpTest(
+		// struct with embedded pointer to time.Time
+		ps,
+		"(spew_test.embeddedTimePtr) 2006-01-02 15:04:05.999999999 +0000 UTC\n",
+	)
+	addDumpTest(
+		// nil time.Time
+		tptr,
+		"(*time.Time)(<nil>)\n",
+	)
+	addDumpTest(
+		// **time.Time
+		ppts,
+		"(**time.Time)("+ppAddr+"->"+ppIAddr+")(2006-01-02 15:04:05.999999999 +0000 UTC)\n",
+	)
+	addDumpTest(
+		panick, // this is a stringer, but the inner member that implements String() string is nil
+		"(spew_test.embeddedTimePtr) (PANIC=runtime error: invalid memory address or nil pointer dereference){\n Time: (*time.Time)(<nil>)\n}\n",
+	)
+	addDumpTest(
+		// redeclared type convertible to time.Time
+		redeclaredTime(ts),
+		"(spew_test.redeclaredTime) 2006-01-02 15:04:05.999999999 +0000 UTC\n",
+	)
+	addDumpTest(
+		// redeclared type convertible to *time.Time
+		//
+		// NOTE: the information about the original (redeclared) type is lost. This is due to
+		// how displaying pointer type information is displayed (i.e. using v.Elem().Type()).
+		rtptr,
+		"(*time.Time)("+tsAddr+")(2006-01-02 15:04:05.999999999 +0000 UTC)\n",
+	)
+}
+
 // TestDump executes all of the tests described by dumpTests.
 func TestDump(t *testing.T) {
 	// Setup tests.
@@ -979,14 +1065,15 @@ func TestDump(t *testing.T) {
 	addPanicDumpTests()
 	addErrorDumpTests()
 	addCgoDumpTests()
+	addTimeDumpTests()
 
 	t.Logf("Running %d tests", len(dumpTests))
 	for i, test := range dumpTests {
 		buf := new(bytes.Buffer)
 		spew.Fdump(buf, test.in)
 		s := buf.String()
 		if testFailed(s, test.wants) {
-			t.Errorf("Dump #%d\n got: %s %s", i, s, stringizeWants(test.wants))
+			t.Errorf("Dump #%d\n  got: %s %s", i, s, stringizeWants(test.wants))
 			continue
 		}
 	}
@@ -1041,3 +1128,8 @@ func TestDumpSortedKeys(t *testing.T) {
 	}
 
 }
+
+func ptr[T any](value T) *T {
+	v := value
+	return &v
+}

internal/spew/format.go

@@ -222,7 +222,7 @@ func (f *formatState) format(v reflect.Value) {
 
 	// Call Stringer/error interfaces if they exist and the handle methods
 	// flag is enabled.
-	if !f.cs.DisableMethods {
+	if !f.cs.DisableMethods || (f.cs.EnableTimeStringer && isTime(v)) { // we consider the case when we want times printed out
 		if (kind != reflect.Invalid) && (kind != reflect.Interface) {
 			if handled := handleMethods(f.cs, f.fs, v); handled {
 				return