Cats-1567 Replace Split with Commutative Arrow. Introduces Commutative Monad. #1719
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
| * `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)) |
There was a problem hiding this comment.
quick note, codecov somehow mistakenly thinks this line is uncovered.
| 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))) |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| 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))) } |
There was a problem hiding this comment.
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.
|
It may be out of scope for this PR, but I'd also like to see |
|
@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
def flip$(x: A)(f: A => B): B = f(x) // Haskell `flip ($)`
ap(f,a) <-> ap( a.map(flip$), f)
|
|
@diesalbla Agreed, though I think |
| 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 } |
There was a problem hiding this comment.
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]
|
@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.
|
looks like one of the new tests failed |
|
It looks like that the current |
|
@diesalbla I've created a PR on your branch which should fix the conflicts and build error. |
|
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
|
This PR were continued in PR #1766, which has been merged. I am closing this Pull Request. |
This PR carries the work to solve issue #1567 following the direction agreed in this comment.
Splittype-class, which was only declaring asplitmethod. This method is now integrated intoArrow, with a default implementation. We also remove the (flawed)SplitLaws, itsSplitTests.CommutativeArrow, which restricts theArrowtype-class with an extra law, the commutativity of thesplitoperation. 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.SplitforKleisliandCoKleisli; the special implementation forsplitis now integrated into their respectiveArrowinstances.We found that this would unable some tests of the arrow-commutativity laws for certain instances of
Arrow[Kleisli[F,?,?]], such asF=IdandF=Option, in which the law holds. To keep these tests:Monad,CommutativeMonad, in which theflatMapoperation is commutative. We change the sets of instances ofMonad[Option]andMonad[Id]toCommutativeMonad.CommutativeArrow[Kleisli[F,?,?]]`` for thoseF[_]that areCommutativeMonad`.I can mention a couple of design choices that may be open to discussion:
Cokleisli, but I have not found any existing tests for those so there seems to be no urgent need at present.ArrowandMonad, but perhaps forComposeandApplyas well.PS (Edited): The instances of
CommutativeMonadforOptionandIdare the ones needed to keep existing tests. There may be other typesF[_]for which aMonad[F]is provided incatsthat is also aCommutativeMonad[F].