Comparing reflect types behaves not as expected

[posener]

When comparing two equal types, the response is false, for example, the following code print false:

t1 := reflect.TypeOf(1)
t2 := reflect.TypeOf(2)
fmt.Println(cmp.Equal(t1, t2, deepAllowUnexported(t1)))

See full code

Just to clarify, if the diff is being printed, this is the reason that those two types are not the same:

{*reflect.rtype}.alg.hash:
	-: (func(unsafe.Pointer, uintptr) uintptr)(0x451530)
	+: (func(unsafe.Pointer, uintptr) uintptr)(0x451530)
{*reflect.rtype}.alg.equal:
	-: (func(unsafe.Pointer, unsafe.Pointer) bool)(0x401a50)
	+: (func(unsafe.Pointer, unsafe.Pointer) bool)(0x401a50)

This is being solved in the standard refelect.DeepEqual when comparing two pointers.

If the compareAny will do the same trick, the above code will print true as expected. It also does not break any of the tests:

diff --git a/cmp/compare.go b/cmp/compare.go
index 1260603..8fbbaa9 100644
--- a/cmp/compare.go
+++ b/cmp/compare.go
@@ -239,6 +239,12 @@ func (s *state) compareAny(vx, vy reflect.Value) {
                        s.report(vx.IsNil() && vy.IsNil(), vx, vy)
                        return
                }
+
+               if vx.Pointer() == vy.Pointer() {
+                       s.report(true, vx, vy)
+                       return
+               }
+

I am not sure why this comparison is not done in the go-cmp implementation, I can't see any harm in doing it, it can also saves running time.

[dsnet]

This is behaving as intended. reflect.Type is a good example of a type that should not be compared by deeply descending into it since it contains state that it not safe to compare.

The user should be explicit about the proper way to compare reflect.Type:

cmp.Comparer(func(x, y reflect.Type) bool { return x == y })

Sometimes pointer comparisons are the right approach, other times the right approach is to descend into the value. reflect.DeepEqual is inconsistent in that it only does pointer comparison if they are identical, otherwise it descends. cmp.Equal chose to always descends into pointers.

We chose the current behavior for the following reasons:

1. Always descending is more expressive, since you could always "opt-out" of it by providing a custom Comparer to do pointer comparison. The opposite is not true.
2. There exists comparisons in Go that are irreflexive (e.g., NaN != NaN or comparisons on function pointers). Thus, performing pointer comparison hides the fact that the underlying values may actually be unequal by other rules of comparison (see this example). Personally, I'm not fond of irreflexive equalities, but they exist and I prefer to keep the number of exceptions lower.

[dsnet]

Here's an example of how an option can be used to obtain the equivalent behavior of reflect.DeepEqual with regard to pointers:

func EquateIdenticalPointers() cmp.Option {
	return cmp.FilterPath(func(p cmp.Path) bool {
		// Filter for pointer kinds only.
		t := p.Last().Type()
		return t != nil && t.Kind() == reflect.Ptr
	}, cmp.FilterValues(func(x, y interface{}) bool {
		// Filter for pointer values that are identical.
		vx := reflect.ValueOf(x)
		vy := reflect.ValueOf(y)
		return vx.IsValid() && vy.IsValid() && vx.Pointer() == vy.Pointer()
	}, cmp.Comparer(func(_, _ interface{}) bool {
		// Consider them equal no matter what.
		return true
	})))
}

Note that this option will conflict with another option that is applicable on *T. See #36 for a proposed solution around that. With #36, you could always wrap inside testify as such:

return cmp.Diff(x, y,
	DeepAllowUnexported(x ,y),
	cmp.FilterOrder(
		cmp.Options(userOpts), // User-provided options take precedence
		EquateIdenticalPointers(),
	),
)

[posener]

I see
Thank you very much for the detailed explanation.
Make sense all that you said here.

One last thing that is not very clear to me.
Can you explain why the comparison failed? the diff looks like they are identical:

{*reflect.rtype}.alg.hash:
	-: (func(unsafe.Pointer, uintptr) uintptr)(0x451530)
	+: (func(unsafe.Pointer, uintptr) uintptr)(0x451530)
{*reflect.rtype}.alg.equal:
	-: (func(unsafe.Pointer, unsafe.Pointer) bool)(0x401a50)
	+: (func(unsafe.Pointer, unsafe.Pointer) bool)(0x401a50)

[dsnet]

Both of those are function pointers. Functions in Go are not comparable (as defined in the language spec). In this situation, cmp.Equal took the same definition as reflect.DeepEqual with regard to function pointers where they are only equal if both are nil, otherwise they are never equal.

Even though both appear to have the same pointer, that is actually an implementation detail of the current Go compiler. For top-level functions, a pointer has a well-defined meaning. However, when you derive a function pointer from a method or closure, the meaning becomes more hazzy.

func MakeClosure() func() int {
    var x int
    return func() {
        x++
        return x
    }
}

f1 := MakeClosure()
f2 := MakeClosure()
f1 == f2 // Suppose this were possible, what should this return?

[prochac]

@dsnet
It is what @matryer is doing here, right?
https://github.com/matryer/is/blob/master/is.go#L257

The simple function pointer works, closure is not.
It kinda makes sense, as it does have different stack bound to it.
https://go.dev/play/p/IcBdFTMhrgT

[dsnet]

That seems highly suspect. See golang/go#18871.

Comparing the reflect.Value directly also compares the reflect.flags type, which is entirely an implementation detail of the "reflect" package.