Remove some unneeded Arbitrary and Gen instances

[larsrh]

Follow-up to #3772.

ScalaCheck already provides instances for Duration and FiniteDuration. We should use them and delete ours. If it turns out that the ScalaCheck provided instances don't work for us, we should fix them upstream.

Requires deleting some lines in https://github.com/typelevel/cats/blob/master/kernel-laws/shared/src/test/scala/cats/kernel/laws/LawTests.scala.

[tnielens]

Removing cats overrides of Duration and FiniteDuration crashes the tests on at least the two following issues:

• integer overflow

ScalaTestFailureLocation: org.scalatestplus.scalacheck.CheckerAsserting$$anon$2 at (LawTests.scala:248)
org.scalatest.exceptions.GeneratorDrivenPropertyCheckFailedException: IllegalArgumentException was thrown during property evaluation.
  Message: integer overflow
  Occurred when passed generated values (
    arg0 = 9223372036854775807 nanoseconds,
    arg1 = 9223372036854775807 nanoseconds
  )
	at org.scalatestplus.scalacheck.CheckerAsserting$$anon$2.indicateFailure(CheckerAsserting.scala:251)
	at org.scalatestplus.scalacheck.CheckerAsserting$$anon$2.indicateFailure(CheckerAsserting.scala:238)
	at org.scalatestplus.scalacheck.UnitCheckerAsserting$CheckerAssertingImpl.check(CheckerAsserting.scala:160)
	at org.scalatestplus.scalacheck.Checkers.check(Checkers.scala:371)
	at org.scalatestplus.scalacheck.Checkers.check$(Checkers.scala:369)
	...
Caused by: java.lang.IllegalArgumentException: integer overflow
	at scala.concurrent.duration.FiniteDuration.safeAdd(Duration.scala:614)
	at scala.concurrent.duration.FiniteDuration.add(Duration.scala:619)
	at scala.concurrent.duration.FiniteDuration.$plus(Duration.scala:652)
	at cats.kernel.instances.FiniteDurationGroup.combine(FiniteDurationInstances.scala:42)

• Duration.Undefined not being equal to itself

GeneratorDrivenPropertyCheckFailedException was thrown during property evaluation.
 (LawTests.scala:246)
  Falsified after 22 successful property evaluations.
  Location: (LawTests.scala:246)
  Occurred when passed generated values (
    arg0 = Duration.Undefined
  )
  Label of failing property:
    Expected: Duration.Undefined
Received: Duration.Undefined

[larsrh]

Is this something we can fix upstream?

[tnielens]

What's the policy for scalacheck default Arbitrary instances? Is "it breaks cats law tests" a sufficient reason for accepting changes?
Duration.Undefined breaks most laws. It indeed makes implementation of a lawful Order[Duration] for example (and probably many others) impossible.

As for the overflow issue, I'm giving a second look at cat's current law tests implementation as it uses default Arbitrary instances of numerical primitive types without overflowing issues.

[larsrh]

Are you sure it's the Arbitrary instance that breaks the laws, and not the Cogen instance?

[larsrh]

If it's just about Duration.Undefined I'd prefer taking the upstream instance and filtering out this value. It's cleaner and more explicit than to define our own instance that happens to omit Undefined.

[tnielens]

Are you sure it's the Arbitrary instance that breaks the laws, and not the Cogen instance?

The tests still pass when I remove cats' overrides of the Cogen instances. They fail when I remove the Arbitrary overrides.

If it's just about Duration.Undefined I'd prefer taking the upstream instance and filtering out this value. It's cleaner and more explicit than to define our own instance that happens to omit Undefined.

Will try that.

As for the overflow, Duration has an exception-throwing implementation of addition and substraction upon overflow, contrary to numerical primitives which silently overflow. The default instance must be constrained further.

[larsrh]

Thanks a lot for your investigation.

[tnielens]

It seems that filtering the default instances from scalacheck doesn't work.
Here is my attempt:

  val overflowPreventionFactor = 500
  val maxNs = Long.MaxValue / overflowPreventionFactor
  implicit val arbitraryDuration: Arbitrary[Duration] =
    Arbitrary(
      Gen.duration
        .filter {
          case fd: FiniteDuration => fd.toNanos >= - maxNs && fd.toNanos <= maxNs
          case _ => true
        }
        .filterNot(_ eq Duration.Undefined)
    )

• there are still issues with Duration.Undefined as adding Duration.Inf and Duration.MinusInf yields Duration.Undefined. The Monoid and Group laws bump into this.
• the fitlering discards too many results: Gave up after 99 successful property evaluations. 938 evaluations were discarded.

What can still be done is deleting the Cogen instances, and deduplicate the FiniteDuration/Duration instance.

[tnielens]

Thanks @larsrh for opening this small issue and for the high reactivity 👍

[larsrh]

Thank you for taking care of it 😄