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

checkNilCase test helper function invalid logic #503

Closed
cnnrrss opened this issue Mar 1, 2022 · 1 comment
Closed

checkNilCase test helper function invalid logic #503

cnnrrss opened this issue Mar 1, 2022 · 1 comment

Comments

@cnnrrss
Copy link
Contributor

cnnrrss commented Mar 1, 2022

During my work on issue 491 I noticed that the logic of the test helper function checkNilCase is invalid because the function arguments a and b will never be nil.

This is a common bug in go where an interface value that holds a nil value is itself non-nil. This is mentioned in the tour of go.

I created a simplified demonstration in the playground https://go.dev/play/p/lNyfmdEpOnE.

Possible solutions to this problem include:

  1. Remove the checkNilCase helper function. It's currently not doing anything so it would be safe to delete. If this is the desired approach, this PR could be merged: Fix remove checkNilCase test helper function #504
  2. Never use (*T)(nil) as an interface{}, and instead do the check before. This would mean duplicating the checkNilCase function logic into separate functions for any type that would need it.
  3. Use a popular third party library such as https://pkg.go.dev/github.com/google/go-cmp/cmp
  4. Use reflection reflect.ValueOf(a).IsNil() (not recommended)
@pavelnikolov
Copy link
Member

pavelnikolov commented Mar 1, 2022

I don't like 3. because I don't want to add dependencies to this library.
Another possible solution is to wait for the generics in go v1.18 and use that instead.
The PR is merged. Thank you for your contribution.

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

2 participants