out-of-the-box support for atomic.Pointer

[pohly]

Suppose a struct contains an atomic.Pointer instead of a normal pointer:

import "sync/atomic"

type A struct {
    Ptr atomic.Pointer[int]
}

Using cmp.Diff for such struct instances fails because the Pointer type contains unexported fields:

panic: cannot handle unexported field at {*main.A}.Ptr.v:
	"sync/atomic".Pointer[int]
consider using a custom Comparer; if you control the implementation of type, you can also consider using an Exporter, AllowUnexported, or cmpopts.IgnoreUnexported

See https://go.dev/play/p/Jp91WpSKFRx for a full example.

It would be nice if cmp.Diff automatically did the same thing for atomic.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 part of the Go standard library.
• Doing it via a Transformer for 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 take address of field for comparison #310 (comment).

[dsnet]

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 #310 (comment) 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.

[pohly]

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 😁

Let's keep this issue open until some solution is implemented? But up to you...

[pohly]

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?

[dsnet]

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()) }

[pohly]

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].

[pohly]

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.

[dsnet]

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.

[pohly]

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.

[dsnet]

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".