CoflatMap for Sync

[ChristopherDavenport]

Fixes #97
Extends CoFlatMap and uses methods directly as that was only way that was inserting methods. I'd be interested in a better approach.

[codecov-io]

Codecov Report

Merging #100 into master will increase coverage by 0.02%.
The diff coverage is 57.14%.

@@            Coverage Diff             @@
##           master     #100      +/-   ##
==========================================
+ Coverage   88.44%   88.47%   +0.02%     
==========================================
  Files          20       20              
  Lines         450      451       +1     
  Branches       40       41       +1     
==========================================
+ Hits          398      399       +1     
  Misses         52       52

[rossabaker]

This passes the tests, but I'm concerned that the reason for this cats-core concern being in cats-effect is "we thought of it 48 hours too late for it to be in cats-core."

An alternative proposal. This compiles and passes MiMa in cats-core:

object CoflatMap {
  implicit def catsCoflatMapFromApplicative[F[_]](implicit F: Applicative[F]): CoflatMap[F] =
    new CoflatMap[F] {
      def coflatMap[A, B](fa: F[A])(f: F[A] => B): F[B] = F.pure(f(fa))
      def map[A, B](fa: F[A])(f: A => B): F[B] = F.map(fa)(f)
    }
}

And then:

trait Foo[_]
object Foo {
  implicit val catsApplicativeForIO: Applicative[Foo] = null
}

object TestResolution {
  implicitly[CoflatMap[Foo]]
}

That would miss cats-1.0, but wouldn't have to wait for cats-2.0. I don't know what ambiguous instance havoc this might unleash, but it works for me so far.

[LukaJCB]

I'm not a hundred percent sure this is a cats-core concern. I view this the same way as I do Applicative.monoid or MonoidK.algebra. instances that can be derived, but the derived instance might not be exactly what you want, so the solution is to make them non implicit and make the caller decide for themselves.

I can't say for certain if Sync should extend CoflatMap as I'm not super familiar with the exact in and outs of the hierarchy, but for the use case @ChristopherDavenport described it makes sense. Would like to hear input from others as well :)

[alexandru]

As long as Sync instances don't violate any CoflatMap laws, I don't see a problem.

[rossabaker]

I say it's a cats-core concern because I think the only reason we're talking about this in cats-effect was because it was too late to do anything deeper in cats-core. There's no extended property of Sync that makes the relationship appropriate here and not all the way down to cats-core typeclasses.

I think @LukaJCB hits on the key question: do we want this derivation to be implicit or explicit? I think the MonoidK.algebra is a good analogy: something that could be derived from just the one type class without any extra evidence (unlike Applicative.monoid).

If it should be explicit, then what we have now in cats-core is good. If it should be implicit, then I'd like to first rule out putting it on the CoflatMap companion. That approach has the advantage of making it work for Applicatives that aren't Syncs, but I'm worried about subtle issues in resolution.

[mpilquist]

This would also prevent a default CoflatMap[IO] that forks the IO action in extend -- not sure that's a good idea or not but picking the CoflatMap that's derived from Applicative seems to limit us from having more interesting/useful instances.

[rossabaker]

Prevents a default, forking CoflatMap[IO] because it would require an ExecutionContext and clash with ioEffect, which otherwise doesn't?

[mpilquist]

Yep

[ChristopherDavenport]

If it requires an execution context to do the above actions is it the same coflatMap?

[mpilquist]

The ExecutionContext would be captured on the CoflatMap instance. Not necessarily suggesting it's a good idea, just pointing out that there are often more useful implementations of CoflatMap than the one derived from Applicative.

[ChristopherDavenport]

My primary use case was based on Stream(1,2,3).map(i => Stream.emit(i)).joinUnbounded being able to transtion instead to Stream(1,2,3).coflatten.joinUnbounded However most other implementations don't follow the applicative version anyways(despite most of them implementing applicative), So as long as the concrete types like IO, and Stream were to implement a CoflatMap I think that would be useful. This would allow it to be called inside polymorphic code. F[_]: Sync = F.delay(???).coflatten where those concrete instances would lose that ability.

[rossabaker]

Existing CoflatMap instances that behave like Applicative.coflatMap:

• Function0

Existing CoflatMap instances that don't behave like Applicative.coflatMap:

• Eval
• Future
• List
• Option
• NonEmptyList
• NonEmptyVector
• OneAnd
• Queue
• Stream
• Try
• Tuple2
• Vector

Some of these differences are very subtle, but they're all there.

As for defining CoflatMap[IO], note that CoflatMap[Future] forks.

[ChristopherDavenport]

I think I found a reason not to use the applicative one in the least.

F[A] => (F[A] => A) => F[A], its pathological but F[A].coflatMap(_.unsafeRunSync) using Pure seems to be wrong, as we would need to codify what that edge behavior of coflatMap would allow/capture.

[ChristopherDavenport]

I think this should not delay a release I would like to work to get an implicit implemented for next week or if I can beat the bus. I am going to see how this can be worked in meaningfully in several places.

[rossabaker]

Precedent: the Try and Future instances both capture failures of the coflatMapped function instead of using their pures.