Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CommutativeGroup for BigDecimal isn't actually commutative #3317

Open
travisbrown opened this issue Feb 24, 2020 · 0 comments
Open

CommutativeGroup for BigDecimal isn't actually commutative #3317

travisbrown opened this issue Feb 24, 2020 · 0 comments

Comments

@travisbrown
Copy link
Contributor

travisbrown commented Feb 24, 2020

…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's BigDecimal wraps) doesn't have a MathContext attached to instances at all, and many users of Scala's BigDecimal use it in that way.
  • Our Eq instances for BigDecimal don't compare MathContext, so with respect to Cats's idea of equality for BigDecimals the instance actually is commutative.

@johnynek has argued in #3303 that we should do one of two things:

  • Define an ordering on MathContext and use that (probably via the max) in our implicit CommutativeGroup for BigDecimal.
  • Remove the implicit CommutativeGroup instance (possibly leaving an implicit Group with no commutativity guarantee, I guess?) and provide a method that takes a MathContext and 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant