Add Arrow Choice #2096

stephen-lazaro · 2017-12-12T08:02:15Z

Coming in right under the wire, this resolves issue #2067.
The laws are admittedly hideous, but seem morally correct. I believe I caught all the binary incompatibilities.
I also updated the symbols table with the new +++ for choose (whose name I feel somewhat undecided about).
If coverage complains, I'll add more doc tests for the ArrowChoice methods.

codecov-io · 2017-12-12T08:44:04Z

Codecov Report

Merging #2096 into master will increase coverage by 0.12%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2096      +/-   ##
==========================================
+ Coverage   94.66%   94.78%   +0.12%     
==========================================
  Files         322      325       +3     
  Lines        5451     5487      +36     
  Branches      215      207       -8     
==========================================
+ Hits         5160     5201      +41     
+ Misses        291      286       -5

Impacted Files	Coverage Δ
core/src/main/scala/cats/arrow/Choice.scala	`50% <ø> (ø)`	⬆️
.../scala/cats/laws/discipline/ArrowChoiceTests.scala	`100% <100%> (ø)`
core/src/main/scala/cats/instances/function.scala	`93.75% <100%> (ø)`	⬆️
...aws/src/main/scala/cats/laws/ArrowChoiceLaws.scala	`100% <100%> (ø)`
core/src/main/scala/cats/arrow/ArrowChoice.scala	`100% <100%> (ø)`
core/src/main/scala/cats/data/Kleisli.scala	`97.84% <100%> (+0.07%)`	⬆️
core/src/main/scala/cats/instances/tuple.scala	`100% <0%> (ø)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 275f723...8b5d497. Read the comment docs.

julienrf

Note that, like Arrow[A, ?] has an SemiGroupal instance (which has a product[A, B](fa: F[A], fb: F[B]): F[(A, B)]), ArrowChoice[A, ?] has a CoSemiGroupal instance (which would have a sum[A, B](fa: F[A], fb: F[B]): F[Either[A, B]] operation). I don’t know if there is an established name for CoSemiGroupal, nor if this sum method is useful, but I like the symmetry :)

julienrf · 2017-12-12T08:37:54Z

core/src/main/scala/cats/arrow/ArrowChoice.scala

+    choose(lift(identity[C]))(fab)
+
+  override def choice[A, B, C](f: F[A, C], g: F[B, C]): F[Either[A, B], C] =
+    rmap(choose(f)(g))(_.fold(identity, identity))


I noticed that choice doesn’t have ||| as a symbolic alias. This is not really related to this PR but that would be a good thing to have.

Good call, I can add.

kailuowang · 2017-12-12T14:56:50Z

core/src/main/scala/cats/data/Kleisli.scala

-
-  implicit val catsDataCommutativeArrowForKleisliId: CommutativeArrow[Kleisli[Id, ?, ?]] =
-    catsDataCommutativeArrowForKleisli[Id]
+  implicit def catsDataArrowForKleisli[F[_]](implicit M: Monad[F]): Arrow[Kleisli[F, ?, ?]] =


If I am not mistaken, since both Arrow and ArrowChoice instances have the same requirement (Monad[F]), one ArrowChoice instance should be fine. You can basically leave catsDataCommutativeArrowForKleisliId and catsDataCommutativeArrowForKleisli as is.

kailuowang · 2017-12-12T14:59:09Z

core/src/main/scala/cats/data/Kleisli.scala

@@ -237,6 +241,18 @@ private[data] trait KleisliCommutativeArrow[F[_]] extends CommutativeArrow[Kleis
  implicit def F: CommutativeMonad[F]
 }

+private[data] trait KleisliArrowChoice[F[_]] extends ArrowChoice[Kleisli[F, ?, ?]] with KleisliArrow[F] {


We can fold KleisliArrow into KleisliArrowChoice right? KleisliCommutativeArrow can extend KleisliArrowChoice .

kailuowang · 2017-12-12T14:59:55Z

core/src/main/scala/cats/data/Kleisli.scala

@@ -161,6 +159,12 @@ private[data] sealed abstract class KleisliInstances1 extends KleisliInstances2
  implicit def catsDataMonadForKleisli[F[_], A](implicit M: Monad[F]): Monad[Kleisli[F, A, ?]] =
    new KleisliMonad[F, A] { def F: Monad[F] = M }

+  implicit def catsDataCommutativeArrowForKleisli[F[_]](implicit M: CommutativeMonad[F]): CommutativeArrow[Kleisli[F, ?, ?]] =


These can return CommutativeArrow with ArrowChoice, if we let KleisliCommutativeArrow extend KleisliArrowChoice as mentioned below

Sounds good, will do.

kailuowang · 2017-12-12T15:25:27Z

laws/src/main/scala/cats/laws/ArrowChoiceLaws.scala

+ */
+trait ArrowChoiceLaws[F[_, _]] extends ArrowLaws[F] with ChoiceLaws[F] {
+  implicit override def F: ArrowChoice[F]
+  implicit def Function: ArrowChoice[Function1]


Are these laws based on the ones defined in Control-ArrowChoice?

Hmm, they are meant to but interestingly the ones in the source and the ones in the Haskell doc do not quite look the same, I'll give them another pass tonight to make sure I transcribed the correct set.

Yeah, I totally misread them, somehow. How embarrassing! Sorry about that. Fixing.

LukaJCB

This looks great!

kailuowang · 2017-12-13T16:12:37Z

core/src/main/scala/cats/instances/function.scala

+  implicit val catsStdInstancesForFunction1: ArrowChoice[Function1] with CommutativeArrow[Function1] =
+    new ArrowChoice[Function1] with CommutativeArrow[Function1] {
+      def choose[A, B, C, D](f: A => C)(g: B => D): Either[A, B] => Either[C, D] =
+        _.fold(


using pattern match rather than fold has performance gain. see #1951

Yup, can do. I tend to forget that end of things.

kailuowang · 2017-12-13T16:26:06Z

core/src/main/scala/cats/arrow/ArrowChoice.scala

+    choose(fab)(lift(identity[C]))
+
+  def right[A, B, C](fab: F[A, B]): F[Either[C, A], Either[C, B]] =
+    choose(lift(identity[C]))(fab)


This is never tested by law as I noticed from the law set defined in Haskell. Thus it doesn't have any test coverage.
Shall we add a law for this something like

def leftRightConsistent[A, B, C](f: A => B): IsEq[F[Either[C, A], Either[C, B]]] = F.right[A, B, C](F.lift[A, B](f)) <-> F.left[A, B, C](F.lift[A, B](f)).dimap(_.swap, _.swap)

I actually added symmetric versions for all the laws last night for that reason, but it takes the method well over 50 lines. Something like that sounds like a more reasonable way to get similar effect. I'll try it out today, it certainly should work.

kailuowang

Thanks so much @stephen-lazaro! Awesome contribution!

stephen-lazaro · 2017-12-13T21:40:08Z

Thanks @kailuowang!

LukaJCB

Thanks again!

stephen-lazaro added 3 commits December 11, 2017 22:18

ArrowChoice, laws, function instance

a8de85b

Kleisli instances, mima exceptions, tests

f315767

Update symbols table

7369b6f

julienrf reviewed Dec 12, 2017

View reviewed changes

kailuowang reviewed Dec 12, 2017

View reviewed changes

kailuowang added the enhancement label Dec 12, 2017

kailuowang added this to the 1.0.0 milestone Dec 12, 2017

stephen-lazaro added 4 commits December 12, 2017 14:37

Consolidate instances, add |||

791d9d1

Fix laws

93574b3

Clean up laws code

191c68c

Merge branch 'master' into 2067_add_arrow_choice

48d9458

LukaJCB previously approved these changes Dec 13, 2017

View reviewed changes

kailuowang reviewed Dec 13, 2017

View reviewed changes

Consistency law, match over fold

8b5d497

stephen-lazaro dismissed LukaJCB’s stale review via 8b5d497 December 13, 2017 19:23

kailuowang approved these changes Dec 13, 2017

View reviewed changes

LukaJCB approved these changes Dec 13, 2017

View reviewed changes

LukaJCB merged commit fca2014 into typelevel:master Dec 13, 2017

stephen-lazaro mentioned this pull request Dec 15, 2017

Add ArrowChoice #2067

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Arrow Choice #2096

Add Arrow Choice #2096

stephen-lazaro commented Dec 12, 2017

codecov-io commented Dec 12, 2017 •

edited

Loading

julienrf left a comment

julienrf Dec 12, 2017

stephen-lazaro Dec 12, 2017

kailuowang Dec 12, 2017

kailuowang Dec 12, 2017

kailuowang Dec 12, 2017 •

edited

Loading

stephen-lazaro Dec 12, 2017

kailuowang Dec 12, 2017

stephen-lazaro Dec 12, 2017

stephen-lazaro Dec 13, 2017

LukaJCB left a comment

kailuowang Dec 13, 2017 •

edited

Loading

stephen-lazaro Dec 13, 2017

kailuowang Dec 13, 2017 •

edited

Loading

stephen-lazaro Dec 13, 2017

kailuowang left a comment

stephen-lazaro commented Dec 13, 2017

LukaJCB left a comment

Add Arrow Choice #2096

Add Arrow Choice #2096

Conversation

stephen-lazaro commented Dec 12, 2017

codecov-io commented Dec 12, 2017 • edited Loading

Codecov Report

julienrf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kailuowang Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukaJCB left a comment

Choose a reason for hiding this comment

kailuowang Dec 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kailuowang Dec 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kailuowang left a comment

Choose a reason for hiding this comment

stephen-lazaro commented Dec 13, 2017

LukaJCB left a comment

Choose a reason for hiding this comment

codecov-io commented Dec 12, 2017 •

edited

Loading

kailuowang Dec 12, 2017 •

edited

Loading

kailuowang Dec 13, 2017 •

edited

Loading

kailuowang Dec 13, 2017 •

edited

Loading