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: errors: make different results from New not reflect.DeepEqual #15910

Closed
dsnet opened this issue May 31, 2016 · 2 comments
Closed

proposal: errors: make different results from New not reflect.DeepEqual #15910

dsnet opened this issue May 31, 2016 · 2 comments

Comments

@dsnet
Copy link
Member

@dsnet dsnet commented May 31, 2016

Currently, in go1.6, different error values returned by errors.New(...) are equal (in the sense of the reflect.DeepEqual usage) if they have the same error message. This encourages people to write unit tests like the following:

want := errors.New("archive/tar: header field too long or contains invalid values")
if !reflect.DeepEqual(got, want) {
    ...
}

This is a poor test it assumes an exact error message and type for an unexported error from a given package. Instead, we should cause DeepEqual to always return false somehow by embedding some field in the concrete errorString type that always compares to false for anything but itself.

Thus, I expect the following to happen:

a := errors.New("foo")
b := errors.New("foo")

fmt.Println(a == a) // True
fmt.Println(a == b) // False
fmt.Println(reflect.DeepEqual(a, a)) // True
fmt.Println(reflect.DeepEqual(a, b)) // False (this is currently true on Go1.6)

We probably cannot change this in Go1 because the sheer number of tests this would probably break, but it may be worth considering for Go2.

@dsnet dsnet added the Go2 label May 31, 2016
@dsnet dsnet changed the title errors: difference error returned by New should not be DeepEqual errors: different errors returned by New should not be DeepEqual May 31, 2016
@davecheney
Copy link
Contributor

@davecheney davecheney commented May 31, 2016

What was your test testing? Was it the presence of the error, the presence
of a particular value, or the contents of the Error() string?

On Wed, 1 Jun 2016, 07:15 Joe Tsai notifications@github.com wrote:

Currently, in go1.6, different error values returned by errors.New(...)
are equal (in the sense of the reflect.DeepEqual usage) if they have the
same error message. This encourages people to write unit tests like the
following:

want := errors.New("archive/tar: header field too long or contains invalid values")if !reflect.DeepEqual(got, want) {
...
}

This is a poor test it assumes an exact error message and type for an
unexported error from a given package. Instead, we should cause DeepEqual
to always return false somehow by embedding some field in the concrete
errorString type that always compares to false for anything but itself.

Thus, I expect the following to happen:

a := errors.New("foo")b := errors.New("foo")

reflect.DeepEqual(a, a) // True
reflect.DeepEqual(a, b) // False (this is currently true)

We probably cannot change this in Go1 because the sheer number of tests
this would probably break, but it may be worth considering for Go2.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#15910, or mute the thread
https://github.com/notifications/unsubscribe/AAAcA6-SWX9Hy-I6M7NkX3-dB23j8V0gks5qHKT5gaJpZM4Iq92H
.

@quentinmit quentinmit added this to the Unplanned milestone Jun 17, 2016
@rsc rsc changed the title errors: different errors returned by New should not be DeepEqual errors: make different results from New not reflect.DeepEqual Jun 16, 2017
@rsc rsc changed the title errors: make different results from New not reflect.DeepEqual proposal: errors: make different results from New not reflect.DeepEqual Jun 17, 2017
@gopherbot gopherbot added the Proposal label Jun 17, 2017
@dsnet
Copy link
Member Author

@dsnet dsnet commented Sep 19, 2018

I'm closing this. Inside Google, most tests have been moving towards cmp.Equal, which makes it more obvious when you're doing comparisons in a forward incompatible way.

Although, I still occasionally see direct string comparisons on error strings, I haven't seen a test use reflect.DeepEqual on an error they created themselves in a while.

@dsnet dsnet closed this Sep 19, 2018
@golang golang locked and limited conversation to collaborators Sep 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.