proposal: reflect: add support for `Equal(T) bool` method in `DeepEqual()` #20444

pierrre · 2017-05-21T23:01:03Z

Currently reflect.DeepEqual() compares values very simply.
It causes issues with types that implement their own equality logic, like time.Time.
I propose to add a check for types that have a Equal(T) bool method.
I think it's better to call the Equal(T) bool method if it's implemented, rather than relying on the default implementation.
Several types could benefit of this change (see https://golang.org/search?q=Equal ):

time.Time
net.IP
regexp/syntax.Regexp

The change is very simple:
Add

if eqMet := v1.MethodByName("Equal"); eqMet.IsValid() {
	eqTyp := eqMet.Type()
	if eqTyp.NumIn() == 1 && eqTyp.In(0) == v2.Type() && eqTyp.NumOut() == 1 && eqTyp.Out(0) == TypeOf(true) {
		return eqMet.Call([]Value{v2})[0].Interface().(bool)
	}
}

before the large switch v1.Kind() block.
(I'm not very familiar with the reflect package, so my code may contain errors).

The text was updated successfully, but these errors were encountered:

mvdan · 2017-05-22T10:17:21Z

I assume this is a proposal? If so, please use a Proposal: prefix in the title. The change seems big enough to warrant one.

I'm concerned about the case where an underlying type implements Equal, but you don't want to use its method. For example, if you're using DeepEqual on a tree and there's a type defined by an imported package somewhere in there, but it doesn't do what you want.

In general it seems like a good idea, though.

pierrre · 2017-05-22T10:22:34Z

I assume this is a proposal? If so, please use a Proposal: prefix in the title. The change seems big enough to warrant one.

done

I'm concerned about the case where an underlying type implements Equal, but you don't want to use its method. For example, if you're using DeepEqual on a tree and there's a type defined by an imported package somewhere in there, but it doesn't do what you want.

I think that if a method Equal(T) bool is defined (where T is the same type as the current type), it is always better to use it.

dsnet · 2017-05-22T17:55:07Z

I'm opposed to adding this functionality to reflect.DeepEqual.

I understand that this is probably solving a real problem. For example, many usages of reflect.DeepEqual are probably going to break with the addition of monotonic times in Go1.9. In fact, I deal with broken uses of reflect.DeepEqual quite a lot in my day-to-day job and my initial reaction was to propose exactly something like this.

However, these are why I don't believe DeepEqual is the right place to make this change.

Add this functionality is itself a breaking change on DeepEqual. The documentation is explicit that it is functionally a recursive relaxation of the == operator in the Go language. Making DeepEqual understand the semantics of a Equal method goes against this.
It can be unsafe to do this. DeepEqual blindly compares both exported and unexported fields. In order for DeepEqual to be able to use an Equal method on an unexported field, it must use unsafe to forcibly retrieve the value of an unexported field (since the reflect package does not allow you to retrieve the interface value of an unexported field). Calling the Equal on hidden fields of a object is pretty fishy.

In my opinion, the right solution is an entirely new API (possibly a new package), that provides that equality for use in tests. Not only should this new equality function call Equal, but it should allow the user to specify their own custom equality function to override the behavior of certain types.

pierrre · 2017-05-22T20:38:22Z

I was reading the source code, in order to (maybe) write my own package, and I found something strange.
https://github.com/golang/go/blob/master/src/reflect/deepequal.go#L119
Why aren't val1 and val2 re-used in the deepValueEqual() call ?

mvdan · 2017-05-22T20:42:05Z

@pierrre it looks like you could indeed simplify it - just send a CL. If it's like that for some reason, there should be a comment clarifying. And any person who can approve the CL will let you know if the change is wrong.

pierrre · 2017-05-23T14:42:34Z

FYI image.Rectangle uses .Eq(), not .Equal().
Does someone know similar cases ?

dsnet · 2017-05-23T15:45:05Z

Other exceptions:

Protocol buffers are also not safe to compare using reflect.DeepEqual, but do not (cannot) have Equal method. Instead you are supposed to using proto.Equal.
The signature for an equal method is not always (T) Equal(T) bool. I have seen cases where it is (T) Equal(I) bool where I is an interface of which T satisfies.

pierrre · 2017-05-24T19:53:28Z

@dsnet I just understood what you mean by

It can be unsafe to do this. DeepEqual blindly compares both exported and unexported fields. In order for DeepEqual to be able to use an Equal method on an unexported field, it must use unsafe to forcibly retrieve the value of an unexported field (since the reflect package does not allow you to retrieve the interface value of an unexported field). Calling the Equal on hidden fields of a object is pretty fishy.

(Sorry, I'm not a Golang reflect expert)

https://golang.org/pkg/reflect/#Value.Call documentation isn't very clear about unexported fields.
(like https://golang.org/pkg/reflect/#Value.Interface )

I'm withdrawing my proposal and closing the issue.

Maybe I'll write my own library...

pierrre · 2017-07-11T20:55:15Z

For anyone interested, I've built this https://github.com/pierrre/compare

mvdan · 2017-07-11T20:59:13Z

I believe this was built simultaneously: https://github.com/google/go-cmp

pierrre changed the title ~~reflect: add support for Equal(T) bool method in DeepEqual()~~ proposal: reflect: add support for Equal(T) bool method in DeepEqual() May 22, 2017

gopherbot added this to the Proposal milestone May 22, 2017

gopherbot added the Proposal label May 22, 2017

pierrre closed this as completed May 24, 2017

This was referenced May 28, 2017

Add using 'Equal' method of structure to test equality stretchr/testify#394

Closed

proposal: Add new API to package reflect for checking equality of values #20519

Closed

ianlancetaylor mentioned this issue Jun 8, 2017

time: TZ=UTC does not cause Location field to be nil #19202

Closed

golang locked and limited conversation to collaborators Jul 11, 2018

gopherbot added the FrozenDueToAge label Jul 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

proposal: reflect: add support for `Equal(T) bool` method in `DeepEqual()` #20444

proposal: reflect: add support for `Equal(T) bool` method in `DeepEqual()` #20444

pierrre commented May 21, 2017

mvdan commented May 22, 2017

pierrre commented May 22, 2017

dsnet commented May 22, 2017

pierrre commented May 22, 2017

mvdan commented May 22, 2017

pierrre commented May 23, 2017

dsnet commented May 23, 2017 •

edited

Loading

pierrre commented May 24, 2017

pierrre commented Jul 11, 2017

mvdan commented Jul 11, 2017

proposal: reflect: add support for Equal(T) bool method in DeepEqual() #20444

proposal: reflect: add support for Equal(T) bool method in DeepEqual() #20444

Comments

pierrre commented May 21, 2017

mvdan commented May 22, 2017

pierrre commented May 22, 2017

dsnet commented May 22, 2017

pierrre commented May 22, 2017

mvdan commented May 22, 2017

pierrre commented May 23, 2017

dsnet commented May 23, 2017 • edited Loading

pierrre commented May 24, 2017

pierrre commented Jul 11, 2017

mvdan commented Jul 11, 2017

proposal: reflect: add support for `Equal(T) bool` method in `DeepEqual()` #20444

proposal: reflect: add support for `Equal(T) bool` method in `DeepEqual()` #20444

dsnet commented May 23, 2017 •

edited

Loading