Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: Use generics to add type safety support to is.Equal #52

Closed
ghost opened this issue Jan 19, 2023 · 10 comments
Closed

Proposal: Use generics to add type safety support to is.Equal #52

ghost opened this issue Jan 19, 2023 · 10 comments

Comments

@ghost
Copy link

ghost commented Jan 19, 2023

I think it is quite common to got caught into pitfalls getting errors like "unit8(0) != int(0)" (by calling is.Equal with an untyped constant for example).

Now that Go has added generics support, it is easy to add type safety support to equal, like:

func (is *I) EqualSafe[T any](a, b T) {
    is.Helper()
    return is.Equal(a, b)
}
@breml
Copy link
Contributor

breml commented Jan 19, 2023

@leafbebop in general, I like this proposal. I think, this could be a reasonable addition to the existing API.

For the name of this function, I would like to share the following proposals:

  • EqualOf: if I remember correctly, the suffix Of has been used/proposed for usage in the stdlib, when a function/method with the same name (without generics) already exists.
  • TypeSafeEqual or SafeEqual: might be easier to use in IDE with code completion, because already the first letter gives you the correct option

@walles
Copy link

walles commented Feb 24, 2023

... or just release v2.0.0 and make the existing Equal generic.

@matryer
Copy link
Owner

matryer commented Mar 13, 2023

@flowchartsman What do you think?

@flowchartsman
Copy link
Collaborator

flowchartsman commented Mar 13, 2023

This won't work as-is. Go generics do not support parameterized methods, so the above will not compile. You could try to write a package global method with a generic signature like
func EqualGeneric[T any](a,b T) bool, but you would then have to call it using the package, which would mean abandoning the convention of is := is.New(), since this otherwise shadows the package namespace with the identifier. This gets you something like:

var (
	a int
	b uint8
)
isVal := is.New(t)
isVal.True(is.EqualGeneric(a,b))
// error: type uint8 of b does not match inferred type int for T 

And that error message is also not particularly clear, since it's primarily a type inference error regarding the generic function itself, which is confusing since there's no T in this code. It is more clear with an explicit type parameter to the function, but then you end up with something like:

var (
    a int
    b uint8
)
isVal := is.New(t)
isVal.True(is.EqualGeneric[int](a,b))
// output: cannot use b (variable of type uint8) as int value in argument to is.EqualGeneric[int]

Ref: https://go.dev/play/p/tKb-6wvHZbx

Given all that, I'm not sure using type parameters really gets us what we want here.

@matryer
Copy link
Owner

matryer commented Mar 13, 2023

what if we just added a method alongside the other? Could we tolerate is.EqualG[string](s1, s2)?

@flowchartsman
Copy link
Collaborator

flowchartsman commented Mar 13, 2023

what if we just added a method alongside the other? Could we tolerate is.EqualG[string](s1, s2)?

It still can't be a method, with or without type inference. You'd need to make it a global, type-parameterized function in the is package, breaking the naming convention like I mentioned above, or make the I type itself generic, and instantiate a new one for every type you wanted to compare (which, would also mean never calling the I value is, and would also mean you couldn't re-use it, since they'd be different types after the short assignment).

@walles
Copy link

walles commented Mar 13, 2023

Related language proposal: golang/go#49085

@flowchartsman
Copy link
Collaborator

flowchartsman commented Mar 13, 2023

Related language proposal: golang/go#49085

Yah, I wouldn't hold my breath for that one, much as I would love it, since it seems to be relatively difficult to solve in a go-y way.

At this time, AFAICT, the only way to do this in a way that looks how the package is used now is to go all-in on global state, and have package-level parameterized functions in place of methods on instances of the I type, but this is a very radical change to make just to get the compiler to tell you that one thing is an int and another is a uint8.

@breml
Copy link
Contributor

breml commented Mar 13, 2023

At this time, AFAICT, the only way to do this in a way that looks how the package is used now is to go all-in on global state, and have package-level parameterized functions in place of methods on instances of the I type, but this is a very radical change to make just to get the compiler to tell you that one thing is an int and another is a uint8.

Even with all the sympathy I have with the original idea of this proposal, I feel that changing the API of the is package in such a dramatic way is not reasonable. This would justify the creation of a completely separate package with its own API style.

@flowchartsman
Copy link
Collaborator

flowchartsman commented May 9, 2023

Given that this proposal can't work as-is (no generic methods), and given that fully embracing generics would be a big enough change to merit an entirely new version (if not a new module), I think we're gonna have to close this issue, but thanks for the submission, and stay tuned!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants