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.

Sign up for GitHub

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

Jump to bottom

Issue 2657 - Ambiguous duration instances #2670

Merged
merged 9 commits into from
Jan 9, 2019
Merged

Issue 2657 - Ambiguous duration instances #2670

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
cf071c9
Added test
barambani Dec 16, 2018
dcf0dd0
Fixed ambiguous implicits for FiniteDuration
barambani Dec 16, 2018
ef4b0c6
Fixed tests
barambani Dec 16, 2018
b0b18e4
Refactoring to reduce allocation on direct use
barambani Dec 16, 2018
b6f3a3e
Pre PR
barambani Dec 16, 2018
baed41b
Test case
barambani Dec 16, 2018
0dcd029
Mitigated existing problematic use case
barambani Dec 20, 2018
708ee5c
prePR
barambani Dec 20, 2018
294b2e8
Removed unneeded method
barambani Jan 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions core/src/main/scala/cats/implicits.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,4 @@ object implicits
with instances.AllInstancesBinCompat0
with instances.AllInstancesBinCompat1
with instances.AllInstancesBinCompat2
with instances.AllInstancesBinCompat3
2 changes: 2 additions & 0 deletions core/src/main/scala/cats/instances/all.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,3 +44,5 @@ trait AllInstancesBinCompat1
with SortedMapInstancesBinCompat0

trait AllInstancesBinCompat2 extends DurationInstances with FiniteDurationInstances

trait AllInstancesBinCompat3 extends AllCoreDurationInstances
21 changes: 19 additions & 2 deletions core/src/main/scala/cats/instances/duration.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,26 @@
package cats
package instances

import scala.concurrent.duration.Duration
import scala.concurrent.duration.{Duration, FiniteDuration}

trait DurationInstances extends cats.kernel.instances.DurationInstances {
implicit val catsStdShowForDuration: Show[Duration] =

@deprecated("Left to keep binary compatibility. Use CoreDurationInstances.catsStdShowForDurationUnambiguous instead.",
"1.5.0")
val catsStdShowForDuration: Show[Duration] =
AllCoreDurationInstances.catsStdShowForDurationUnambiguous
}

private[instances] trait AllCoreDurationInstances extends CoreFiniteDurationInstances

private[instances] trait CoreFiniteDurationInstances extends CoreDurationInstances {
implicit final val catsStdShowForFiniteDurationUnambiguous: Show[FiniteDuration] =
Show.fromToString[FiniteDuration]
}

private[instances] trait CoreDurationInstances {
implicit final val catsStdShowForDurationUnambiguous: Show[Duration] =
Show.fromToString[Duration]
}

private[instances] object AllCoreDurationInstances extends AllCoreDurationInstances
9 changes: 7 additions & 2 deletions core/src/main/scala/cats/instances/finiteDuration.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@ package instances
import scala.concurrent.duration.FiniteDuration

