Clojure

<= is incorrect when args include Double/NaN

Details

  • Type: Defect Defect
  • Status: Reopened Reopened
  • Priority: Trivial Trivial
  • Resolution: Unresolved
  • Affects Version/s: Release 1.6
  • Fix Version/s: Release 1.7
  • Component/s: None
  • Labels:
  • Environment:
    Mac OS X, Java 6
  • Patch:
    Code and Test
  • Approval:
    Screened

Description

user=> (<= Double/NaN 1)
false  
user=> (<= (double Double/NaN) 1)
true    ;; should match Double object result

Cause: The problem was that the logic for lte/gte depended on the fact that lte is equivalent to !gt.
However, in Java, this assumption is invalid - any comparison involving NaN always yields false.

Solution: The fix was to adding lte and gte methods to Numbers.Ops directly, rather than implementing everything in terms of lt. This was the only fix I could see that didn't incur the cost of runtime checks for NaN.

Patch: clj-738-v2.diff

Screened by: Alex Miller

  1. 738.diff
    26/Aug/11 11:33 AM
    4 kB
    Luke VanderHart
  2. 738-tests.diff
    26/Aug/11 11:33 AM
    3 kB
    Luke VanderHart
  3. clj-738-v2.diff
    18/Apr/14 11:41 AM
    6 kB
    Andy Fingerhut

Activity

Hide
Jason Wolfe added a comment -

The source of the issue seems to be incorrect treatment of boxed NaN:

user> (<= 1000 (Double. Double/NaN))
true
user> (<= 1000 (double Double/NaN))
false

Show
Jason Wolfe added a comment - The source of the issue seems to be incorrect treatment of boxed NaN: user> (<= 1000 (Double. Double/NaN)) true user> (<= 1000 (double Double/NaN)) false
Hide
Alexander Taggart added a comment - - edited

Primitive comparisons use java's primitive operators directly, which always return false for NaN, even when testing equality between two NaNs.

In clojure, Number comparisons are all logical variations around calls to Numbers.Ops.lt(Number, Number). So a call to (<= x y) is actually a call to (not (< y x)), which eventually uses the primitive < operator. Alas that logical premise doesn't hold when dealing with NaN:

user=> (<= 1 Double/NaN)
false
user=> (not (< Double/NaN 1))
true

So the bug is not that boxed NaN is treated incorrectly, but rather:

user> (<= 1000 (Double. Double/NaN)) ; becomes !(NaN < 1000) 
true
user> (<= 1000 (double Double/NaN))  ; becomes (1000 <= NaN)
false

In the original example, since there are more than two args, the primitive looking args were boxed:

user=> (<= 10 Double/NaN 1) ; equivalent to logical-and of the following
true
user=> (<= 10 (Double. Double/NaN))  ; becomes !(NaN < 10)
true
user=> (<= (Double. Double/NaN) 1)   ; becomes !(1 < NaN)
true

Note however that java object comparisons for NaNs behave differently: NaN is the largest Double, and NaNs equal each other (see the javadoc).

If we make object NaN comparisons always return false, we would need to add the rest of the comparison methods to Numbers.Ops. Yet doing so could also make collection sorting algorithms behave oddly, deviating from sorting written in java. Besides, (= NaN NaN) => false is annoying.

Clojure already throws out the notion of error-free dividing by zero (which for doubles would otherwise result in NaN or Infinity, depending on the dividend). Perhaps we could similarly error on NaNs passed to clojure numeric ops. They seem to be more trouble than they're worth. That said, people smarter than me thought they were useful.

Then there's that -0.0 nonsense...

Show
Alexander Taggart added a comment - - edited Primitive comparisons use java's primitive operators directly, which always return false for NaN, even when testing equality between two NaNs. In clojure, Number comparisons are all logical variations around calls to Numbers.Ops.lt(Number, Number). So a call to (<= x y) is actually a call to (not (< y x)), which eventually uses the primitive < operator. Alas that logical premise doesn't hold when dealing with NaN:
user=> (<= 1 Double/NaN)
false
user=> (not (< Double/NaN 1))
true
So the bug is not that boxed NaN is treated incorrectly, but rather:
user> (<= 1000 (Double. Double/NaN)) ; becomes !(NaN < 1000) 
true
user> (<= 1000 (double Double/NaN))  ; becomes (1000 <= NaN)
false
In the original example, since there are more than two args, the primitive looking args were boxed:
user=> (<= 10 Double/NaN 1) ; equivalent to logical-and of the following
true
user=> (<= 10 (Double. Double/NaN))  ; becomes !(NaN < 10)
true
user=> (<= (Double. Double/NaN) 1)   ; becomes !(1 < NaN)
true
Note however that java object comparisons for NaNs behave differently: NaN is the largest Double, and NaNs equal each other (see the javadoc). If we make object NaN comparisons always return false, we would need to add the rest of the comparison methods to Numbers.Ops. Yet doing so could also make collection sorting algorithms behave oddly, deviating from sorting written in java. Besides, (= NaN NaN) => false is annoying. Clojure already throws out the notion of error-free dividing by zero (which for doubles would otherwise result in NaN or Infinity, depending on the dividend). Perhaps we could similarly error on NaNs passed to clojure numeric ops. They seem to be more trouble than they're worth. That said, people smarter than me thought they were useful. Then there's that -0.0 nonsense...
Hide
Jouni K. Seppänen added a comment -

