From 23b2ac6c6f7d90bc9ed11aa0fdb799a70f1b3f81 Mon Sep 17 00:00:00 2001 From: Timothy McCarthy Date: Tue, 31 May 2022 21:01:46 +1000 Subject: [PATCH 1/2] Use Fractional[MiniInt] to test Invariant[Fractional] Currently we use Float when doing laws testing for Invariant[Fractional]. This in turn requires us to generate an Eq[Fractional[Float]], which can only be done using DeprecatedEqInstances.catsLawsEqForFn1. In order to stop using this deprecated instances, in this change we now use MiniInt to do our laws testing for Invariant[Fractional]. Note that this approach is spurious since MiniInt is not a fractional number type. But we can use it to test the lawfulness of the Invariant[Fractional] instance, and there is no "MiniFloat" type that is both small enough to have an ExhaustiveCheck instance and fractional. See https://github.com/typelevel/cats/pull/4033 for a broader discussion of this. --- .../cats/tests/ScalaVersionSpecific.scala | 26 +------------ .../cats/tests/ScalaVersionSpecific.scala | 35 +---------------- .../cats/tests/AlgebraInvariantSuite.scala | 38 +++++++++++++++---- 3 files changed, 33 insertions(+), 66 deletions(-) diff --git a/tests/shared/src/test/scala-2.12/cats/tests/ScalaVersionSpecific.scala b/tests/shared/src/test/scala-2.12/cats/tests/ScalaVersionSpecific.scala index 62b1a1346b..fa4e29a511 100644 --- a/tests/shared/src/test/scala-2.12/cats/tests/ScalaVersionSpecific.scala +++ b/tests/shared/src/test/scala-2.12/cats/tests/ScalaVersionSpecific.scala @@ -25,8 +25,6 @@ import cats.kernel.{Eq, Order} import cats.laws.discipline.{ExhaustiveCheck, MiniInt} import cats.laws.discipline.MiniInt._ import cats.laws.discipline.eq._ -import cats.laws.discipline.DeprecatedEqInstances -import org.scalacheck.Arbitrary trait ScalaVersionSpecificFoldableSuite trait ScalaVersionSpecificParallelSuite @@ -35,7 +33,7 @@ trait ScalaVersionSpecificTraverseSuite trait ScalaVersionSpecificAlgebraInvariantSuite { // This version-specific instance is required since 2.12 and below do not have parseString on the Numeric class - protected val integralForMiniInt: Integral[MiniInt] = new Integral[MiniInt] { + protected trait MiniIntNumeric extends Numeric[MiniInt] { def compare(x: MiniInt, y: MiniInt): Int = Order[MiniInt].compare(x, y) def plus(x: MiniInt, y: MiniInt): MiniInt = x + y def minus(x: MiniInt, y: MiniInt): MiniInt = x + (-y) @@ -46,8 +44,6 @@ trait ScalaVersionSpecificAlgebraInvariantSuite { def toLong(x: MiniInt): Long = x.toInt.toLong def toFloat(x: MiniInt): Float = x.toInt.toFloat def toDouble(x: MiniInt): Double = x.toInt.toDouble - def quot(x: MiniInt, y: MiniInt): MiniInt = MiniInt.unsafeFromInt(x.toInt / y.toInt) - def rem(x: MiniInt, y: MiniInt): MiniInt = MiniInt.unsafeFromInt(x.toInt % y.toInt) } // This version-specific instance is required since 2.12 and below do not have parseString on the Numeric class @@ -75,24 +71,4 @@ trait ScalaVersionSpecificAlgebraInvariantSuite { ) } - // This version-specific instance is required since 2.12 and below do not have parseString on the Numeric class - @annotation.nowarn("cat=deprecation") - implicit protected def eqFractional[A: Eq: Arbitrary]: Eq[Fractional[A]] = { - import DeprecatedEqInstances.catsLawsEqForFn1 - - Eq.by { fractional => - ( - fractional.compare _, - fractional.plus _, - fractional.minus _, - fractional.times _, - fractional.negate _, - fractional.fromInt _, - fractional.toInt _, - fractional.toLong _, - fractional.toFloat _, - fractional.toDouble _ - ) - } - } } diff --git a/tests/shared/src/test/scala-2.13+/cats/tests/ScalaVersionSpecific.scala b/tests/shared/src/test/scala-2.13+/cats/tests/ScalaVersionSpecific.scala index 52ce3cc375..e8978b6878 100644 --- a/tests/shared/src/test/scala-2.13+/cats/tests/ScalaVersionSpecific.scala +++ b/tests/shared/src/test/scala-2.13+/cats/tests/ScalaVersionSpecific.scala @@ -23,7 +23,6 @@ package cats.tests import cats._ import cats.data.NonEmptyLazyList -import cats.laws.discipline.DeprecatedEqInstances import cats.laws.discipline.ExhaustiveCheck import cats.laws.discipline.MiniInt import cats.laws.discipline.NonEmptyParallelTests @@ -31,7 +30,6 @@ import cats.laws.discipline.ParallelTests import cats.laws.discipline.arbitrary._ import cats.laws.discipline.eq._ import cats.syntax.all._ -import org.scalacheck.Arbitrary import org.scalacheck.Prop._ trait ScalaVersionSpecificFoldableSuite { self: FoldableSuiteAdditional => @@ -191,7 +189,7 @@ trait ScalaVersionSpecificTraverseSuite { self: TraverseSuiteAdditional => trait ScalaVersionSpecificAlgebraInvariantSuite { // This version-specific instance is required since 2.12 and below do not have parseString on the Numeric class - protected val integralForMiniInt: Integral[MiniInt] = new Integral[MiniInt] { + protected trait MiniIntNumeric extends Numeric[MiniInt] { def compare(x: MiniInt, y: MiniInt): Int = Order[MiniInt].compare(x, y) def plus(x: MiniInt, y: MiniInt): MiniInt = x + y def minus(x: MiniInt, y: MiniInt): MiniInt = x + (-y) @@ -202,8 +200,6 @@ trait ScalaVersionSpecificAlgebraInvariantSuite { def toLong(x: MiniInt): Long = x.toInt.toLong def toFloat(x: MiniInt): Float = x.toInt.toFloat def toDouble(x: MiniInt): Double = x.toInt.toDouble - def quot(x: MiniInt, y: MiniInt): MiniInt = MiniInt.unsafeFromInt(x.toInt / y.toInt) - def rem(x: MiniInt, y: MiniInt): MiniInt = MiniInt.unsafeFromInt(x.toInt % y.toInt) def parseString(str: String): Option[MiniInt] = Integral[Int].parseString(str).flatMap(MiniInt.fromInt) } @@ -238,35 +234,6 @@ trait ScalaVersionSpecificAlgebraInvariantSuite { ) } - // This version-specific instance is required since 2.12 and below do not have parseString on the Numeric class - @annotation.nowarn("cat=deprecation") - implicit protected def eqFractional[A: Eq: Arbitrary]: Eq[Fractional[A]] = { - // This deprecated instance is required since there is not `ExhaustiveCheck` for any types for which a `Fractional` - // can easily be defined - import DeprecatedEqInstances.catsLawsEqForFn1 - - Eq.by { fractional => - val parseFloatStrings: Option[Double] => Option[A] = { - case Some(f) => fractional.parseString(f.toString) - case None => fractional.parseString("invalid") // Use this to test parsing of non-numeric strings - } - - ( - fractional.compare _, - fractional.plus _, - fractional.minus _, - fractional.times _, - fractional.negate _, - fractional.fromInt _, - fractional.toInt _, - fractional.toLong _, - fractional.toFloat _, - fractional.toDouble _, - parseFloatStrings - ) - } - } - } class TraverseLazyListSuite extends TraverseSuite[LazyList]("LazyList") diff --git a/tests/shared/src/test/scala/cats/tests/AlgebraInvariantSuite.scala b/tests/shared/src/test/scala/cats/tests/AlgebraInvariantSuite.scala index ea083a372f..70be9b3127 100644 --- a/tests/shared/src/test/scala/cats/tests/AlgebraInvariantSuite.scala +++ b/tests/shared/src/test/scala/cats/tests/AlgebraInvariantSuite.scala @@ -179,11 +179,27 @@ class AlgebraInvariantSuite extends CatsSuite with ScalaVersionSpecificAlgebraIn implicit private val arbCommutativeGroupInt: Arbitrary[CommutativeGroup[Int]] = Arbitrary(genCommutativeGroupInt) + protected val integralForMiniInt: Numeric[MiniInt] with Integral[MiniInt] = new MiniIntNumeric + with Integral[MiniInt] { + def quot(x: MiniInt, y: MiniInt): MiniInt = MiniInt.unsafeFromInt(x.toInt / y.toInt) + def rem(x: MiniInt, y: MiniInt): MiniInt = MiniInt.unsafeFromInt(x.toInt % y.toInt) + } + + // This is a spurious instance since MiniInt is not a Fractional data type. But we use it since we don't have a + // Fractional type for which ExhaustiveCheck is also implemented. See https://github.com/typelevel/cats/pull/4033 + protected val fractionalForMiniInt: Numeric[MiniInt] with Fractional[MiniInt] = new MiniIntNumeric + with Fractional[MiniInt] { + def div(x: MiniInt, y: MiniInt): MiniInt = + if (y == MiniInt.zero) { + MiniInt.maxValue + } else { + x / y + } + } + implicit private val arbNumericMiniInt: Arbitrary[Numeric[MiniInt]] = Arbitrary(Gen.const(integralForMiniInt)) implicit private val arbIntegralMiniInt: Arbitrary[Integral[MiniInt]] = Arbitrary(Gen.const(integralForMiniInt)) - implicit private val arbFractionalFloat: Arbitrary[Fractional[Float]] = Arbitrary( - Gen.const(implicitly[Fractional[Float]]) - ) + implicit private val arbFractionalFloat: Arbitrary[Fractional[MiniInt]] = Arbitrary(Gen.const(fractionalForMiniInt)) implicit protected def eqIntegral[A: Eq: ExhaustiveCheck]: Eq[Integral[A]] = { def makeDivisionOpSafe(unsafeF: (A, A) => A): (A, A) => Option[A] = @@ -209,6 +225,14 @@ class AlgebraInvariantSuite extends CatsSuite with ScalaVersionSpecificAlgebraIn } } + implicit protected def eqFractional[A: Eq: ExhaustiveCheck]: Eq[Fractional[A]] = + Eq.by { fractional => + ( + fractional: Numeric[A], + fractional.div(_, _) + ) + } + checkAll("InvariantMonoidal[Semigroup]", SemigroupTests[Int](InvariantMonoidal[Semigroup].point(0)).semigroup) checkAll("InvariantMonoidal[CommutativeSemigroup]", CommutativeSemigroupTests[Int](InvariantMonoidal[CommutativeSemigroup].point(0)).commutativeSemigroup @@ -218,10 +242,6 @@ class AlgebraInvariantSuite extends CatsSuite with ScalaVersionSpecificAlgebraIn InvariantSemigroupalTests[Monoid].invariantSemigroupal[Option[MiniInt], Option[Boolean], Option[Boolean]] ) - checkAll("Invariant[Numeric]", InvariantTests[Numeric].invariant[MiniInt, Boolean, Boolean]) - checkAll("Invariant[Integral]", InvariantTests[Integral].invariant[MiniInt, Boolean, Boolean]) - checkAll("Invariant[Fractional]", InvariantTests[Fractional].invariant[Float, Boolean, Boolean]) - { val S: Semigroup[Int] = Semigroup[Int].imap(identity)(identity) checkAll("Semigroup[Int]", SemigroupTests[Int](S).semigroup) @@ -310,6 +330,10 @@ class AlgebraInvariantSuite extends CatsSuite with ScalaVersionSpecificAlgebraIn checkAll("Invariant[CommutativeGroup]", InvariantTests[CommutativeGroup].invariant[MiniInt, Boolean, Boolean]) checkAll("Invariant[CommutativeGroup]", SerializableTests.serializable(Invariant[CommutativeGroup])) + checkAll("Invariant[Numeric]", InvariantTests[Numeric].invariant[MiniInt, Boolean, Boolean]) + checkAll("Invariant[Integral]", InvariantTests[Integral].invariant[MiniInt, Boolean, Boolean]) + checkAll("Invariant[Fractional]", InvariantTests[Fractional].invariant[MiniInt, Boolean, Boolean]) + checkAll("InvariantMonoidal[Semigroup]", InvariantMonoidalTests[Semigroup].invariantMonoidal[Option[MiniInt], Option[Boolean], Option[Boolean]] ) From 75c8d09385fd010197fad7f4082e48305cf860fc Mon Sep 17 00:00:00 2001 From: Timothy McCarthy Date: Sun, 5 Jun 2022 19:55:53 +1000 Subject: [PATCH 2/2] Reduce scope of spurious Fractional[MiniInt] instance as per PR feedback --- .../cats/tests/AlgebraInvariantSuite.scala | 30 ++++++++++--------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/tests/shared/src/test/scala/cats/tests/AlgebraInvariantSuite.scala b/tests/shared/src/test/scala/cats/tests/AlgebraInvariantSuite.scala index 70be9b3127..aacb1cc9e3 100644 --- a/tests/shared/src/test/scala/cats/tests/AlgebraInvariantSuite.scala +++ b/tests/shared/src/test/scala/cats/tests/AlgebraInvariantSuite.scala @@ -185,21 +185,8 @@ class AlgebraInvariantSuite extends CatsSuite with ScalaVersionSpecificAlgebraIn def rem(x: MiniInt, y: MiniInt): MiniInt = MiniInt.unsafeFromInt(x.toInt % y.toInt) } - // This is a spurious instance since MiniInt is not a Fractional data type. But we use it since we don't have a - // Fractional type for which ExhaustiveCheck is also implemented. See https://github.com/typelevel/cats/pull/4033 - protected val fractionalForMiniInt: Numeric[MiniInt] with Fractional[MiniInt] = new MiniIntNumeric - with Fractional[MiniInt] { - def div(x: MiniInt, y: MiniInt): MiniInt = - if (y == MiniInt.zero) { - MiniInt.maxValue - } else { - x / y - } - } - implicit private val arbNumericMiniInt: Arbitrary[Numeric[MiniInt]] = Arbitrary(Gen.const(integralForMiniInt)) implicit private val arbIntegralMiniInt: Arbitrary[Integral[MiniInt]] = Arbitrary(Gen.const(integralForMiniInt)) - implicit private val arbFractionalFloat: Arbitrary[Fractional[MiniInt]] = Arbitrary(Gen.const(fractionalForMiniInt)) implicit protected def eqIntegral[A: Eq: ExhaustiveCheck]: Eq[Integral[A]] = { def makeDivisionOpSafe(unsafeF: (A, A) => A): (A, A) => Option[A] = @@ -332,7 +319,22 @@ class AlgebraInvariantSuite extends CatsSuite with ScalaVersionSpecificAlgebraIn checkAll("Invariant[Numeric]", InvariantTests[Numeric].invariant[MiniInt, Boolean, Boolean]) checkAll("Invariant[Integral]", InvariantTests[Integral].invariant[MiniInt, Boolean, Boolean]) - checkAll("Invariant[Fractional]", InvariantTests[Fractional].invariant[MiniInt, Boolean, Boolean]) + + { + // This is a spurious instance since MiniInt is not a Fractional data type. But we use it since we don't have a + // Fractional type for which ExhaustiveCheck is also implemented. See https://github.com/typelevel/cats/pull/4033 + val fractionalForMiniInt: Fractional[MiniInt] = new MiniIntNumeric with Fractional[MiniInt] { + def div(x: MiniInt, y: MiniInt): MiniInt = + if (y == MiniInt.zero) { + MiniInt.maxValue + } else { + x / y + } + } + implicit val arbFractionalMiniInt: Arbitrary[Fractional[MiniInt]] = Arbitrary(Gen.const(fractionalForMiniInt)) + + checkAll("Invariant[Fractional]", InvariantTests[Fractional].invariant[MiniInt, Boolean, Boolean]) + } checkAll("InvariantMonoidal[Semigroup]", InvariantMonoidalTests[Semigroup].invariantMonoidal[Option[MiniInt], Option[Boolean], Option[Boolean]]