Fix cycle detection in internal/value.Format

The cycle detection in value.Format uses a map of uintptrs, which is not
correct as this assumes that the Go GC never uses a moving collector.
Instead, we should be using unsafe.Pointer, which the GC knows how to scan.

Also, push pointer checking of slices down to the level of individual elements.
We do this because the slice pointer is not sufficient to determine equality.
For example, v[:0] and v[:1] both have the same slice pointer, but are
clearly different values.

Since the purego environment forbids the use of unsafe, create a Pointer type
that is an abstraction over an opaque pointer. The Pointer type can only be
compared. In the purego, continue to use uintptr, which is the best we can do.

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,25 @@
+// 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()}
+}