Fix cycle detection in internal/value.Format

cmp/internal/value/format.go

@@ -26,7 +26,7 @@ func Format(v reflect.Value, conf FormatConfig) string {
 	conf.printType = true
 	conf.followPointers = true
 	conf.realPointers = true
-	return formatAny(v, conf, nil)
+	return formatAny(v, conf, visited{})
 }
 
 type FormatConfig struct {
@@ -37,7 +37,7 @@ type FormatConfig struct {
 	realPointers       bool // Should we print the real address of pointers?
 }
 
-func formatAny(v reflect.Value, conf FormatConfig, visited map[uintptr]bool) string {
+func formatAny(v reflect.Value, conf FormatConfig, m visited) string {
 	// TODO: Should this be a multi-line printout in certain situations?
 
 	if !v.IsValid() {
@@ -79,38 +79,42 @@ func formatAny(v reflect.Value, conf FormatConfig, visited map[uintptr]bool) str
 			}
 			return "<nil>"
 		}
-		if visited[v.Pointer()] || !conf.followPointers {
+		if m.Visit(v) || !conf.followPointers {
 			return formatPointer(v, conf)
 		}
-		visited = insertPointer(visited, v.Pointer())
-		return "&" + formatAny(v.Elem(), conf, visited)
+		return "&" + formatAny(v.Elem(), conf, m)
 	case reflect.Interface:
 		if v.IsNil() {
 			if conf.printType {
 				return fmt.Sprintf("%v(nil)", v.Type())
 			}
 			return "<nil>"
 		}
-		return formatAny(v.Elem(), conf, visited)
+		return formatAny(v.Elem(), conf, m)
 	case reflect.Slice:
 		if v.IsNil() {
 			if conf.printType {
 				return fmt.Sprintf("%v(nil)", v.Type())
 			}
 			return "<nil>"
 		}
-		if visited[v.Pointer()] {
-			return formatPointer(v, conf)
-		}
-		visited = insertPointer(visited, v.Pointer())
 		fallthrough
 	case reflect.Array:
 		var ss []string
 		subConf := conf
 		subConf.printType = v.Type().Elem().Kind() == reflect.Interface
 		for i := 0; i < v.Len(); i++ {
-			s := formatAny(v.Index(i), subConf, visited)
-			ss = append(ss, s)
+			vi := v.Index(i)
+			if vi.CanAddr() { // Check for recursive elements
+				p := vi.Addr()
+				if m.Visit(p) {
+					subConf := conf
+					subConf.printType = true
+					ss = append(ss, "*"+formatPointer(p, subConf))
+					continue
+				}
+			}
+			ss = append(ss, formatAny(vi, subConf, m))
 		}
 		s := fmt.Sprintf("{%s}", strings.Join(ss, ", "))
 		if conf.printType {
@@ -124,19 +128,18 @@ func formatAny(v reflect.Value, conf FormatConfig, visited map[uintptr]bool) str
 			}
 			return "<nil>"
 		}
-		if visited[v.Pointer()] {
+		if m.Visit(v) {
 			return formatPointer(v, conf)
 		}
-		visited = insertPointer(visited, v.Pointer())
 
 		var ss []string
 		keyConf, valConf := conf, conf
 		keyConf.printType = v.Type().Key().Kind() == reflect.Interface
 		keyConf.followPointers = false
 		valConf.printType = v.Type().Elem().Kind() == reflect.Interface
 		for _, k := range SortKeys(v.MapKeys()) {
-			sk := formatAny(k, keyConf, visited)
-			sv := formatAny(v.MapIndex(k), valConf, visited)
+			sk := formatAny(k, keyConf, m)
+			sv := formatAny(v.MapIndex(k), valConf, m)
 			ss = append(ss, fmt.Sprintf("%s: %s", sk, sv))
 		}
 		s := fmt.Sprintf("{%s}", strings.Join(ss, ", "))
@@ -155,7 +158,7 @@ func formatAny(v reflect.Value, conf FormatConfig, visited map[uintptr]bool) str
 			}
 			name := v.Type().Field(i).Name
 			subConf.UseStringer = conf.UseStringer
-			s := formatAny(vv, subConf, visited)
+			s := formatAny(vv, subConf, m)
 			ss = append(ss, fmt.Sprintf("%s: %s", name, s))
 		}
 		s := fmt.Sprintf("{%s}", strings.Join(ss, ", "))
@@ -229,15 +232,6 @@ func formatHex(u uint64) string {
 	return fmt.Sprintf(f, u)
 }
 
-// insertPointer insert p into m, allocating m if necessary.
-func insertPointer(m map[uintptr]bool, p uintptr) map[uintptr]bool {
-	if m == nil {
-		m = make(map[uintptr]bool)
-	}
-	m[p] = true
-	return m
-}
-
 // isZero reports whether v is the zero value.
 // This does not rely on Interface and so can be used on unexported fields.
 func isZero(v reflect.Value) bool {
@@ -275,3 +269,12 @@ func isZero(v reflect.Value) bool {
 	}
 	return false
 }
+
+type visited map[Pointer]bool
+
+func (m visited) Visit(v reflect.Value) bool {
+	p := PointerOf(v)
+	visited := m[p]
+	m[p] = true
+	return visited
+}

cmp/internal/value/format_test.go

@@ -54,7 +54,7 @@ func TestFormat(t *testing.T) {
 			a[0] = a
 			return a
 		}(),
-		want: "[]interface {}{([]interface {})(0x00)}",
+		want: "[]interface {}{[]interface {}{*(*interface {})(0x00)}}",
 	}, {
 		in: func() interface{} {
 			type A *A
@@ -85,7 +85,7 @@ func TestFormat(t *testing.T) {
 		// ensure the format logic does not depend on read-write access
 		// to the reflect.Value.
 		v := reflect.ValueOf(struct{ x interface{} }{tt.in}).Field(0)
-		got := formatAny(v, FormatConfig{UseStringer: true, printType: true, followPointers: true}, nil)
+		got := formatAny(v, FormatConfig{UseStringer: true, printType: true, followPointers: true}, visited{})
 		if got != tt.want {
 			t.Errorf("test %d, Format():\ngot  %q\nwant %q", i, got, tt.want)
 		}

cmp/internal/value/pointer_purego.go

@@ -0,0 +1,23 @@
+// Copyright 2018, The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE.md file.
+
+// +build purego
+
+package value
+
+import "reflect"
+
+// Pointer is an opaque typed pointer and is guaranteed to be comparable.
+type Pointer struct {
+	p uintptr
+	t reflect.Type
+}
+
+// PointerOf returns a Pointer from v, which must be a
+// reflect.Ptr, reflect.Slice, or reflect.Map.
+func PointerOf(v reflect.Value) Pointer {
+	// NOTE: Storing a pointer as an uintptr is technically incorrect as it
+	// assumes that the GC implementation does not use a moving collector.
+	return Pointer{v.Pointer(), v.Type()}
+}

cmp/internal/value/pointer_unsafe.go

@@ -0,0 +1,26 @@
+// Copyright 2018, The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE.md file.
+
+// +build !purego
+
+package value
+
+import (
+	"reflect"
+	"unsafe"
+)
+
+// Pointer is an opaque typed pointer and is guaranteed to be comparable.
+type Pointer struct {
+	p unsafe.Pointer
+	t reflect.Type
+}
+
+// PointerOf returns a Pointer from v, which must be a
+// reflect.Ptr, reflect.Slice, or reflect.Map.
+func PointerOf(v reflect.Value) Pointer {
+	// The proper representation of a pointer is unsafe.Pointer,
+	// which is necessary if the GC ever uses a moving collector.
+	return Pointer{unsafe.Pointer(v.Pointer()), v.Type()}
+}