Skip to content

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

Closed
tgpfeiffer opened this issue Aug 5, 2015 · 19 comments
Closed

reflect: DeepEqual fails for nested NaN values #12025

tgpfeiffer opened this issue Aug 5, 2015 · 19 comments
Milestone

Comments

@tgpfeiffer
Copy link

What version of Go are you using (go version)?

go version go1.4 linux/amd64

What operating system and processor architecture are you using?

Fedora Linux 20, 3.19.8-100.fc20.x86_64 #1 SMP kernel

What did you do?
What did you expect to see?
What did you see instead?

I tried to compare a map that contains NaN values for a fixed key using reflect.DeepEqual and the result was true. I expected false because two NaN values are never equal. reflect.DeepEqual(math.NaN(), math.NaN()) is also false.

package main

import "fmt"
import "math"
import "reflect"

func main() {
    fmt.Println(reflect.DeepEqual(math.NaN(), math.NaN()))

    a := map[string]float64{"a": math.NaN()}
    fmt.Println(reflect.DeepEqual(a, a))

    b := []float64{math.NaN()}
    fmt.Println(reflect.DeepEqual(b, b))
}

outputs:

false
true
true
@ianlancetaylor
Copy link
Member

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.

@griesemer
Copy link
Contributor

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.

@ianlancetaylor ianlancetaylor added this to the Go1.6 milestone Aug 5, 2015
@ianlancetaylor
Copy link
Member

I think it's pretty important that DeepEqual follow the same rules as the language.

@mikioh mikioh changed the title reflect.DeepEqual fails for nested NaN values reflect: DeepEqual fails for nested NaN values Aug 5, 2015
@griesemer
Copy link
Contributor

@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.

@cespare
Copy link
Contributor

cespare commented Aug 5, 2015

Seems like the documentation should mention this, at least.

It uses normal == equality where possible but will scan elements of arrays, slices, maps, and fields of structs. In maps, keys are compared with == but elements use deep equality.

@mikioh mikioh modified the milestones: Go1.5, Go1.6 Aug 5, 2015
@tgpfeiffer
Copy link
Author

It does where the language defines what equality means.

Well, one issue is that the current behavior is not exactly consistent:

a := map[string]float64{"a": math.NaN()}
reflect.DeepEqual(a, map[string]float64{"a": math.NaN()}) // false
reflect.DeepEqual(a, a) // true

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.

@ianlancetaylor
Copy link
Member

@griesemer Good point that map equality is not defined anyhow.

But we need to document this, so reopening.

@ianlancetaylor ianlancetaylor reopened this Aug 6, 2015
@ianlancetaylor ianlancetaylor modified the milestones: Go1.6, Go1.5 Aug 6, 2015
@ianlancetaylor ianlancetaylor added the Documentation Issues describing a change to documentation. label Aug 6, 2015
@griesemer
Copy link
Contributor

@ianlancetaylor Yes, should be addressed one way or the other (I didn't meant to close this, I accidentally pressed the wrong button).

@mdempsky
Copy link
Contributor

mdempsky commented Aug 6, 2015

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.

@rsc
Copy link
Contributor

rsc commented Oct 16, 2015

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.

@rsc
Copy link
Contributor

rsc commented Nov 5, 2015

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 rsc removed the Documentation Issues describing a change to documentation. label Nov 5, 2015
@griesemer
Copy link
Contributor

@rsc +1

@benbjohnson
Copy link

@RSC1+1

@shanhuhai5739
Copy link

package main

import (
    "fmt"
    "reflect"
)

type format map[string]interface{}

func main() {
    aaa := map[string]interface {}{"meta":map[string]interface {}{"category":"paragraph"}, "content":[]string{"111","222"}}
    bbb := map[string]interface {}{"content":[]string{"222","111"}, "meta":map[string]interface {}{"category":"paragraph"}}

    if reflect.DeepEqual(aaa, bbb) {
        fmt.Println("true")
    } else {
        fmt.Println("false")
    }
}

Result:false

Why is that true?

@rsc
Copy link
Contributor

rsc commented Nov 12, 2015

@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.

@rsc
Copy link
Contributor

rsc commented Nov 25, 2015

More awfulness:

package main

import (
    "fmt"
    "math"
    "reflect"
)

func main() {
    m1 := map[float64]float64{math.NaN(): 1}
    m2 := map[float64]float64{1: 2}
    fmt.Println(reflect.DeepEqual(m1, m2))
}

And the rules about func types introduce similar problems. I think we can't change much here and can just document the current behaviors.

@rsc
Copy link
Contributor

rsc commented Nov 25, 2015

Actually, there is a simple fix, and it's much simpler than trying to update the docs. Will send CL.

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/17450 mentions this issue.

@rsc
Copy link
Contributor

rsc commented Dec 7, 2015

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.

fmt.Println(reflect.DeepEqual(math.NaN(), math.NaN()))

False. NaN != NaN.

a := map[string]float64{"a": math.NaN()}
fmt.Println(reflect.DeepEqual(a, a))

True. In Go, a == a, so there is no need to look into the map.

b := []float64{math.NaN()}
fmt.Println(reflect.DeepEqual(b, b))

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.

@rsc rsc closed this as completed in a77182f Dec 11, 2015
@golang golang locked and limited conversation to collaborators Dec 14, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

10 participants