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

Add cmpopts.FilterValue

Open dsnet opened this issue 4 years ago • 4 comments

cmp.FilterValues expects a function of the signature func(T, T) bool where the function is called with values from both the left argument and the right argument.

I propose the addition of cmpopts.FilterValue that is semantically equivalent to:

func FilterValue[T any](f func(T) bool) cmp.Option {
    return cmp.FilterValues(func(x, y T) bool {
        return f(x) && f(y)
    })
}

The current cmp.FilterValues is more powerful as you can make a filtering decision based on information from both values, but most usages of it want the logical AND of a single-argument predicate function. In fact, all of the usages of cmp.FilterValues in cmpopts would prefer to use a single-argument predicate function. Examples:

  • https://github.com/google/go-cmp/blob/6faefd0594fae82639a62c23f0aed1451509dcc0/cmp/cmpopts/equate.go#L26-L31
  • https://github.com/google/go-cmp/blob/6faefd0594fae82639a62c23f0aed1451509dcc0/cmp/cmpopts/equate.go#L60-L62
  • https://github.com/google/go-cmp/blob/6faefd0594fae82639a62c23f0aed1451509dcc0/cmp/cmpopts/equate.go#L85-L87
  • https://github.com/google/go-cmp/blob/6faefd0594fae82639a62c23f0aed1451509dcc0/cmp/cmpopts/equate.go#L104-L106
  • https://github.com/google/go-cmp/blob/6faefd0594fae82639a62c23f0aed1451509dcc0/cmp/cmpopts/equate.go#L144-L148

Alternatively, we could modify cmp.FilterValues to also accept a function of the form func(T) bool where it effectively performs the AND of the predicate function applies to both arguments.

\cc @neild

dsnet avatar Oct 18 '21 22:10 dsnet

Alternatively, we could modify cmp.FilterValues to also accept a function of the form func(T) bool where it effectively performs the AND of the predicate function applies to both arguments.

This seems simpler for the user and avoids introducing yet another place where people need to remember whether a function is in cmp or in cmpopts. Any reason not to do it this way?

neild avatar Oct 18 '21 23:10 neild

Any reason not to do it this way?

I don't have a strong preference of the approach taken.

This might be a pie-in-the-sky goal, but it'd be nice to switch the API to use generics where sensible.

Thus, today's cmp.FilterValues would be written as:

func FilterValues[T any](func(T, T) bool) Option

If we were make it accept either func(T) bool or func(T, T) bool, I'm not sure how to do that with generics.

If we had golang/go#41716, I think we can do something like:

func FilterValues[T any](interface{ type func(T) bool, func(T, T) bool }) Option

where the argument is effectively a sum-type of either func(T) bool or func(T, T) bool.

avoids introducing yet another place where people need to remember whether a function is in cmp or in cmpopts

If/when golang/go#48305 gets accepted, I think we should merge cmpopts into cmp as there'll be sufficient godoc features to better control the presentation of the documentation in a way that's clean.

dsnet avatar Oct 19 '21 00:10 dsnet

It seems that we can do this with the following signature:

func FilterValues[T any, F interface{ func(T) bool | func(T, T) bool }](f F) Option

dsnet avatar Dec 09 '21 18:12 dsnet

I realized that we can't convert our existing functions to generics in a backwards compatible way.

Consider the following user code:

func WrapTransformer(fn interface{}) cmp.Option {
     return cmp.FilterPath(..., cmp.Transformer("", fn))
}

The fn passed in is a interface{} and we've lost all information about the original type. If we switch Transformer to be generic, then this won't build since we can't statically determine the type at compile time without rewriting all wrapper options in existence to also be generic (ain't gonna happen).

dsnet avatar Dec 09 '21 21:12 dsnet