Add implicit variance support for Profunctor typeclass #3495
Conversation
| * scala> val x2: Int => Foo = x1.rightWiden | ||
| * }}} | ||
| */ | ||
| def rightWiden[A, B, BB >: B](fab: F[A, B]): F[A, BB] = fab.asInstanceOf[F[A, BB]] |
There was a problem hiding this comment.
added these both to avoid lmap and rmap with identity
| @@ -23,24 +25,45 @@ class VarianceSuite extends CatsSuite { | |||
| } | |||
|
|
|||
| test("Auto-variance should widen a bifunctor automatically") { | |||
| def shouldInfer[F[_, _]: Bifunctor](fi: F[Int, Int]) = | |||
| def shouldInfer[F[_, _]: Bifunctor](fi: F[Int, Int]): F[Either[Bar, Baz.type], Either[Bar, Baz.type]] = | |||
There was a problem hiding this comment.
minor, thought that the test would be more readable if explicitly typed
There was a problem hiding this comment.
The idea was to assert here that scalac can infer the correct result type :)
There was a problem hiding this comment.
Sorry, I think I misunderstood something-
inferred method supposed to return F[Either[Foo, Foo], Either[Foo, Foo]], is still able to return F[Either[Bar, Baz.type], Either[Bar, Baz.type]]; this would have been impossible unless F was declared covariant (given Either is already covariant for both it's type parameters).
And this still compiles for we have implicit autoWidenProfunctor in scope.
please let me know if I should revert
There was a problem hiding this comment.
The test is still checking that the conversion happens at calls site
def inferred[F[_, _]: Bifunctor](fi: F[Int, Int]): F[Either[Foo, Foo], Either[Foo, Foo]] = shouldInfer[F](fi)If we remove the import cats.conversions.all._ the compilation fails also in this version, so the test is still valid. I don't have strong preferences about the type annotation.
There was a problem hiding this comment.
Now if I think some more, between the two not having types is still more useful, thanks @LukaJCB
Codecov Report
@@ Coverage Diff @@
## master #3495 +/- ##
==========================================
- Coverage 91.77% 91.76% -0.01%
==========================================
Files 383 383
Lines 8400 8405 +5
Branches 208 219 +11
==========================================
+ Hits 7709 7713 +4
- Misses 691 692 +1 |
|
|
||
| trait VarianceConversions extends VarianceConversionsLowPriority { | ||
| implicit def autoWidenBifunctor[F[_, _]: Bifunctor, A, B >: A, C, D >: C](fac: F[A, C]): F[B, D] = | ||
| Bifunctor[F].leftWiden(Bifunctor[F].rightFunctor.widen(fac)) | ||
|
|
||
| implicit def autoWidenProfunctor[F[_, _]: Profunctor, A, B <: A, C, D >: C](fac: F[A, C]): F[B, D] = |
There was a problem hiding this comment.
i think the name of this method should reflect the fact that it does not only widen, but narrow, too.
There was a problem hiding this comment.
The idea as I understood is that we get the behaviour F[A, C] <: F[B, D] given constraints A >: B and C <: D, the opposite however won't hold with these them.
The same is true for other implicits here, if the return type is A then we can return any instance of A or it's subtypes
There was a problem hiding this comment.
I guess the suggestion is just to reflect in the name that this also narrows the left (the A). Something on the line of autoConvertVarianceProfunctor or autoNarrowLeftWidenRightProfunctor. Any other suggestion is good. Also considering that this is and implicit, it will probably never be used directly.
| def inferred[F[_, _]: Profunctor](fi: F[Int, Int]): F[Either[Foo, Foo], Either[Foo, Foo]] = shouldInfer[F](fi) | ||
| } | ||
|
|
||
| test("Auto-variance should left widen a profunctor automatically") { |
There was a problem hiding this comment.
looks like I botched up left and right
| fi.bimap(i => if (true) Left(Bar(i)) else Right(Baz), identity) | ||
|
|
||
| def inferred[F[_, _]: Bifunctor](fi: F[Int, Int]): F[Either[Foo, Foo], Int] = shouldInfer[F](fi) | ||
| } | ||
|
|
||
| test("Auto-variance should widen a profunctor automatically") { |
There was a problem hiding this comment.
I agree. If we don't want to be specific about the side we could also stay general like
| test("Auto-variance should widen a profunctor automatically") { | |
| test("Auto-variance should convert a profunctor's variance automatically") { |
resolves #3452