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

A regression test for a brittle Equal method. #62

Closed
wants to merge 1 commit into from

Conversation

filmil
Copy link
Member

@filmil filmil commented Dec 21, 2017

This seems to me to demonstrate that the contract for the Equal method should
either be precisely specified, or that go-cmp should be fixed to ensure that
it behaves sensibly when encountered with nils.

This seems to me to demonstrate that the contract for the Equal method should
either be precisely specified, or that go-cmp should be fixed to ensure that
it behaves sensibly when encountered with nils.
@dsnet
Copy link
Collaborator

dsnet commented Dec 21, 2017

Anytime you indirect a pointer you should either:

  • Check whether the pointer is nil
  • Verify that the code leading up to that point guarantees that the pointer is set
  • Or decide that a panic is exactly what you want.

For an Equal method there is a sensible definition of what should happen:

func (x *foo) Equal(y *foo) bool {
	if x == nil || y == nil {
		return x == nil && y == nil
	}
	return x.value == y.value
}

Thus, I argue that this is a bug in the definition of Equal.

Alternatively, the Equal method could be defined on the non-pointer receiver version. The time.Time type uses this pattern.

func (x foo) Equal(y foo) bool {
	return x.value == y.value
}

@dsnet
Copy link
Collaborator

dsnet commented Dec 21, 2017

Closing this as it seems this was demonstrate the problem with the associated issue.

@dsnet dsnet closed this Dec 21, 2017
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

Successfully merging this pull request may close these issues.

None yet

2 participants