On current master, (<= x y) seems to be special-cased by the compiler, but when <= is called dynamically, the bug is still there:

user=> (<= 1 Float/NaN)
false
user=> (let [op <=] (op 1 Float/NaN))
true

Since CLJ-354 got marked "Completed", perhaps there was an attempt to fix this.

Show
Jouni K. Seppänen added a comment - On current master, (<= x y) seems to be special-cased by the compiler, but when <= is called dynamically, the bug is still there:
user=> (<= 1 Float/NaN)
false
user=> (let [op <=] (op 1 Float/NaN))
true
Since CLJ-354 got marked "Completed", perhaps there was an attempt to fix this.
Hide
Alexander Taggart added a comment -

Using let forces calling <= as a function rather than inlining Numbers/lte, which means the args are treated as objects not primitives, thus the different behaviour as I discussed earlier.

Show
Alexander Taggart added a comment - Using let forces calling <= as a function rather than inlining Numbers/lte, which means the args are treated as objects not primitives, thus the different behaviour as I discussed earlier.
Alexander Taggart made changes -
Field Original Value New Value
Waiting On richhickey
Hide
Aaron Bedra added a comment -

Rich, what should the behavior be?

Show
Aaron Bedra added a comment - Rich, what should the behavior be?
Aaron Bedra made changes -
Fix Version/s Release.Next [ 10038 ]
Affects Version/s Release 1.2 [ 10037 ]
Priority Critical [ 2 ] Minor [ 4 ]
Hide
Jouni K. Seppänen added a comment -

My suggestion for the behavior is to follow Java (Java Language Specification §15.20.1) and IEEE 754. The java.sun.com site seems to be down right now, but here's a Google cache link:

http://webcache.googleusercontent.com/search?q=cache:http://java.sun.com/docs/books/jls/third_edition/html/expressions.html#15.20.1

Show
Jouni K. Seppänen added a comment - My suggestion for the behavior is to follow Java (Java Language Specification §15.20.1) and IEEE 754. The java.sun.com site seems to be down right now, but here's a Google cache link: http://webcache.googleusercontent.com/search?q=cache:http://java.sun.com/docs/books/jls/third_edition/html/expressions.html#15.20.1
Hide
Rich Hickey added a comment -

It should work the same as primitive double in all cases

Show
Rich Hickey added a comment - It should work the same as primitive double in all cases
Rich Hickey made changes -
Waiting On richhickey
Luke VanderHart made changes -
Assignee Luke VanderHart [ lvanderhart ]
Luke VanderHart made changes -
Status Open [ 1 ] In Progress [ 3 ]
Hide
Luke VanderHart added a comment - - edited

Added patches. The problem was that our logic for lte/gte depended on the fact that lte is equivalent to !gt.

However, in Java, this assumption is invalid - any comparison involving NaN always yields false.

The fix was to adding lte and gte methods to Numbers.Ops directly, rather than implementing everything in terms of lt. This was the only fix I could see that didn't incur the cost of runtime checks for NaN.

Show
Luke VanderHart added a comment - - edited Added patches. The problem was that our logic for lte/gte depended on the fact that lte is equivalent to !gt. However, in Java, this assumption is invalid - any comparison involving NaN always yields false. The fix was to adding lte and gte methods to Numbers.Ops directly, rather than implementing everything in terms of lt. This was the only fix I could see that didn't incur the cost of runtime checks for NaN.
Luke VanderHart made changes -
Attachment 738.diff [ 10321 ]
Attachment 738-tests.diff [ 10322 ]
Luke VanderHart made changes -
Status In Progress [ 3 ] Resolved [ 5 ]
Resolution Completed [ 1 ]
Patch Code and Test
Stuart Halloway made changes -
Status Resolved [ 5 ] Closed [ 6 ]
Hide
Alex Miller added a comment -

