-
Notifications
You must be signed in to change notification settings - Fork 18k
reflect: DeepEqual fails for nested NaN values #12025
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
Comments
This is because of a shortcut in reflect.DeepEqual: when comparing a map with itself, DeepEqual returns true without bothering to compare the keys and values. I guess we need to take that shortcut out. |
Same is true when comparing slices. Being able to avoid element-wise comparison of possibly large maps and slices that are identical seems more valuable than searching for that rare NaN. DeepEqual is often used to compare test results, and if the results contain NaNs, one would have to special-case that situation - it might actually be more useful for NaNs to follow the "usual" bitwise equality of floats. Or in other words, DeepEqual perhaps should be defined to return true when comparing two NaNs that are bitwise identical. |
I think it's pretty important that DeepEqual follow the same rules as the language. |
@iant It does where the language defines what equality means. For floats it's indirectly defined by IEEE. NaN != NaN makes sense for some floating-point applications; but even there it's very rare. |
Seems like the documentation should mention this, at least.
|
Well, one issue is that the current behavior is not exactly consistent:
I wouldn't call this expected behavior. As mentioned by @griesemer this is a very special case and I am not sure that efficient map identity comparison should be sacrificed for it. However, I think the documentation should in fact mention it, as "but it will scan elements of arrays, slices, maps ..." is not correct as is, as can be seen in the example above. |
@griesemer Good point that map equality is not defined anyhow. But we need to document this, so reopening. |
@ianlancetaylor Yes, should be addressed one way or the other (I didn't meant to close this, I accidentally pressed the wrong button). |
Relatedly, the documentation says "[DeepEqual] uses normal == equality where possible", but that's not true for pointers. The == operator is defined for pointers to compare whether they point to the same variable, but DeepEqual returns whether the the pointed-to variables are themselves deeply equal. For example: http://play.golang.org/p/IxTbaiNXYO. |
I do not think we should trigger recursion when comparing x with itself. Where does it stop? I think this should be a doc-only bug fix. Failing that, I think we could consider making NaN == NaN for DeepEqual. |
Having thought more about this, and about the places DeepEqual gets used (testing of structural identity) I think it makes the most sense for DeepEqual to treat NaN == NaN always. DeepEqual is already its own definition of equality. We can tweak that definition, especially given the current inconsistencies. |
@rsc +1 |
@RSC1+1 |
Result:false Why is that true? |
@shanhuhai5739 That doesn't have any NaNs in it, so it's off-topic here. But they're unequal because one content is [111 222] and the other content is [222 111]. Slices have order even though maps do not. |
More awfulness:
And the rules about func types introduce similar problems. I think we can't change much here and can just document the current behaviors. |
Actually, there is a simple fix, and it's much simpler than trying to update the docs. Will send CL. |
CL https://golang.org/cl/17450 mentions this issue. |
My simple fix made all these examples return false, but that broke real programs due to reflect.Types no longer being deep equal, because they eventually link to uncomparable values like function pointers. So I backed off somewhat. The semantics were never documented before, so I documented them as a relaxation of Go's == operator. That is, DeepEqual(x, y) is true when the Go expression x==y is both valid (compiles, does not panic) and true, and it is also true in some enumerated cases that implement recursive equality checking. I believe this is fairly defensible, and it is only a minor adjustment from what DeepEqual used to do. It does mean that the original program still prints somewhat inconsistent results (false, true, false; was false, true, true), but now I can explain them at least.
False. NaN != NaN.
True. In Go, a == a, so there is no need to look into the map.
False. In Go, slices are not comparable, so elements are compared, and b[0] != b[0]. An argument might be made for special-casing NaN-equality, but we'd still have the problem that non-nil funcs always compare unequal: main != main. And it's basically impossible to get that one right in programs that aren't statically linked, which is why Go disallows func value equality checks. Given that the example above could be written with s/float64/func()/ and s/math.NaN()/main/, the argument for special-casing NaN seems weakened to the point of not being worth doing. |
go version go1.4 linux/amd64
Fedora Linux 20, 3.19.8-100.fc20.x86_64 #1 SMP kernel
I tried to compare a map that contains NaN values for a fixed key using
reflect.DeepEqual
and the result wastrue
. I expectedfalse
because two NaN values are never equal.reflect.DeepEqual(math.NaN(), math.NaN())
is also false.outputs:
The text was updated successfully, but these errors were encountered: