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

reflect.DeepEqual on two empty slices returns false #42265

Closed
3noch opened this issue Oct 29, 2020 · 22 comments
Closed

reflect.DeepEqual on two empty slices returns false #42265

3noch opened this issue Oct 29, 2020 · 22 comments

Comments

@3noch
Copy link

@3noch 3noch commented Oct 29, 2020

What did you expect to see?

reflect.DeepEqual on two slices with the same type and same length of 0 should be true.

What did you see instead?

See repro here: https://play.golang.org/p/YjCo7OcS7hv

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 29, 2020

reflect.DeepEqual implements and extends the equality operation of the language. In the language, a nil slice value is not equal to an empty slice value. So that is what reflect.DeepEqual implements.

@dsnet
Copy link
Member

@dsnet dsnet commented Oct 29, 2020

DeepEqual distinguishes between a slice that is empty and non-nil, versus one that is nil. You are probably seeing that effect.

@dsnet dsnet reopened this Oct 29, 2020
@dsnet
Copy link
Member

@dsnet dsnet commented Oct 29, 2020

Oops, Ian posted at the same time.

@dsnet dsnet closed this Oct 29, 2020
@dsnet
Copy link
Member

@dsnet dsnet commented Oct 29, 2020

I should further note that this is documented on DeepEqual as:

Slice values are deeply equal when all of the following are true: they are both nil or both non-nil

If you need a fuzzier comparison for slices, consider using the cmp.Equal with the cmpopts.EquateEmpty option:

import "github.com/google/go-cmp/cmp"
import "github.com/google/go-cmp/cmp/cmpopts"

cmp.Equal(x, y, cmpopts.EquateEmpty())
@3noch
Copy link
Author

@3noch 3noch commented Oct 29, 2020

How is that possible when both slices have length of 0?

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 29, 2020

A nil slice has a length of zero.

See the discussion of nil at https://blog.golang.org/slices.

@3noch
Copy link
Author

@3noch 3noch commented Oct 29, 2020

Wow. So printing it gives "[]". Getting length gives 0. But comparing them yields false. That is quite a surprising edge case. I'm new to Go obviously but that's really surprising behavior. Far too late now to do anything about it I suppose. Thanks.

@3noch
Copy link
Author

@3noch 3noch commented Oct 29, 2020

Actually, I guess DeepEqual isn't quite what I want here but it was suggested by go itself when I tried to use == on this type. Perhaps that hint can be updated to suggest what you recommended instead?

@3noch
Copy link
Author

@3noch 3noch commented Oct 29, 2020

Another idea for making Go more friendly is to improve DeepEqual's docs to mention this edge case and also mention alternative methods.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 29, 2020

Actually, I guess DeepEqual isn't quite what I want here but it was suggested by go itself when I tried to use == on this type. Perhaps that hint can be updated to suggest what you recommended instead?

Where did that suggestion come from?

Another idea for making Go more friendly is to improve DeepEqual's docs to mention this edge case and also mention alternative methods.

The DeepEqual docs do in fact mention this edge case: "Slice values are deeply equal when all of the following are true: they are both nil or both non-nil...."

@3noch
Copy link
Author

@3noch 3noch commented Oct 29, 2020

The edge case I had in mind was actually that len(nil) == 0 for nil slices. That is not mentioned. That seems to be an inconsistency: we're willing to pretend nil is the same length but not the same equality.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 29, 2020

I suppose I don't think it's necessary for DeepEqual to report that. It's part of the language. It's documented in the slice blog post I mentioned before, and in the language spec at https://golang.org/ref/spec#Length_and_capacity.

@3noch
Copy link
Author

@3noch 3noch commented Oct 30, 2020

Sure. I get that. I'm only suggesting ideas for helping newcomers to Go (like me). Surely it's documented somewhere. But in many languages you can use == instead of reflect.DeepEqual but there are surprises in that translation.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 30, 2020

I just don't think it is the place of the reflect.DeepEqual docs to explain that the length of a nil slice value is zero.

The implementation of reflect.DeepEqual matches the implementation of == with regard to nil slices. And the existing documentation is clear about how DeepEqual handles nil slices.

@3noch
Copy link
Author

@3noch 3noch commented Oct 30, 2020

I opened #42272 to make it clearer on Read()'s end since you could also claim that the documentation burden is on that side. In fact, the reviewer of that says that it's the standard library's custom to treat nil the same as empty slices. But since DeepEqual doesn't do that, there is an incongruity that causes confusion.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 30, 2020

Yes, which is why it is explicitly documented in the reflect.DeepEqual documentation.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 30, 2020

I understand that this area is a source of confusion for new Go programmers. And I understand that it was confusing for you in particular. And if there is good way for us to avoid that confusion, we should definitely do it.

But I am not at all convinced that what we need to do is explain in the reflect.DeepEqual documentation that len of a nil slice value is zero. That is the wrong place for that kind of documentation.

@3noch
Copy link
Author

@3noch 3noch commented Oct 30, 2020

Agreed. I was more thinking of a "gotchas" section or something like that. Something that communicates the incongruity like: "NOTE: Unlike len, Printf, etc. DeepEqual does not treat nil slices as moral equivalents to empty slices"

@3noch
Copy link
Author

@3noch 3noch commented Oct 30, 2020

I can't even promise that such a wording would have helped me. But what I didn't understand is that everything treats nil and empty slices as if they're the same and that DeepEqual stood out in this regard.

@3noch
Copy link
Author

@3noch 3noch commented Oct 30, 2020

Notably, writing your own version of DeepEqual specialized to slices has a natural implementation (check the length, and then check all the corresponding elements). But that implementation is actually not what DeepEqual is doing for slices even though that's what you would have probably written if you were going to write it on slices only.

I'm saying all of this only because I suspect I'm not the only person who has or will run into this.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 30, 2020

The reflect.DeepEqual documentation already says "Note that a non-nil empty slice and a nil slice (for example, []byte{} and []byte(nil)) are not deeply equal." See https://golang.org/pkg/reflect/#DeepEqual.

@3noch
Copy link
Author

@3noch 3noch commented Oct 31, 2020

Fair enough. I guess I was just too new to Go to make sense of that correctly.

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

Successfully merging a pull request may close this issue.

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