trait FiniteDurationInstances extends cats.kernel.instances.FiniteDurationInstances {
implicit val catsStdShowForFiniteDuration: Show[FiniteDuration] =
Show.fromToString[FiniteDuration]

@deprecated(
"Left to keep binary compatibility. Use CoreFiniteDurationInstances.catsStdShowForFiniteDurationUnambiguous instead.",
"1.5.0"
)
val catsStdShowForFiniteDuration: Show[FiniteDuration] =
AllCoreDurationInstances.catsStdShowForFiniteDurationUnambiguous
}
4 changes: 2 additions & 2 deletions core/src/main/scala/cats/instances/package.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,12 @@ package object instances {
object byte extends ByteInstances
object char extends CharInstances
object double extends DoubleInstances
object duration extends DurationInstances
object duration extends CoreDurationInstances with DurationInstances
Copy link
Contributor Author

@barambani barambani Dec 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still trying to solve an issue here as importing both cats.instances.duration._ and cats.instances.finiteDuration._ explicitly prevents for the instance to be resolved. The scenario is when there's the need for kernel instances together with core's

object test {

  import cats.instances.duration._
  import cats.instances.finiteDuration._

  implicitly[Order[Duration]]

  implicitly[ContravariantShow[FiniteDuration]]
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is why we don't have variance in type classes. I'd say in this case user have to use workarounds.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found a way to mitigate the problem. Now the only case that would fail is the below (see these test cases as an example).

test("fails") {
  import cats.instances.duration._
  import cats.instances.finiteDuration._

  implicitly[ContravariantShow[Duration]] // fails compilation
  implicitly[ContravariantShow[FiniteDuration]] // is inferred
}

I couldn't find a way to remove also this drawback.

object either extends EitherInstances
object eq extends EqInstances
object equiv extends EquivInstances
object float extends FloatInstances
object finiteDuration extends FiniteDurationInstances
object finiteDuration extends CoreFiniteDurationInstances with FiniteDurationInstances
object function extends FunctionInstances with FunctionInstancesBinCompat0
object future extends FutureInstances
object int extends IntInstances
Expand Down
9 changes: 8 additions & 1 deletion testkit/src/main/scala/cats/tests/CatsSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,13 @@ package tests

import catalysts.Platform

import cats.instances.{AllInstances, AllInstancesBinCompat0, AllInstancesBinCompat1, AllInstancesBinCompat2}
import cats.instances.{
AllInstances,
AllInstancesBinCompat0,
AllInstancesBinCompat1,
AllInstancesBinCompat2,
AllInstancesBinCompat3
}
import cats.syntax.{
AllSyntax,
AllSyntaxBinCompat0,
Expand Down Expand Up @@ -47,6 +53,7 @@ trait CatsSuite
with AllInstancesBinCompat0
with AllInstancesBinCompat1
with AllInstancesBinCompat2
with AllInstancesBinCompat3
with AllSyntax
with AllSyntaxBinCompat0
with AllSyntaxBinCompat1
Expand Down
44 changes: 43 additions & 1 deletion tests/src/test/scala/cats/tests/ShowSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
package cats
package tests

import cats.Contravariant
import java.util.concurrent.TimeUnit

import cats.Show.ContravariantShow
import cats.laws.discipline.arbitrary._
import cats.laws.discipline.{ContravariantTests, SerializableTests}
import cats.laws.discipline.eq._
import org.scalatest.FunSuite

import scala.concurrent.duration.{Duration, FiniteDuration}

class ShowSuite extends CatsSuite {
checkAll("Contravariant[Show]", ContravariantTests[Show].contravariant[Int, Int, Int])
Expand Down Expand Up @@ -34,4 +39,41 @@ class ShowSuite extends CatsSuite {

assertResult("Good morning")(show"Good $tod")
}

test("contravariant show is not ambiguous when both FiniteDuration's and Duration's instances are in scope") {
implicitly[ContravariantShow[FiniteDuration]]
implicitly[ContravariantShow[Duration]]
}

test("show interpolation works when both FiniteDuration's and Duration's instances are in scope") {
show"instance resolution is not ambiguous for ${FiniteDuration(3L, TimeUnit.SECONDS)}"
show"instance resolution is not ambiguous for ${Duration(3L, TimeUnit.SECONDS)}"
}
}

final class ShowSuite2 extends FunSuite {

test(
"contravariant show for FiniteDuration can be inferred when importing both duration's and finiteDuration's instances"
) {

import cats.instances.duration._
import cats.instances.finiteDuration._

implicitly[Order[Duration]]
implicitly[Order[FiniteDuration]]

implicitly[ContravariantShow[FiniteDuration]]
}

test("all the Duration's and FiniteDuration's instances can be correctly inferred when importing implicits") {

import cats.implicits._

implicitly[Order[Duration]]
implicitly[Order[FiniteDuration]]

implicitly[ContravariantShow[Duration]]
implicitly[ContravariantShow[FiniteDuration]]
}
}