Create Empty from EmptyK #4714
Create Empty from EmptyK #4714
Conversation
|
Fixes #3617 |
|
The tests seem flaky, I think we need a maintainer to rerun only the failed ones |
| @@ -38,7 +38,7 @@ object Empty extends EmptyInstances0 { | |||
| def apply[A](a: => A): Empty[A] = | |||
| new Empty[A] { lazy val empty: A = a } | |||
|
|
|||
| def fromEmptyK[F[_], T](implicit ekf: EmptyK[F]): Empty[F[T]] = ekf.synthesize[T] | |||
| implicit def fromEmptyK[F[_], T](implicit ekf: EmptyK[F]): Empty[F[T]] = ekf.synthesize[T] | |||
There was a problem hiding this comment.
I'm worried making this implicit here may create resolution issues. It can certainly change the resolution order. For instance, currently at EmptyInstances0 extends compat.IterableEmptyInstance so the change in this PR will put an EmptyK instance ahead of anything there.
It's hard to say if this is bad or not, because this is a lawless typeclass which is in alleycats. I'm more concerned with the possibility of something compiling that didn't previously due to ambiguous implicit resolution.
I would feel safer to me if we add an EmptyInstances2 which EmptyInstances1 extends and put this implicit there (although, we may or may not need to just add a new function rather than move this function because of MIMA concerns).
There was a problem hiding this comment.
Pushed with your suggestion.
MIMA should tell us if it's not ok to move the function right?
There was a problem hiding this comment.
@armanbilge could you rerun the failing test runs please? :)
|
Error was: Rerunning. |
|
@rossabaker @armanbilge Good to go? |
|
Code looks good. Would it be possible to add a quick test, similar to what's in that Scastie, just to demonstrate that it compiles now? Or even just the |
|
Added a test but open to suggestions on improving it. It's hard because we need to do it without Kittens |
| implicit def emptyA[T: Empty]: Empty[A[T]] = new Empty[A[T]] { | ||
| def empty: A[T] = A(Empty[T].empty) | ||
| } | ||
| implicit val emptyB: Empty[B] = new Empty[B] { |
There was a problem hiding this comment.
You don't need this do you? Shouldn't we have an empty Option[B] for all B?
| @@ -54,6 +54,21 @@ object SyntaxSuite { | |||
| val a1: Boolean = x.nonEmpty | |||
| } | |||
|
|
|||
| def testOptionEmpty[A: Empty]: Unit = { | |||
There was a problem hiding this comment.
Where is this type parameter used? I don't see it. Can we remove it?
There was a problem hiding this comment.
Not sure why I did that 🤦🏻♂️
|
@rossabaker Feel free to merge it. Thanks everyone |
This is solving the case where we have an instance of
Empty[T]but looking for aEmpty[Option[T]]:https://scastie.scala-lang.org/Jv24ybPQRQeQ2IEZy5azmA