Optimize Apply methods in FlatMap

[kailuowang]

I noticed that these implementations can be slightly improved. Given their common usages I think it would worth it.
related issue typelevel/cats-effect#401

[LukaJCB]

👍

[codecov-io]

Codecov Report

Merging #2597 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2597      +/-   ##
==========================================
+ Coverage   95.16%   95.16%   +<.01%     
==========================================
  Files         361      361              
  Lines        6634     6638       +4     
  Branches      294      295       +1     
==========================================
+ Hits         6313     6317       +4     
  Misses        321      321

Impacted Files	Coverage Δ
core/src/main/scala/cats/FlatMap.scala	86.66% <100%> (+4.84%)	⬆️

---

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 1aac375...0a9f9d9. Read the comment docs.

[ceedubs]

I think that this is probably a good change to make, but I'm wondering if it should go into a new minor version as opposed to a patch. My concern is that someone might have an IO/Future-like type where ap runs effects in parallel but a FlatMap instance is also defined. While we have laws that say that ap and flatMap should be consistent, our equality is only based on result values and not rumtime execution characteristics, so it's debatable whether or not their instance broke laws. So I wouldn't want to tank someone's performance without sufficient warning.

In fact I wonder whether this might apply to our instances for standard library Future?

[kailuowang]

@ceedubs this is probably not a breaking change because currently FlatMap already overrides product with implementation using flatMap. And product is called by these methods. So if a FlatMap instance is used, it's already using flatMap v.s. ap, e.g. the current implementation of productL and productR in FlatMap is essentially:

 map(flatMap(fa)(a => map(fb)(b => (a, b))))((a, _) => a)
 map(flatMap(fa)(a => map(fb)(b => (a, b))))((_, b) => b)

You can see wasteful maps.

So I don't blieve this change will change the effect.
I am not 100% sure if product being overriden in FlatMap is more correct at the first place, but my thinking is that user should avoid getting tied to the concrete effect type F[_], and if they keep effect type abstract and use a context bound F[_]: FlatMap then they should expect sequential execution anyway.

[ceedubs]

Ah, okay thanks for the explanation @kailuowang. This sounds good to me (but maybe we should let this hang around just a bit longer in case anyone else has concerns).

[ceedubs]

Coming back to this, the lack of symmetry between productR and productL feels a little off to me. I think that the override of map2 makes sense. But maybe productL and productR should both just delegate to map2?

[kailuowang]

Unlike productL, productR using map2 will incur one extra map. That's where the asymmetry comes from.

[ceedubs]

Oh right. Sorry that I've required so much explanation on this one 😬

LGTM. @mpilquist does this PR look good to you?

[mpilquist]

Yep looks good to me