NaN shouldn't compare EQ to itself

[berdario]

I'm not sure if this makes sense to be merged, but since the fix was trivial, here it is

[garyb]

I saw a bit of the conversation about this on IRC yesterday. Making this change makes Number even less law abiding than it already is, so I'm torn as to whether we should make this change, or whether we should have NaN == NaN = true instead.

Either fix is wrong for different reasons, but I'm not sure which is worse.

[berdario]

Making this change makes Number even less law abiding than it already is

Why is this the case? Is it something about relying on these laws to ensure that sorting is stable?

(Anyhow, another thing that I haven't mentioned: due to the current implementation, NaN is EQ not only to itself, but to every other Number)

[garyb]

(Anyhow, another thing that I haven't mentioned: due to the current implementation, NaN is EQ not only to itself, but to every other Number)

Haha, wow, it's really broken now then.

I don't know how best to proceed here. Maybe we should just say the behaviour of Number when NaN is involved is unspecified (in terms of not being law abiding, at least) and just go ahead and fix Ord this way and leave it at that.

[hdgarrood]

In Haskell, compare x y where either of x or y is NaN will give you GT. Additionally, any comparison involving NaN using the operators >, < etc will give you False. I think we should copy that unless there's a specific reason not to.

I think merging this PR will give us that same behaviour in PureScript (I haven't tested, but I think we do have the second part currently because of inlining).

Can we add tests here for this?

[berdario]

Oh, I missed the github notification of the last message

Can we add tests here for this?

@hdgarrood Sure, but there are no Purescript tests in this repository

I presume that, since this is a base library, we wouldn't use purescript-test-unit and/or purescript-quickcheck?

I guess I could change main Test/Main.purs to something like

main :: AlmostEff
main = do
  mainImpl show
  otherTests

otherTests unit = if (assertSomething) then unit else (throw "errormsg")

assertSomething = compare (0.0/0.0) (0.0/0.0) /= EQ

Or maybe could we add purescript-test-unit, purescript-quickcheck in the devDependencies?

[hdgarrood]

I don't think we should depend on any other libraries in our tests, even if we do find some way to avoid the circular dependency issue, as any other libraries will (rightly) assume that prelude is working correctly.

I think what you proposed with adding tests to test/Main.purs would work, or alternatively do the same as what the existing tests here do, namely, write the tests in JavaScript.

[berdario]

Added the tests

[hdgarrood]

Sorry to be a pain, but can we keep the style consistent with the rest of the file here please? I'm referring to having a space between function(arg) and the opening curly brace, and using semicolons at the end of lines (even if they're not strictly necessary).

[berdario]

done

[hdgarrood]

Aside from my (very minor) comment, this all looks great to me, thank you!

[garyb]

@paf31 any comments on this one before merging?

[garyb]

@paf31 bump! I'll make the 2.0 release once this is merged.

[paf31]

I'm fine with this change, although I'd rather not admit nan :: Number at all, although I admit that's problematic for other reasons.

[garyb]

In a perfect world, I agree, but it's impractical to do so if we use JS's Number type. Infinity is similarly annoying...