David Welte noted: "CLJ-738 is marked Closed but the attached patch is has not been applied and both Clojure 1.5.1 and 1.6.0-beta2 exhibit the bad behavior listed in CLJ-738. The issue that CLJ-738 is that (<= (Double. Double/NaN) 1) evaluates to true while (<= Double/NaN 1) evaluates to false."

I concur that this patch was not applied. It looks likely that Luke marked this as Resolved when the patch was ready instead of whatever state change would have been appropriate at the time of the ticket (the process has varied over the years). AFAICT, this ticket should be open and Vetted (accepted as a problem) but probably needs release targeting and an updated patch based on current code.

Show
Alex Miller added a comment - David Welte noted: "CLJ-738 is marked Closed but the attached patch is has not been applied and both Clojure 1.5.1 and 1.6.0-beta2 exhibit the bad behavior listed in CLJ-738. The issue that CLJ-738 is that (<= (Double. Double/NaN) 1) evaluates to true while (<= Double/NaN 1) evaluates to false." I concur that this patch was not applied. It looks likely that Luke marked this as Resolved when the patch was ready instead of whatever state change would have been appropriate at the time of the ticket (the process has varied over the years). AFAICT, this ticket should be open and Vetted (accepted as a problem) but probably needs release targeting and an updated patch based on current code.
Alex Miller made changes -
Approval Vetted [ 10003 ]
Assignee Luke VanderHart [ lvanderhart ]
Status Closed [ 6 ] Reopened [ 4 ]
Resolution Completed [ 1 ]
Hide
Andy Fingerhut added a comment -

Might want to make the "Fix Version" on this ticket empty so it is back on the JIRA state chart as Vetted.

Show
Andy Fingerhut added a comment - Might want to make the "Fix Version" on this ticket empty so it is back on the JIRA state chart as Vetted.
Alex Miller made changes -
Fix Version/s Release 1.3 [ 10038 ]
Alex Miller made changes -
Description user> (<= 10 Double/NaN 1)
true

(on both 1.2 and 1.3 alpha 5).
{code}
user=> (<= 10 (long Double/NaN) 1)
false
user=> (<= 10 Double/NaN 1)
true ;; should match primitive version
{code}

*Cause:* The problem was that our logic for lte/gte depended on the fact that lte is equivalent to !gt.
However, in Java, this assumption is invalid - any comparison involving NaN always yields false.

*Solution:* The fix was to adding lte and gte methods to Numbers.Ops directly, rather than implementing everything in terms of lt. This was the only fix I could see that didn't incur the cost of runtime checks for NaN.

*Patch:* 738.diff, 738-tests.diff

*Screened by:*
Affects Version/s Release 1.3 [ 10038 ]
Affects Version/s Release 1.6 [ 10157 ]
Alex Miller made changes -
Labels math
Alex Miller made changes -
Description {code}
user=> (<= 10 (long Double/NaN) 1)
false
user=> (<= 10 Double/NaN 1)
true ;; should match primitive version
{code}

*Cause:* The problem was that our logic for lte/gte depended on the fact that lte is equivalent to !gt.
However, in Java, this assumption is invalid - any comparison involving NaN always yields false.

*Solution:* The fix was to adding lte and gte methods to Numbers.Ops directly, rather than implementing everything in terms of lt. This was the only fix I could see that didn't incur the cost of runtime checks for NaN.

*Patch:* 738.diff, 738-tests.diff

*Screened by:*
{code}
user=> (<= (long Double/NaN) 1)
true
user=> (<= Double/NaN 1)
false ;; should match primitive version
{code}

*Cause:* The problem was that the logic for lte/gte depended on the fact that lte is equivalent to !gt.
However, in Java, this assumption is invalid - any comparison involving NaN always yields false.

*Solution:* The fix was to adding lte and gte methods to Numbers.Ops directly, rather than implementing everything in terms of lt. This was the only fix I could see that didn't incur the cost of runtime checks for NaN.

*Patch:* 738.diff, 738-tests.diff

*Screened by:*
Alex Miller made changes -
Priority Minor [ 4 ] Trivial [ 5 ]
Rich Hickey made changes -
Fix Version/s Release 1.7 [ 10250 ]
Hide
Andy Fingerhut added a comment -

Patch clj-738-v2.diff is identical in content to Luke's 2 patches 738.diff and 738-tests.diff, and includes them both, retaining his authorship. The only change is to a few context lines so that the new patch applies cleanly to latest master, whereas the older patches did not.

Show
Andy Fingerhut added a comment - Patch clj-738-v2.diff is identical in content to Luke's 2 patches 738.diff and 738-tests.diff, and includes them both, retaining his authorship. The only change is to a few context lines so that the new patch applies cleanly to latest master, whereas the older patches did not.
Andy Fingerhut made changes -
Attachment clj-738-v2.diff [ 12952 ]
Hide
Alex Miller added a comment -

