Cats-1567 Replace Split with Commutative Arrow. Introduces Commutative Monad. #1719

diesalbla · 2017-06-04T23:02:51Z

This PR carries the work to solve issue #1567 following the direction agreed in this comment.

We remove the Split type-class, which was only declaring a split method. This method is now integrated into Arrow, with a default implementation. We also remove the (flawed) SplitLaws, its SplitTests.
We add a new method-less type-class CommutativeArrow, which restricts the Arrow type-class with an extra law, the commutativity of the split operation. We also create its laws and tests. The use of type-classes with laws but without operations was suggested in Typeclasses that ONLY define laws. #334.
We remove the instances of Split for Kleisli and CoKleisli; the special implementation for split is now integrated into their respective Arrow instances.

We found that this would unable some tests of the arrow-commutativity laws for certain instances of Arrow[Kleisli[F,?,?]], such as F=Id and F=Option, in which the law holds. To keep these tests:

We introduce a sub-type-class of Monad, CommutativeMonad, in which the flatMap operation is commutative. We change the sets of instances of Monad[Option] and Monad[Id] to CommutativeMonad.
We create some instances of CommutativeArrow[Kleisli[F,?,?]]`` for those F[_]that areCommutativeMonad`.

I can mention a couple of design choices that may be open to discussion:

We could as well create a type-class of commutative Comonads, and their use for Cokleisli, but I have not found any existing tests for those so there seems to be no urgent need at present.
We may consider if commutativity should be available not just for Arrow and Monad, but perhaps for Compose and Apply as well.

PS (Edited): The instances of CommutativeMonad for Option and Id are the ones needed to keep existing tests. There may be other types F[_] for which a Monad[F] is provided in cats that is also a CommutativeMonad[F].

diesalbla · 2017-06-04T23:07:06Z

@edmundnoble @fthomas @kailuowang @peterneyens

codecov-io · 2017-06-05T11:33:23Z

Codecov Report

Merging #1719 into master will decrease coverage by 0.19%.
The diff coverage is 89.65%.

@@            Coverage Diff            @@
##           master    #1719     +/-   ##
=========================================
- Coverage   94.02%   93.82%   -0.2%     
=========================================
  Files         256      249      -7     
  Lines        4201     4099    -102     
  Branches       84      153     +69     
=========================================
- Hits         3950     3846    -104     
- Misses        251      253      +2

Impacted Files	Coverage Δ
laws/src/main/scala/cats/laws/ArrowLaws.scala	`100% <ø> (ø)`	⬆️
...c/main/scala/cats/laws/discipline/ArrowTests.scala	`100% <ø> (ø)`	⬆️
...e/src/main/scala/cats/arrow/CommutativeArrow.scala	`0% <0%> (ø)`
core/src/main/scala/cats/CommutativeMonad.scala	`0% <0%> (ø)`
...a/cats/laws/discipline/CommutativeArrowTests.scala	`100% <100%> (ø)`
...rc/main/scala/cats/laws/CommutativeArrowLaws.scala	`100% <100%> (ø)`
core/src/main/scala/cats/data/Kleisli.scala	`91.3% <100%> (-1.35%)`	⬇️
...a/cats/laws/discipline/CommutativeMonadTests.scala	`100% <100%> (ø)`
...rc/main/scala/cats/laws/CommutativeMonadLaws.scala	`100% <100%> (ø)`
core/src/main/scala/cats/package.scala	`100% <100%> (ø)`	⬆️
... and 41 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 bbc97b8...b8f0658. Read the comment docs.

kailuowang · 2017-06-05T13:44:47Z

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

+   *  `f` and `g` in the context of F. This means that `f *** g` may not be equivalent to `g *** f`.
+   */
+  @simulacrum.op("***", alias = true)
+  def split[A, B, C, D](f: F[A, B], g: F[C, D]): F[(A, C), (B, D)] =
    andThen(first(f), second(g))


quick note, codecov somehow mistakenly thinks this line is uncovered.

kailuowang · 2017-06-05T13:58:55Z

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

-  override def split[A, B, C, D](f: Cokleisli[F, A, B], g: Cokleisli[F, C, D]): Cokleisli[F, (A, C), (B, D)] =
-    super[CokleisliSplit].split(f, g)
+  override def split[A1, B1, A2, B2](f: Cokleisli[F, A1, B1], g: Cokleisli[F, A2, B2]): Cokleisli[F, (A1, A2), (B1, B2)] =
+    Cokleisli(fac => f.run(F.map(fac)(_._1)) -> g.run(F.map(fac)(_._2)))


Since we removed the split from the laws, this line is not tested at all. It looks like parametricity guarantees the right implementation, but to be consistent, shall we still have some test coverage (I am fine with a quick doc test)?

The type does imply that in each output value (b,d): (B,D), to obtain the B (resp. D) you need to run f (resp g) on the input component of type A (resp. C). However, that still leaves space to at least two possible implementations: one in which f runs before g, and one in which g runs before f. These implementations are not equivalent if the effects interfere, for instance, if they each one echoes and prints its input into stdout. Commutativity is what tells you that the effects of f and g do not interfere, and that is the only useful law I can see for split.

We can test this line if we write the dual of the commutative monad, and use it to derive instances as done for Kleisli.

Note: The conventional implementation (AFAIK from Haskell) is to run f first, which I guess is because f refers to the first components of input and output.

Right. I was saying that the split method is not directly tested in the commutativity law, so it's not tested for both Kleisli and Cokleisli. I agree that it's probably fine since the commutativity law is the only relevant law to it and that together with parametricity probably are enough to ensure its correctness (although, as you pointed out, in the noncommutative case the correctness is a bit arbitrary).
I don't feel strong about it but adding a doctest (or just a regular unit test) to provide some codecov coverage wouldn't hurt right? I mean right now you can replace the implementation of these two split impl with ???s without breaking any test, which is a bit uneasy to me.

kailuowang · 2017-06-05T14:16:08Z

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

-}
-
-private trait KleisliSplit[F[_]] extends Split[Kleisli[F, ?, ?]] with KleisliCompose[F] {
-  implicit def F: FlatMap[F]

  override def split[A, B, C, D](f: Kleisli[F, A, B], g: Kleisli[F, C, D]): Kleisli[F, (A, C), (B, D)] =
    Kleisli{ case (a, c) => F.flatMap(f.run(a))(b => F.map(g.run(c))(d => (b, d))) }


same situation here as in Cokleisli.
An alternative approach we can take is to add split to the arrowLaws that simply tests whatever override is consistent with andThen(first(f), second(g)). Unfortunately, we don't have a consistent strategy over how to tests these overrides in instances yet. So for now, I'd vote for just adding a doctest.

edmundnoble · 2017-06-11T20:06:35Z

It may be out of scope for this PR, but I'd also like to see CommutativeCartesian, CommutativeApply and CommutativeApplicative.

diesalbla · 2017-06-11T21:41:42Z

@edmundnoble I am afraid that the issue is a bit out of scope for this PR, whose original intent was just to fix a problem in the arrow package. It can easily be done in a separate PR, concurrently from this one. I suppose that the laws for such type-classes would be as follows:

Commutative Cartesian: for any a: F[A] and b : F[B] , it holds that
product(a,b) <-> product(b,a).map(swap).
Commutative Apply: for any a: F[A] and f: F[A => B],

def flip$(x: A)(f: A => B): B = f(x)  // Haskell `flip ($)`
ap(f,a) <-> ap( a.map(flip$), f)

Commutative Applicative: no special laws, since Applicative only gives the Unit. Just subtyping and inheritance.

edmundnoble · 2017-06-11T23:32:02Z

@diesalbla Agreed, though I think CommutativeApply has no extra laws because that one is derivable from CommutativeCartesian's.

kailuowang · 2017-06-21T14:13:09Z

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

-    def pure[B](x: B): Cokleisli[F, A, B] =
-      Cokleisli.pure(x)
+  implicit def catsDataCommutativeArrowForCokleisli[F[_]](implicit ev: CommutativeComonad[F]): CommutativeArrow[Cokleisli[F, ?, ?]] =
+    new CokleisliCommutativeArrow[F] { def F: CommutativeComonad[F] = ev }


Looks like that the build wasn't happy because you forgot to give this extra help scalac needed.

implicit val catsDataCommutativeArrowForCokleisliId: CommutativeArrow[Cokleisli[Id, ?, ?]] = catsDataCommutativeArrowForCokleisli[Id]

kailuowang · 2017-06-27T14:39:11Z

@diesalbla just curious if you have time to finish this one up. If not I would be more than happy to help. I think it's very close, just need to fix the build according to the comment I made above.

This commit removes from the `core` module the `Split` typeclass. This includes the following changes: * The `Arrow` typeclass is no longer a subtype of `Split`. * The `split` operation, previously in the `Split` typeclass, is now integrated in the `Arrow` typeclass, with a default implementation. * Following notation from the `Control.Arrow` library for Haskell, we use the operator `***` as an alias for `split`. * `SplitSyntax` is replaced with `ArrowSyntax`. * We remove the `SplitLaws` and the `SplitTests`. In consequence, ArrowLaws does not inherit from SplitLaws anymore. * We modify the instances for `Kleisli`. We remove the trait `KleisliSpli`, and we replace the _factory_ method that was generating it to generate a `KleisliCompose` instead. * We remove the tests on the `SplitLaws` for Kleisli and CoKleisli

We add a type-class of commutative arrows, which are those in which the `split` operation is commutative (can pair in any order).

We introduce a Commutative Monad Type-class, a subclass of Monad in which the flatMap of independent operations is commutative.

We introduce some instances of CommutativeArrow for Kleisli, which are based on CommutativeMonad.

We introduce a new type class, CommutativeFlatMap, to load the commutativity law one level up.

We introduce the duals of Commutative Comonads and CoflatMap, as the duals of commutative Flatmaps and Monads. This is done to generate and test CommutativeArrow instances for the Cokleisli datatype.

We complete the tests for the CommutativeArrow instance of Cokleisli. To use it, we mark the Monad instance of `NonEmptyList` as a Commutative Monad.

kailuowang · 2017-06-29T15:43:49Z

looks like one of the new tests failed

 NonEmptyList[Int].commutative comonad.coflatmap commutativity *** FAILED *** (32 milliseconds)�
 GeneratorDrivenPropertyCheckFailedException was thrown during property evaluation.�
  (Discipline.scala:14)�
   Falsified after 3 successful property evaluations.�
   Location: (Discipline.scala:14)�
   Occurred when passed generated values (�
     arg0 = NonEmptyList(-1),�
     arg1 = NonEmptyList(890208370, -2147483648),�
     arg2 = org.scalacheck.GenArities$$Lambda$5347/112471456@45377d5e�
   )�
   Label of failing property:�
     (NonEmptyList(NonEmptyList(1, 1)) ?== NonEmptyList(NonEmptyList(1), NonEmptyList(1))) failed�

kailuowang · 2017-07-06T15:22:13Z

It looks like that the current NonEmptyList.coflatmap isn't commutative.

kailuowang · 2017-07-06T17:54:04Z

@diesalbla I've created a PR on your branch which should fix the conflicts and build error.
https://github.com/diesalbla/cats/pull/1

kailuowang · 2017-07-13T14:22:46Z

continued with #1766

* Cats-1567 Replace Split with Commutative Arrow This commit removes from the `core` module the `Split` typeclass. This includes the following changes: * The `Arrow` typeclass is no longer a subtype of `Split`. * The `split` operation, previously in the `Split` typeclass, is now integrated in the `Arrow` typeclass, with a default implementation. * Following notation from the `Control.Arrow` library for Haskell, we use the operator `***` as an alias for `split`. * `SplitSyntax` is replaced with `ArrowSyntax`. * We remove the `SplitLaws` and the `SplitTests`. In consequence, ArrowLaws does not inherit from SplitLaws anymore. * We modify the instances for `Kleisli`. We remove the trait `KleisliSpli`, and we replace the _factory_ method that was generating it to generate a `KleisliCompose` instead. * We remove the tests on the `SplitLaws` for Kleisli and CoKleisli * Cats-1567 Add Commutative Arrows We add a type-class of commutative arrows, which are those in which the `split` operation is commutative (can pair in any order). * Cats-1567 Introduce Commutative Monad We introduce a Commutative Monad Type-class, a subclass of Monad in which the flatMap of independent operations is commutative. * Cats-1567 Kleisli Instances We introduce some instances of CommutativeArrow for Kleisli, which are based on CommutativeMonad. * Cats-1567 Split CommutativeMonad into Commutative FlatMap We introduce a new type class, CommutativeFlatMap, to load the commutativity law one level up. * Cats-1567 Commutative Comonad - CoflatMap We introduce the duals of Commutative Comonads and CoflatMap, as the duals of commutative Flatmaps and Monads. This is done to generate and test CommutativeArrow instances for the Cokleisli datatype. * 1567 Complete tests for Cokleisli We complete the tests for the CommutativeArrow instance of Cokleisli. To use it, we mark the Monad instance of `NonEmptyList` as a Commutative Monad. * NonEmptyList comonad is not commutative * fix unmoored statement * added CommutativeMonad instances for Kleisli and WriterT * fix import error * made function1 commutativeArrow * removed CommutativeComonad and CommutativeCoflatmap * added back arrow tests

diesalbla · 2017-07-19T20:13:34Z

This PR were continued in PR #1766, which has been merged. I am closing this Pull Request.

kailuowang reviewed Jun 5, 2017

View reviewed changes

kailuowang added the Source Breaking label Jun 5, 2017

kailuowang added this to the 1.0.0-MF milestone Jun 5, 2017

kailuowang mentioned this pull request Jun 5, 2017

Prepare for the 1.0.0-MF release #1702

Merged

26 tasks

kailuowang reviewed Jun 21, 2017

View reviewed changes

diesalbla added 7 commits June 29, 2017 00:48

Cats-1567 Add Commutative Arrows

e59a0e0

We add a type-class of commutative arrows, which are those in which the `split` operation is commutative (can pair in any order).

Cats-1567 Introduce Commutative Monad

5380fa3

We introduce a Commutative Monad Type-class, a subclass of Monad in which the flatMap of independent operations is commutative.

Cats-1567 Kleisli Instances

0fd9a0f

We introduce some instances of CommutativeArrow for Kleisli, which are based on CommutativeMonad.

Cats-1567 Split CommutativeMonad into Commutative FlatMap

670955a

We introduce a new type class, CommutativeFlatMap, to load the commutativity law one level up.

Cats-1567 Commutative Comonad - CoflatMap

56efbec

We introduce the duals of Commutative Comonads and CoflatMap, as the duals of commutative Flatmaps and Monads. This is done to generate and test CommutativeArrow instances for the Cokleisli datatype.

1567 Complete tests for Cokleisli

b8f0658

We complete the tests for the CommutativeArrow instance of Cokleisli. To use it, we mark the Monad instance of `NonEmptyList` as a Commutative Monad.

kailuowang mentioned this pull request Jul 13, 2017

Replace Split with Commutative Arrow. Continuing #1719 #1766

Merged

diesalbla closed this Jul 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cats-1567 Replace Split with Commutative Arrow. Introduces Commutative Monad. #1719

Cats-1567 Replace Split with Commutative Arrow. Introduces Commutative Monad. #1719

diesalbla commented Jun 4, 2017 •

edited

Loading

diesalbla commented Jun 4, 2017

codecov-io commented Jun 5, 2017 •

edited

Loading

kailuowang Jun 5, 2017

kailuowang Jun 5, 2017 •

edited

Loading

diesalbla Jun 5, 2017 •

edited

Loading

kailuowang Jun 6, 2017 •

edited

Loading

kailuowang Jun 5, 2017

edmundnoble commented Jun 11, 2017

diesalbla commented Jun 11, 2017 •

edited

Loading

edmundnoble commented Jun 11, 2017

kailuowang Jun 21, 2017

kailuowang commented Jun 27, 2017

kailuowang commented Jun 29, 2017

kailuowang commented Jul 6, 2017

kailuowang commented Jul 6, 2017

kailuowang commented Jul 13, 2017

diesalbla commented Jul 19, 2017

Cats-1567 Replace Split with Commutative Arrow. Introduces Commutative Monad. #1719

Cats-1567 Replace Split with Commutative Arrow. Introduces Commutative Monad. #1719

Conversation

diesalbla commented Jun 4, 2017 • edited Loading

diesalbla commented Jun 4, 2017

codecov-io commented Jun 5, 2017 • edited Loading

Codecov Report

kailuowang Jun 5, 2017

Choose a reason for hiding this comment

kailuowang Jun 5, 2017 • edited Loading

Choose a reason for hiding this comment

diesalbla Jun 5, 2017 • edited Loading

Choose a reason for hiding this comment

kailuowang Jun 6, 2017 • edited Loading

Choose a reason for hiding this comment

kailuowang Jun 5, 2017

Choose a reason for hiding this comment

edmundnoble commented Jun 11, 2017

diesalbla commented Jun 11, 2017 • edited Loading

edmundnoble commented Jun 11, 2017

kailuowang Jun 21, 2017

Choose a reason for hiding this comment

kailuowang commented Jun 27, 2017

kailuowang commented Jun 29, 2017

kailuowang commented Jul 6, 2017

kailuowang commented Jul 6, 2017

kailuowang commented Jul 13, 2017

diesalbla commented Jul 19, 2017

diesalbla commented Jun 4, 2017 •

edited

Loading

codecov-io commented Jun 5, 2017 •

edited

Loading

kailuowang Jun 5, 2017 •

edited

Loading

diesalbla Jun 5, 2017 •

edited

Loading

kailuowang Jun 6, 2017 •

edited

Loading

diesalbla commented Jun 11, 2017 •

edited

Loading