Open pohly opened 1 year ago
At present, cmp
has no special support for any type in the Go standard library. The original reason why cmp
was invented was because Go 1.9 introduced monotonic clock readings in time.Time
, which broke thousands of tests that relied on t1 == t2
being semantically identical to t1.Equal(t2)
. To fix this, we never gave special meaning to time.Time
, but rather gave special meaning to types that have an Equal
method since this was more versatile. Similarly, I don't think we should give special support for the atomic
types. Perhaps they should gain a Equal
method instead? This would be in line with other proposed methods to add (see golang/go#54582).
Regarding the inability to properly write a transformer for this, I've stated in https://github.com/google/go-cmp/issues/310#issuecomment-1291174594 that the inability of cmp
to take the address of things by default is my greatest regret in how cmp
came about. I feel strongly that we should do something to fix #310, which would at least make it possible to write a transformer to safely perform the comparison yourself.
Both of these other alternatives sound better to me, in particular adding Equal
because it wouldn't need changes in apps. I might try to drive adding atomic/sync.Equal
but if someone else beats me to it, then I certainly wouldn't mind :grin:
Let's keep this issue open until some solution is implemented? But up to you...
After thinking about Equal
a bit more, I am not sure how atomic/sync
could implement it in a way that would match how cmp
handles plain pointers. What sync.Pointer
can compare is the address that is being pointed to (not the same as cmp
, not useful). It could call Equal
on the items being pointed to, but only if implemented (less flexible than cmp
).
Also, if false
is returned, how would cmp
produce a useful diff without access to the two items?
You could imagine the following being added:
package atomic
func (x *Int64) Equal(y *Int64) bool { return x.Load() == y.Load() }
func (x *Int64) String() string { return fmt.Sprint(x.Load()) }
I suspect fmt.Sprint
will lead to very unreadable formatting of more complex types and not work well in combination with cmp.Diff
- think of atomic.Pointer[SomeStruct]
, not atomic.Pointer[int]
.
Also, x.Load() == y.Load()
would compare the pointers for atomic.Pointer[T any]
, not the T
values that are being pointed to. *x.Load() == *y.Load()
wouldn't compile because T
is not required to be comparable.
In the case of atomic.Pointer
, it is unclear whether atomic.Pointer.Equal
should be a pointer comparison or deep comparison. Different use-cases are going to reasonably want different behavior. This perhaps suggests that the atomic
types should not have an Equal
method and further suggests that cmp
should not have default comparison behavior for atomic
types since the default equality behavior is ill-specified.
Regardless of what we do here, the general philosophy of cmp
is to not have specialized behavior of types, but to off-load customized behavior to user-provided cmp.Transformer
options.
Of course, this still requires #310 be addressed, which is a prerequisite.
Doesn't cmp.Diff
already have an opinion about what equality of plain pointer fields means (compare nil vs non-nil, then what's being pointed to)? Doing the same for atomic.Pointer
IMHO would make sense from a consistency perspective. The same with atomic.Int64
- I'd expect that to be treated like an int64
.
But that's just my two cents. Addressing #310 is the more general solution.
Doesn't cmp.Diff already have an opinion about what equality of plain pointer fields means (compare nil vs non-nil, then what's being pointed to)?
Perhaps, but I still stand by "the general philosophy of cmp is to not have specialized behavior of types".
Suppose a struct contains an atomic.Pointer instead of a normal pointer:
Using
cmp.Diff
for such struct instances fails because thePointer
type contains unexported fields:See https://go.dev/play/p/Jp91WpSKFRx for a full example.
It would be nice if
cmp.Diff
automatically did the same thing foratomic.Pointer
as it does for normal pointers: compare the value that is being pointed to.The argument for such special support is two-fold:
atomic.Pointer
is impossible without copying that value, which correctly fails the "go vet"copylocks
check. See https://github.com/kubernetes/kubernetes/pull/115777/files#diff-9b169e646cb953f34c0362a885d844daea86f5bef52012cb3af974f176bb281f and https://github.com/google/go-cmp/issues/310#issuecomment-1430338479.