Generics in comparison functions?

[glenjamin]

Hello!

I was wondering if you'd considered whether it's worth it to use generics in the comparison functions, and what a migration path to that model might look like?

[dnephin]

Hey Glen! I think it would make sense to start using generics for Equal and DeepEqual at some point. I'm not sure if there are any other functions that would benefit from generics. Maybe Assert and Check could use it as well.

I'm hoping it'll be a backwards compatible change. Any assertion that uses different types for Equal or DeepEqual would already be failing, so I think it might be ok to start using generics without a new major version. I've been waiting a bit for everyone to upgrade their Go version to at least 1.18 so that nothing breaks. I guess with the release of go1.20 everyone should be upgraded. We can drop Go1.17 from CI when we add Go1.20 and try using some generic arguments in Equal and DeepEqual.

I guess the signatures would look like this:

func Equal[X any](t TestingT, x X, y X, msgAndArgs ...any) { ... }

func DeepEqual[X any](t TestingT, x X, y X, opts ...gocmp.Option) { ... }

I think it's only the exported function signatures that need to change so that the compiler can check the types match. Otherwise I think all the internals probably don't need to change.

What do you think? Are there other functions that might benefit from generics? Do you know of any potential problems with changing Equal and DeepEqual without releasing a new major version?

A little while ago I added a property check that used generics here: https://github.com/gotestyourself/gotest.tools/blob/main/x/generics/property/complete.go#L60-L73, I couldn't quite get the module alias working, so I don't think it's importable yet. Once the main package is using generics I think I'll move that to x/property

[glenjamin]

That's great! This pretty much aligns with what I was thinking - I think possibly it'd be nice if .Contains and .Len could be made generic, but I don't think Go's generics are flexible enough to allow string/array/slice/map in the same signature.

I don't know whether it's worth adding something like cmp.ListContains and cmp.HasKey which could be typesafe on collections and their members.

The case I ran into recently that prompted this was incorrectly comparing a struct with a pointer to the same struct type - which isn't particularly a massive pain as the diff does include the (rather subtle) &.

Given the relative stability of this package it's probably not worth using build tags to continue to provide backwards compatibility for projects using older Go versions that want to continue to update assert.

[dnephin]

Nice! I like the idea of type-safe implementations like cmp.ListContains (maybe cmp.SliceContains and document that it also accepts arrays, since arrays tend to be much less common?), and cmp.HasKey ! And I guess we'd want one for cmp.StringContains as well? I also don't know if there's any way to represent a generic constraint of "is slice, map, or string".

[dnephin]

I put together #255 which adds generics to the main assertions, if you'd like to check it out.

For Contains and Len, how often do you use those to check for an empty slice vs checking for a non-zero number of items? I don't use either myself, I generally do this:

// strings
assert.Assert(t, strings.Contains(actual, expected), actual)

// slices or maps
assert.DeepEqual(t, actual, expected, cmpSomething)

// slice or map empty
assert.Assert(t, len(actual), 0, "got: %v", actual)

For slices and maps, I always prefer DeepEqual instead of a Len or Contains. I'll use comparison options to limit the fields that are compared, sometimes doing a shallow comparison of only a few fields (ex: an primary key ID).