Welcome! Please see the About page for a little more info on how this works.

0 votes
in Clojure by
retagged by

I was looking through the new clojure.core/abs implementation on type Ratio, and it seemed that there is an assumption in the code that uses the internal details of type clojure.lang.Ratio that the denominator is always positive (e.g. see the definitions of isNeg and isPos methods for type Ratio).

I started looking for cases where that assumption might be violated, and found the following, all of which might be distinctive to values of type Ratio produced by an expression with Long/MIN_VALUE as the denominator and an integer with type Long or smaller as the numerator:

$ clj -Sdeps '{:deps {org.clojure/clojure {:mvn/version "1.11.0-alpha4"}}}'
Clojure 1.11.0-alpha4

user=> (/ 1 Long/MIN_VALUE)
-1/-9223372036854775808
user=> (< (/ 1 Long/MIN_VALUE) 0)         ;; This gives correct numerical answer
true
user=> (< (* 1 (/ 1 Long/MIN_VALUE)) 0)        ;; This does not
false
user=> (abs (/ 1 Long/MIN_VALUE))            ;; Gives incorrect numerical answer
1/-9223372036854775808
user=> (< (abs (/ 1 Long/MIN_VALUE)) 0)        ;; correct numerical answer
false
user=> (< (* 1 (abs (/ 1 Long/MIN_VALUE))) 0)       ;; incorrect numerical answer
true

I have carefully checked other occurrences of Ratio in source file Numbers.java in Clojure's implementation, and as far as I can tell the only issue is with the method named divide in the LongOps class. If that method avoided this problem by checking for the special case when the numerator and/or denominator is equal to Long/MIN_VALUE and behaved a little differently in that case, I do not see any other bugs.

Here is another issue if the numerator is equal to Long/MIN_VALUE, where the root cause is in the same LongOps divide method:

user=> (/ Long/MIN_VALUE -3)   ;; should return positive value, but returns negative value
-9223372036854775808/3
user=> (< (/ Long/MIN_VALUE -3) 0)
true

2 Answers

0 votes
by
selected by
 
Best answer

Filed this as https://clojure.atlassian.net/browse/CLJ-2694 and added a patch to detect the use of Long/MIN_VALUE and defer to the bigint impl instead. Added these cases as tests.

0 votes
by
by
I don't know off the top of my head, but I think fixing one will not necessarily fix the other, i.e. the Java methods that underly each issue are disjoint, at least today.
by
Can you clarify?
by
Hmmm, perhaps you are right, and/or some of this falls into this WontFix JIRA: https://clojure.atlassian.net/browse/CLJ-1253

It does seem a little odd that / in almost all cases returns an exact answer via a Ratio, but the test cases above show that in some cases it does not return a correct answer, even when it returns a Ratio.  These are not cases where the return value is stored in a Long/long.
by
The test cases in this ask question give the wrong answer because of how LongOps method divide is implemented.  CLJ-1254's root cause is because of how LongOps method quotient is implemented, I believe, so independent bugs in different parts of the implementation, as far as I can tell.
...