While the patch looks good and tests all pass, the example listed in the ticket description did not actually change behavior with the patch?

Show
Alex Miller added a comment - While the patch looks good and tests all pass, the example listed in the ticket description did not actually change behavior with the patch?
Alex Miller made changes -
Approval Vetted [ 10003 ] Incomplete [ 10006 ]
Description {code}
user=> (<= (long Double/NaN) 1)
true
user=> (<= Double/NaN 1)
false ;; should match primitive version
{code}

*Cause:* The problem was that the logic for lte/gte depended on the fact that lte is equivalent to !gt.
However, in Java, this assumption is invalid - any comparison involving NaN always yields false.

*Solution:* The fix was to adding lte and gte methods to Numbers.Ops directly, rather than implementing everything in terms of lt. This was the only fix I could see that didn't incur the cost of runtime checks for NaN.

*Patch:* 738.diff, 738-tests.diff

*Screened by:*
{code}
user=> (<= (long Double/NaN) 1)
true
user=> (<= Double/NaN 1)
false ;; should match primitive version
{code}

*Cause:* The problem was that the logic for lte/gte depended on the fact that lte is equivalent to !gt.
However, in Java, this assumption is invalid - any comparison involving NaN always yields false.

*Solution:* The fix was to adding lte and gte methods to Numbers.Ops directly, rather than implementing everything in terms of lt. This was the only fix I could see that didn't incur the cost of runtime checks for NaN.

*Patch:* clj-738-v2.diff

*Screened by:*
Hide
Jason Wolfe added a comment -

The ticket description has a typo (long, not double) – sorry. The first comment has a real test case.

user> (<= 1000 (Double. Double/NaN))
true
user> (<= 1000 (double Double/NaN))
false

Show
Jason Wolfe added a comment - The ticket description has a typo (long, not double) – sorry. The first comment has a real test case. user> (<= 1000 (Double. Double/NaN)) true user> (<= 1000 (double Double/NaN)) false
Hide
Alex Miller added a comment -

Doh! Thank you. I'm the one that introduced it too.

Show
Alex Miller added a comment - Doh! Thank you. I'm the one that introduced it too.
Alex Miller made changes -
Description {code}
user=> (<= (long Double/NaN) 1)
true
user=> (<= Double/NaN 1)
false ;; should match primitive version
{code}

*Cause:* The problem was that the logic for lte/gte depended on the fact that lte is equivalent to !gt.
However, in Java, this assumption is invalid - any comparison involving NaN always yields false.

*Solution:* The fix was to adding lte and gte methods to Numbers.Ops directly, rather than implementing everything in terms of lt. This was the only fix I could see that didn't incur the cost of runtime checks for NaN.

*Patch:* clj-738-v2.diff

*Screened by:*
{code}
user=> (<= (double Double/NaN) 1)
true
user=> (<= Double/NaN 1)
false ;; should match primitive version
{code}

*Cause:* The problem was that the logic for lte/gte depended on the fact that lte is equivalent to !gt.
However, in Java, this assumption is invalid - any comparison involving NaN always yields false.

*Solution:* The fix was to adding lte and gte methods to Numbers.Ops directly, rather than implementing everything in terms of lt. This was the only fix I could see that didn't incur the cost of runtime checks for NaN.

*Patch:* clj-738-v2.diff

*Screened by:*
Alex Miller made changes -
Approval Incomplete [ 10006 ] Screened [ 10004 ]
Description {code}
user=> (<= (double Double/NaN) 1)
true
user=> (<= Double/NaN 1)
false ;; should match primitive version
{code}

*Cause:* The problem was that the logic for lte/gte depended on the fact that lte is equivalent to !gt.
However, in Java, this assumption is invalid - any comparison involving NaN always yields false.

*Solution:* The fix was to adding lte and gte methods to Numbers.Ops directly, rather than implementing everything in terms of lt. This was the only fix I could see that didn't incur the cost of runtime checks for NaN.

*Patch:* clj-738-v2.diff

*Screened by:*
{code}
user=> (<= Double/NaN 1)
false
user=> (<= (double Double/NaN) 1)
true ;; should match Double object result
{code}

*Cause:* The problem was that the logic for lte/gte depended on the fact that lte is equivalent to !gt.
However, in Java, this assumption is invalid - any comparison involving NaN always yields false.

*Solution:* The fix was to adding lte and gte methods to Numbers.Ops directly, rather than implementing everything in terms of lt. This was the only fix I could see that didn't incur the cost of runtime checks for NaN.

*Patch:* clj-738-v2.diff

*Screened by:* Alex Miller

People

Vote (1)
Watch (6)

Dates

  • Created:
    Updated: