Why does toString fail to produce the correct value on an immutable BigDecimal?

It’s not so hard to track down the reason for the odd behavior.

The divide call goes to

public BigDecimal divide(BigDecimal divisor, int scale, RoundingMode roundingMode) {
    return divide(divisor, scale, roundingMode.oldMode);
}

This, internally, delegates to another divide method, based on the rounding mode:

public BigDecimal divide(BigDecimal divisor, int scale, int roundingMode) {
    if (roundingMode < ROUND_UP || roundingMode > ROUND_UNNECESSARY)
        throw new IllegalArgumentException("Invalid rounding mode");
    if (this.intCompact != INFLATED) {
        if ((divisor.intCompact != INFLATED)) {
            return divide(this.intCompact, this.scale, divisor.intCompact, divisor.scale, scale, roundingMode);
        } else {
            return divide(this.intCompact, this.scale, divisor.intVal, divisor.scale, scale, roundingMode);
        }
    } else {
        if ((divisor.intCompact != INFLATED)) {
            return divide(this.intVal, this.scale, divisor.intCompact, divisor.scale, scale, roundingMode);
        } else {
            return divide(this.intVal, this.scale, divisor.intVal, divisor.scale, scale, roundingMode);
        }
    }
}

In this case, the last call applies. Note that the intVal (which is a BigInteger that is stored in the BigDecimal) is passed directly to this method as the first argument:

private static BigDecimal divide(BigInteger dividend, int dividendScale, BigInteger divisor, int divisorScale, int scale, int roundingMode) {
    if (checkScale(dividend,(long)scale + divisorScale) > dividendScale) {
        int newScale = scale + divisorScale;
        int raise = newScale - dividendScale;
        BigInteger scaledDividend = bigMultiplyPowerTen(dividend, raise);
        return divideAndRound(scaledDividend, divisor, scale, roundingMode, scale);
    } else {
        int newScale = checkScale(divisor,(long)dividendScale - scale);
        int raise = newScale - divisorScale;
        BigInteger scaledDivisor = bigMultiplyPowerTen(divisor, raise);
        return divideAndRound(dividend, scaledDivisor, scale, roundingMode, scale);
    }
}

Finally, the path to the second divideAndRound is taken here, again passing the dividend on (which was the intVal of the original BigDecimal), ending up with this code:

private static BigDecimal divideAndRound(BigInteger bdividend, BigInteger bdivisor, int scale, int roundingMode,
                                         int preferredScale) {
    boolean isRemainderZero; // record remainder is zero or not
    int qsign; // quotient sign
    // Descend into mutables for faster remainder checks
    MutableBigInteger mdividend = new MutableBigInteger(bdividend.mag);
    MutableBigInteger mq = new MutableBigInteger();
    MutableBigInteger mdivisor = new MutableBigInteger(bdivisor.mag);
    MutableBigInteger mr = mdividend.divide(mdivisor, mq);
    ...

And this is where the error is introduced: The mdivididend is a mutable BigInteger, that was created as a mutable view on the mag array of the BigInteger that is stored in the BigDecimal x from the original call. The division modifies the mag field, and thus, the state of the (now not-so-immutable) BigDecimal.

This is clearly a bug in the implementation of one of the divide methods. I already started tracking the change sets of the OpenJDK, but have not yet spotted the definite culprit. (Edit: See updates below)

(A side note: Calling x.toString() before doing the division does not really avoid, but only hide the bug: It causes a string cache of the correct state to be created internally. The right value is printed, but the internal state is still wrong – which is concerning, to say the least…)


Update: To confirm what @MikeM said: Bug has been listed on openjdk bug list and it has been resolved in JDK8 Build 51

Update : Kudos to Mike and exex zian for digging out the bug reports. According to the discussion there, the bug was introduced with this changeset.
(Admittedly, while skimming through the changes, I also considered this as a hot candidate, but could not believe that this was introduced four years ago and remained unnoticed until now…)

Leave a Comment

Hata!: SQLSTATE[HY000] [1045] Access denied for user 'divattrend_liink'@'localhost' (using password: YES)