go-cmp icon indicating copy to clipboard operation
go-cmp copied to clipboard

take address of field for comparison

Open elagergren-spideroak opened this issue 3 years ago • 4 comments

I'm using intsets.Sparse as a field in a struct:

type T struct {
   v intsets.Sparse
   // other fields
}

When traversing T, the field v is copied, causing all methods to fail. This precludes the use of Transformer.

Ideally, there would be an Option that would take the address of the field for use with other Options. Notably, Sparse has an Equals(*Sparse) bool method, so perhaps an alternate idea is to allow different equality methods? But I'm not sure whether go-cmp will take the address of a field in order to invoke the Equal method.

elagergren-spideroak avatar Oct 25 '22 21:10 elagergren-spideroak

Is there a reason this was a closed? I think it's fairly reasonable.

dsnet avatar Oct 25 '22 21:10 dsnet

@dsnet I closed it because my complaint about the diff was invalid, but I suppose the rest of it still stands. Will reopen.

elagergren-spideroak avatar Oct 25 '22 21:10 elagergren-spideroak

The fact that values are not always treated as addressable is my biggest regret for the cmp package. I wish it had treated all values as addressable.

I'm sympathetic to adding a fundamental option like:

// ForceAddressable treats all values as if they were addressable.
func ForceAddressable() Option

but would also like to see if it could be enabled by default in a v1.0.0 release of the package.

As a workaround, you can do something like:

// MakeAddressable takes the address of values of T,
// so that methods or options that operate on *T become applicable.
func MakeAddressable[T any]() cmp.Option {
	return cmpopts.AcyclicTransformer("Addr", func(v T) *T {
		return &v
	})
}

and perhaps that's also something we should add to the cmpopts package.

Example usage of MakeAddressable: https://go.dev/play/p/DR371K17bn6

dsnet avatar Oct 25 '22 21:10 dsnet

A similar problem occurs with a struct that contains a atomic.Pointer. I'd like to use a transformer to compare the element that is pointed to instead of the atomic.Pointer value (always different). The transformer itself works:

		cmp.Transformer("podPointer", func(p atomic.Pointer[v1.Pod]) *v1.Pod {
			return p.Load()
		})

But "go vet" (correctly) complains:

func passes lock by value: sync/atomic.Pointer[k8s.io/kubernetes/vendor/k8s.io/api/core/v1.Pod] contains sync/atomic.noCopy

This doesn't work:

		cmp.Transformer("podPointer", func(p *atomic.Pointer[v1.Pod]) *v1.Pod {
			return p.Load()
		})

cmp then doesn't apply the transformation, leading to the original problem:

panic: cannot handle unexported field at {*framework.NodeInfo}.Pods[1].Pod.v:
	"sync/atomic".Pointer[k8s.io/api/core/v1.Pod]
consider using a custom Comparer; if you control the implementation of type, you can also consider using an Exporter, AllowUnexported, or cmpopts.IgnoreUnexported [recovered]

Any suggestions?

pohly avatar Feb 14 '23 20:02 pohly