Permalink
Browse files

Quasi-comprehensive BigDecimal soundness/correctness fix.

This fixes issues SI-6153, SI-6173, SI-6456, SI-6699, and SI-8116, along with a number of other similar possible issues.

Relevant changes include
  * Changes to avoid heap explosion when working with BigInt
    - to isWhole
    - to hashCode
    - to equals
    - to BigInt's equals
  * Changes to enable equality matching hashCode
    - Only for sufficiently small BigInt
    - For identical values with different precision
  * Changes to isValidDouble
    - Takes precision into account now
    - New methods added to test whether even if the Double is not represented exactly, it's a representation of a certain type
    - New companion methods added to allow intended expansion of Double (binary/decimal difference)
  * Changes to constructor
    - Null arguments are not allowed (these can throw NPEs later at awkward/unexpected times)
  * New JUnit test to test all these things
  * Fixed existing tests to expect new behavior
  * Modified scaladocs to explain the issues
  * Deprecated problematic methods
  * Made application of MathContext more consistent (it is where you expect it and not where you don't)

These changes are coordinated, for the most part, hence the monolithic
commit.
  • Loading branch information...
1 parent ef4c5d2 commit 29541ce3961a797836b232ccc12a60cc09f5de3e @Ichoran Ichoran committed Dec 29, 2013

Large diffs are not rendered by default.

Oops, something went wrong.
@@ -119,7 +119,7 @@ final class BigInt(val bigInteger: BigInteger) extends ScalaNumber with ScalaNum
*/
override def equals(that: Any): Boolean = that match {
case that: BigInt => this equals that
- case that: BigDecimal => that.toBigIntExact exists (this equals _)
+ case that: BigDecimal => that equals this
case that: Double => isValidDouble && toDouble == that
case that: Float => isValidFloat && toFloat == that
case x => isValidLong && unifiedPrimitiveEquals(x)
@@ -31,7 +31,6 @@ object Test_BigDecimal {
val xi: BigDecimal = 1
val xd: BigDecimal = 1.0
- val xf: BigDecimal = BigDecimal(1.0f)
val xs: BigDecimal = BigDecimal("1.0")
val xbi: BigDecimal = BigDecimal(scala.BigInt(1))
@@ -3,4 +3,4 @@
0
0
0
-14
+15
@@ -19,25 +19,27 @@ object Test {
assert(!x.isValidChar, x)
assert(!x.isValidShort, x)
assert(!x.isValidByte, x)
-// assert(y.isWhole, y)
+ assert(y.isWhole, y)
assert(!y.isValidShort, y)
assert(y.isValidChar, y)
assert(y.isValidInt, y)
- assert(y.isValidFloat, y)
- assert(y.isValidDouble, y)
+ assert(y.isDecimalFloat, y)
+ assert(y.isDecimalDouble, y)
assert(y.isValidLong, y)
assert(!y.isValidByte, y)
-// assert(!y1.isWhole)
+ assert(!y1.isWhole)
assert(!y1.isValidLong, y1)
- assert(!y1.isValidFloat, y1)
- assert(!y1.isValidDouble, y1)
+ assert(y1.isDecimalFloat, y1)
+ assert(y1.isDecimalDouble, y1)
+ assert(!y1.isExactFloat, y1)
+ assert(!y1.isExactDouble, y1)
assert(!y1.isValidInt, y1)
assert(!y1.isValidChar, y1)
assert(!y1.isValidShort, y1)
assert(!y1.isValidByte, y1)
assert(!y2.isValidLong, y2)
- assert(y2.isValidFloat, y2)
- assert(y2.isValidDouble, y2)
+ assert(y2.isExactFloat, y2)
+ assert(y2.isExactDouble, y2)
assert(!l1.isValidInt && (l1 - 1).isValidInt, l1)
assert(!l2.isValidInt && (l2 + 1).isValidInt, l2)
@@ -170,8 +172,8 @@ object Test {
if (!d.isInfinity) {
val bd = BigDecimal(new java.math.BigDecimal(d))
// assert(!bd.isWhole, bd)
- assert(bd.isValidDouble, bd)
- assert(bd.isValidFloat == isFloat, bd)
+ assert(bd.isExactDouble, bd)
+ assert(bd.isExactFloat == isFloat, bd)
assert(!bd.isValidLong, bd)
assert(!bd.isValidInt, bd)
assert(!bd.isValidChar, bd)
@@ -210,9 +212,9 @@ object Test {
val isFloat = !bi.toFloat.isInfinity && bd.compare(BigDecimal(new java.math.BigDecimal(bi.toFloat))) == 0
val isDouble = !bi.toDouble.isInfinity && bd.compare(BigDecimal(new java.math.BigDecimal(bi.toDouble))) == 0
-// assert(bd.isWhole, bd)
- assert(bd.isValidDouble == isDouble, bd)
- assert(bd.isValidFloat == isFloat, bd)
+ assert(bd.isWhole, bd)
+ assert(bd.isBinaryDouble == isDouble, bd)
+ assert(bd.isBinaryFloat == isFloat, bd)
assert(bd.isValidLong == isLong, bd)
assert(bd.isValidInt == isInt, bd)
assert(bd.isValidChar == isChar, bd)
@@ -31,6 +31,24 @@ object Test {
).flatten
}
+ // Don't necessarily expect BigDecimal created from BigInt to agree with Double here.
+ def isIffy(x: Any, y: Any, canSwap: Boolean = true): Boolean = x match {
+ case bd: BigDecimal => y match {
+ case _: Float | _: Double => bd.toString.length > 15
+ case _ => false
+ }
+ case _ => canSwap && isIffy(y, x, false)
+ }
+
+ // Don't necessarily expect BigInt to agree with Float/Double beyond a Long
+ def isIffyB(x: Any, y: Any, canSwap: Boolean = true): Boolean = x match {
+ case bi: BigInt => y match {
+ case _: Float | _: Double => bi < Long.MinValue || bi > Long.MaxValue
+ case _ => false
+ }
+ case _ => canSwap && isIffyB(y, x, false)
+ }
+
def main(args: Array[String]): Unit = {
val ints = (0 to 15).toList map (Short.MinValue >> _)
val ints2 = ints map (x => -x)
@@ -46,7 +64,6 @@ object Test {
val sets = setneg1 ++ setneg2 ++ List(zero) ++ setpos1 ++ setpos2
for (set <- sets ; x <- set ; y <- set) {
- // println("'%s' == '%s' (%s == %s) (%s == %s)".format(x, y, x.hashCode, y.hashCode, x.##, y.##))
assert(x == y, "%s/%s != %s/%s".format(x, x.getClass, y, y.getClass))
assert(x.## == y.##, "%s != %s".format(x.getClass, y.getClass))
}
@@ -64,9 +81,11 @@ object Test {
val sets2 = setneg1 ++ setneg1b ++ setneg2 ++ setneg2b ++ List(zero) ++ setpos1 ++ setpos1b ++ setpos2 ++ setpos2b
for (set <- sets2 ; x <- set ; y <- set) {
-// println("'%s' == '%s' (%s == %s) (%s == %s)".format(x, y, x.hashCode, y.hashCode, x.##, y.##))
- assert(x == y, "%s/%s != %s/%s".format(x, x.getClass, y, y.getClass))
-// assert(x.## == y.##, "%s != %s".format(x.getClass, y.getClass)) Disable until Double.## is fixed (SI-5640)
+ if (!isIffy(x,y)) {
+ assert(x == y, "%s/%s != %s/%s".format(x, x.getClass, y, y.getClass))
+ // The following is blocked by SI-8150
+ // if (!isIffyB(x,y)) assert(x.## == y.##, "%x/%s != %x/%s from %s.## and %s.##".format(x.##, x.getClass, y.##, y.getClass, x, y))
+ }
}
}
}
@@ -0,0 +1,225 @@
+package scala.math
+
+import org.junit.runner.RunWith
+import org.junit.runners.JUnit4
+import org.junit.Test
+import java.math.{BigDecimal => BD, MathContext => MC}
+
+/* Tests various maps by making sure they all agree on the same answers. */
+@RunWith(classOf[JUnit4])
+class BigDecimalTest {
+
+ // Motivated by SI-6173: BigDecimal#isWhole implementation is very heap intensive
+ @Test
+ def isWholeTest() {
+ val wholes = List(
+ BigDecimal(1),
+ BigDecimal(10L),
+ BigDecimal(14.000),
+ BigDecimal(new BD("19127981892347012385719827340123471923850195")),
+ BigDecimal("1e1000000000"),
+ BigDecimal(14.1928857191985e22),
+ BigDecimal(14.12519823759817, new MC(2))
+ )
+ val fracs = List(
+ BigDecimal(0.1),
+ BigDecimal(new BD("1.000000000000000000000000000000000001")),
+ BigDecimal(new BD("275712375971892375127591745810580123751.99999")),
+ BigDecimal("14.19238571927581e6"),
+ BigDecimal("912834718237510238591285")/2
+ )
+ assert(wholes.forall(_.isWhole) && fracs.forall(! _.isWhole))
+ }
+
+ // Motivated by SI-6699: BigDecimal.isValidDouble behaves unexpectedly
+ @Test
+ def isValidDoubleTest() {
+ val valids = List(
+ BigDecimal(1),
+ BigDecimal(19571.125),
+ BigDecimal.decimal(0.1),
+ BigDecimal(1e15)
+ )
+ val invalids = List(
+ BigDecimal(new BD("1.0000000000000000000000000000000000000000001")),
+ BigDecimal("10e1000000"),
+ BigDecimal("10e-1000000")
+ )
+ assert(
+ valids.forall(_.isDecimalDouble) &&
+ invalids.forall(! _.isDecimalDouble)
+ )
+ }
+
+ // Motivated by SI-6173: BigDecimal#isWhole implementation is very heap intensive
+ @Test
+ def doesNotExplodeTest() {
+ val troublemaker = BigDecimal("1e1000000000")
+ val reasonable = BigDecimal("1e1000")
+ val reasonableInt = reasonable.toBigInt
+ assert(
+ reasonable.hashCode == reasonableInt.hashCode &&
+ reasonable == reasonableInt &&
+ reasonableInt == reasonable &&
+ troublemaker.hashCode != reasonable.hashCode &&
+ !(troublemaker == reasonableInt) &&
+ !(reasonableInt == troublemaker)
+ )
+ }
+
+ // Motivated by SI-6456: scala.math.BigDecimal should not accept a null value
+ @Test
+ def refusesNullTest() {
+ def isIAE[A](a: => A) = try { a; false } catch { case iae: IllegalArgumentException => true }
+ def isNPE[A](a: => A) = try { a; false } catch { case npe: NullPointerException => true }
+ assert(
+ isIAE(new BigDecimal(null: BD, new MC(2))) &&
+ isIAE(new BigDecimal(new BD("5.7"), null: MC)) &&
+ isNPE(BigDecimal(null: BigInt)) &&
+ isNPE(BigDecimal(null: String)) &&
+ isNPE(BigDecimal(null: Array[Char]))
+ )
+ }
+
+ // Motivated by SI-6153: BigDecimal.hashCode() has high collision rate
+ @Test
+ def hashCodesAgreeTest() {
+ val bi: BigInt = 100000
+ val bd: BigDecimal = 100000
+ val l: Long = 100000
+ val d: Double = 100000
+ assert(
+ d.## == l.## &&
+ l.## == bd.## &&
+ bd.## == bi.## &&
+ (bd pow 4).hashCode == (bi pow 4).hashCode &&
+ BigDecimal("1e150000").hashCode != BigDecimal("1e150000").toBigInt.hashCode
+ )
+ }
+
+ // Motivated by noticing BigDecimal(0.1f) != BigDecimal(0.1)
+ @Test
+ def consistentTenthsTest() {
+ def tenths = List[Any](
+ BigDecimal("0.1"),
+ 0.1,
+ BigDecimal.decimal(0.1f),
+ BigDecimal.decimal(0.1),
+ BigDecimal(0.1),
+ BigDecimal(BigInt(1), 1),
+ BigDecimal(new BD("0.1")),
+ BigDecimal(1L, 1),
+ BigDecimal(1) / BigDecimal(10),
+ BigDecimal(10).pow(-1)
+ )
+ for (a <- tenths; b <- tenths) assert(a == b, s"$a != $b but both should be 0.1")
+ }
+
+ // Motivated by noticing BigDecimal(123456789, mc6) != BigDecimal(123456789L, mc6)
+ // where mc6 is a MathContext that rounds to six digits
+ @Test
+ def consistentRoundingTest() {
+ val mc6 = new MC(6)
+ val sameRounding = List(
+ List(
+ 123457000,
+ 123457000L,
+ 123457e3,
+ BigDecimal(123456789, mc6),
+ BigDecimal(123456789L, mc6),
+ BigDecimal(123456789d, mc6),
+ BigDecimal("123456789", mc6),
+ BigDecimal(Array('1','2','3','4','5','6','7','8','9'), mc6),
+ BigDecimal(BigInt(123456789), mc6),
+ BigDecimal(BigInt(1234567890), 1, mc6),
+ BigDecimal.decimal(123456789, mc6),
+ BigDecimal.decimal(123456789d, mc6),
+ BigDecimal.decimal(new BD("123456789"), mc6)
+ ),
+ List(
+ 123456789,
+ 123456789L,
+ 123456789d,
+ new BigDecimal(new BD("123456789"), mc6),
+ new BigDecimal(new BD("123456789")),
+ BigDecimal(123456789),
+ BigDecimal(123456789L),
+ BigDecimal(123456789d),
+ BigDecimal("123456789"),
+ BigDecimal(Array('1','2','3','4','5','6','7','8','9')),
+ BigDecimal(BigInt(123456789)),
+ BigDecimal(BigInt(1234567890), 1),
+ BigDecimal.decimal(123456789),
+ BigDecimal.decimal(123456789d),
+ BigDecimal.valueOf(123456789d, mc6)
+ )
+ )
+ sameRounding.map(_.zipWithIndex).foreach{ case xs =>
+ for ((a,i) <- xs; (b,j) <- xs) {
+ assert(a == b, s"$a != $b (#$i != #$j) but should be the same")
+ assert(a.## == b.##, s"Hash code mismatch in equal BigDecimals: #$i != #$j")
+ }
+ }
+ val List(xs, ys) = sameRounding.map(_.zipWithIndex)
+ for ((a,i) <- xs; (b,j) <- ys) assert(a != b, s"$a == $b (#$i == #$j) but should be different")
+ }
+
+ // This was unexpectedly truncated in 2.10
+ @Test
+ def noPrematureRoundingTest() {
+ val text = "9791375983750284059237954823745923845928547807345082378340572986452364"
+ val same = List[Any](
+ BigInt(text), BigDecimal(text), BigDecimal(new BD(text))
+ )
+ for (a <- same; b <- same) assert(a == b, s"$a != $b but should be the same")
+ }
+
+ // Tests attempts to make sane the representation of IEEE binary32 and binary64
+ // (i.e. Float and Double) with Scala's text-is-King BigDecimal policy
+ @Test
+ def churnRepresentationTest() {
+ val rn = new scala.util.Random(42)
+ for (i <- 1 to 1000) {
+ val d = rn.nextDouble
+ assert({
+ BigDecimal.decimal(d).isDecimalDouble &&
+ BigDecimal.binary(d).isBinaryDouble &&
+ BigDecimal.exact(d).isExactDouble
+ }, s"At least one wrong BigDecimal representation for $d")
+ }
+ for (i <- 1 to 1000) {
+ val f = rn.nextFloat
+ assert({
+ BigDecimal.decimal(f).isDecimalFloat &&
+ BigDecimal.binary(f).isBinaryFloat &&
+ BigDecimal.exact(f).isExactFloat
+ }, s"At least one wrong BigDecimal representation for $f")
+ }
+ for (i <- 1 to 1000) {
+ val ndig = 15+rn.nextInt(5)
+ val s = Array.fill(ndig)((rn.nextInt(10)+'0').toChar).mkString
+ val bi = BigInt(s)
+ val l = bi.toLong
+ val d = bi.toDouble
+ val bd = BigDecimal(bi)
+ val bd2 = BigDecimal.decimal(d)
+ assert(!bi.isValidLong || bi == l, s"Should be invalid or equal: $bi $l")
+ assert(!bi.isValidDouble || bi == d, s"Should be invalid or equal: $bi $d")
+ assert(bd == bi, s"Should be equal $bi $bd")
+ assert(bd.## == bi.##, s"Hash codes for $bi, $bd should be equal")
+ assert(bd == bd2 || bd2 != BigDecimal.exact(d) || !bi.isValidDouble,
+ s"$bd != $bd2 should only be when inexact or invalid")
+ assert(d == bd2 && bd2 == d, s"$d != $bd2 but they should equal")
+ }
+ val different = List(
+ BigDecimal.decimal(0.1),
+ BigDecimal.binary(0.1),
+ BigDecimal.binary(0.1, new MC(25)),
+ BigDecimal.exact(0.1),
+ BigDecimal.exact(0.1f),
+ BigDecimal.decimal((0.1f).toDouble)
+ )
+ for (a <- different; b <- different if (a ne b))
+ assert(a != b, "BigDecimal representations of Double mistakenly conflated")
+ }
+}

0 comments on commit 29541ce

Please sign in to comment.