CommutativeGroup for BigDecimal isn't actually commutative
Nobody has claimed this yet.
- Dominant language
- Scala
- Stars
- 5.5k
- Forks
- 1.2k
- Avg merge
- 2d 10h
- Merged PRs (30d)
- 5
Description
…but only since you can observe the MathContext it carries along with it:
scala> import cats.implicits._, java.math.MathContext
import cats.implicits._
import java.math.MathContext
scala> val zero = BigDecimal("0")
zero: scala.math.BigDecimal = 0
scala> val zeroU = BigDecimal("0", MathContext.UNLIMITED)
zeroU: scala.math.BigDecimal = 0
scala> (zero |+| zeroU).mc
res0: java.math.MathContext = precision=34 roundingMode=HALF_EVEN
scala> (zeroU |+| zero).mc
res1: java.math.MathContext = precision=0 roundingMode=HALF_UP
This isn't cool, but I'm also not sure it's a major emergency, or even that it needs to be fixed, for a few reasons:
- The current behavior is the same as the standard library's and will be expected by many users. Changing it to enforce commutativity is likely to break at least some users' expectations.
- Java's
BigDecimal(which Scala'sBigDecimalwraps) doesn't have aMathContextattached to instances at all, and many users of Scala'sBigDecimaluse it in that way. - Our
Eqinstances forBigDecimaldon't compareMathContext, so with respect to Cats's idea of equality forBigDecimals the instance actually is commutative.
@johnynek has argued in #3303 that we should do one of two things:
- Define an ordering on
MathContextand use that (probably via the max) in our implicitCommutativeGroupforBigDecimal. - Remove the implicit
CommutativeGroupinstance (possibly leaving an implicitGroupwith no commutativity guarantee, I guess?) and provide a method that takes aMathContextand returns a
CommutativeGroup[BigDecimal].
If we do this we should probably also change our Eq instance for BigDecimal to make it compare the MathContexts (at least in our tests).
My vote is not to do anything, since that's the least likely to surprise users and doesn't seem terribly inconsistent to me, but I wanted to make sure all the options are on the table.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by locating the implicit CommutativeGroup and Eq instances for Scala BigDecimal and reproduce the MathContext examples from the issue. Review the alternatives described in the discussion, including ordering MathContext or removing the implicit instance. Done requires a decided compatibility-preserving behavior and corresponding tests, but the issue does not specify which option to implement.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- scala
- Domain
- tooling
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100