google / go-cmp

Package for comparing Go values in tests
BSD 3-Clause "New" or "Revised" License
4.24k stars 209 forks source link

Comparing contravariant types doesn't work as expected using Transformer #261

Open tamird opened 3 years ago

tamird commented 3 years ago
package main

import (
    "fmt"

    "github.com/google/go-cmp/cmp"
)

type intHolder struct {
    i int
}

func main() {
    got := []intHolder{
        {i: 1},
    }
    want := []int{1}
    fmt.Println(cmp.Diff(want, got, cmp.Transformer("myTransformer", func(h intHolder) int {
        return h.i
    })))
}

produces:

  interface{}(
-   []int{1},
+   []main.intHolder{{i: 1}},
  )

https://play.golang.org/p/GQqAZmbYUD8

Digging into the implementation revealed effectively that Transform is skipped whenever the types being compared aren't identical.

The same thing happens when got and want are scalars rather than slices: https://play.golang.org/p/r2O2UKYRD-r

dsnet commented 3 years ago

In Go, a []T and []R are invariant types (i.e., []T is not a sub-type of []R, nor is []R a sub-type of []T assuming T is not the same type as R). See https://blog.merovius.de/2018/06/03/why-doesnt-go-have-variance-in.html

Given that []T and []R are invariant, the current behavior seems expected.

Note that cmp determined []int and []main.intHolder to be different values since the types are different before the cmp.Transformer even has a chance to trigger. Even if we could get past variance issues with slices, the cmp.Transformer still wouldn't trigger since it only does so if both types being compared are the same. See https://play.golang.org/p/toWjEf2zabm

dsnet commented 3 years ago

If you want to operate on both []int and []main.intHolder, you would need a transformer that operates on either of those two types and coverts it a []int.

See https://play.golang.org/p/iRvh9tTJk_q for example.

tamird commented 3 years ago

Sorry, of course - invariant is the term.

Thanks for the example! That works for mapping both slice types to a single slice type. It would be nice to be able to do that using a transformer that operates on just an element. That's not possible, is it?

tamird commented 3 years ago

Context: https://fuchsia-review.googlesource.com/c/fuchsia/+/532541

dsnet commented 3 years ago

It would be nice to be able to do that using a transformer that operates on just an element. That's not possible, is it?

Perhaps. The current semantic is that transformers only apply if both values are the same type, and deviating from that would probably be a breaking change.

Futhermore, I imagine there are thorny issues that would arise from an assymetric application of transformers. For example, the reporter indicates the use of transformers with a Inverse(funcName, ...) call (example). I don't know how an asymmetric application of transformers would be adequately represented in the diff.