Relax a constraint for Alternative's unite and separate methods from Monad to FlatMap

[satorg]

The initial discussion is here.

Rationale

• The unite and separate methods don't need a Monad actually to do their job, because they only use flatMap in addition to methods from Alternative. Therefore the FlatMap[F] constraint is just enough there.
• Moreover, both Alternative and Monad implement Applicative. That means the Applicative methods can come by two different paths which creates sort of a loophole for ambiguity:

  implicit val fooMonad: Monad[Foo] = ??? // assume Monad[Foo] is different from Alternative[Foo]
  Alternative[Foo].unite[Bar, Int](Foo(Bar(123)) // which `pure` should it use – one from `Alternative` or `Monad` ? 

Discussion

In fact, if it is okay to constraint the unite and separate methods with a Monad, then the Alternative typeclass is absolutely redundant in such a case – we can simply put these two methods directly into MonoidK.

Another approach (perhaps even better one) could be a new typeclass (let's name it Monadditive for the sake of this example) which would inherit to Monad and MonoidK (via Alternative) instead of just Applicative and MonoidK (which the current Alternative inherits to). In this case the definition of those two methods – unite and separate – could be simplified to the following:

trait Monadditive[F[_]] extends Alternative[F] with Monad[F] { // implies `MonoidK` as well
  def unite[G[_], A](fga: F[G[A]])(implicit G: Foldable[G]): F[A] = ??? // implementation remains the same
  def separate[G[_, _], A, B](fgab: F[G[A, B]])(implicit G: Bifoldable[G]): (F[A], F[B]) = ??? // implementation remains the same
}

In other words, an additional constraint for F[_] is not necessary anymore.

Therefore such a new typeclass would allow to reduce number of additional constraints passed to those methods.
And looks like it would make a lot of sense anyway – most of the containers which Cats supports implement corresponding Monad instances, not only Applicative.

[satorg]

Converting the PR to Draft since MiMa is not happy with it:

[error] (coreJVM / mimaReportBinaryIssues) Failed binary compatibility check against org.typelevel:cats-core_2.13:2.6.1! Found 1 potential problems

Let me investigate the issue first. If it is not worth the effort to fix it, I'll close the PR.

[rossabaker]

If you keep the old one and make it private[cats], you'll fix that MiMa complaint. Then you'll be fighting ambiguous overloads at call sites within cats, and cats-rooted projects like cats-effect. You can fix the ambiguous overload by putting the old one in a bincompat parent trait, but then a different MiMa error pops up. We've done this successfully on instances, but I don't remember a precedent on typeclasses.

There's also syntax to wrestle with.

[satorg]

@rossabaker thank you for the advice. As I can see now it won't be an easy lift. Seems I managed to calm MiMa down at this point, although the solution I came up with looks quite ugly to me. As you can see in the last commit, I added a new AlternativeBinCompat0 trait for that. Initially I tried to move the old unite method to this trait, but it didn't make MiMa happy unfortunately. It simply changed the way it complaints to

[error] Cats core: Failed binary compatibility check against org.typelevel:cats-core_2.13:2.6.1! Found 1 potential problems
[error]  * in current version, classes mixing cats.Alternative need be recompiled to wire to the new static mixin forwarder method all super calls to method unite(java.lang.Object,cats.Monad,cats.Foldable)java.lang.Object
[error]    filter with: ProblemFilters.exclude[NewMixinForwarderProblem]("cats.Alternative.unite")

So I decided to try the opposite: place the new unite into AlternativeBinCompat0 while leaving the old one in place (but restricting its visibility via private[cats]. Currently all checks pass, but I haven't got started with the syntax yet.

[satorg]

Seems I managed to get through all the thorns with this task. The only caveat: Simulacrum Scalafix rules don't work correctly with all these binary compatibility tricks in Alternative I had to apply. But that Simulacrum generated syntax is not currently in use for Alternative anyway.

[satorg]

The original implementation is replaced with this better optimized one which utilizes Foldable.foldMapK (instead of the former's foldLeft).

[satorg]

cc @rossabaker, in case if you're curious in how I've fought it :)

[satorg]

Hmm.. Seems the build failure is not related to the PR:

[error] org.scalajs.ir.IRVersionNotSupportedException: Failed to deserialize a file compiled with Scala.js 1.7 (supported up to: 1.5): /home/runner/.cache/coursier/v1/https/repo1.maven.org/maven2/org/scala-js/scalajs-library_2.13/1.7.1/scalajs-library_2.13-1.7.1.jar:/scala/volatile.sjsir
[error] You may need to upgrade the Scala.js sbt plugin to version 1.7 or later.
[error] 	at org.scalajs.sbtplugin.ScalaJSPluginInternal$.enhanceIRVersionNotSupportedException(ScalaJSPluginInternal.scala:83)
[error] 	at org.scalajs.sbtplugin.ScalaJSPluginInternal$.$anonfun$scalaJSStageSettings$19(ScalaJSPluginInternal.scala:227)

I wonder – is the main branch broken (how come?)

[rossabaker]

Yes, that scala.js issue came up on another branch. Pretty sure it's a result of #4018.

[rossabaker]

#4048 should fix the build.

[rossabaker]

Nice work! I've used package private to hide overloads, but can't recall ever using a protected. It seems to work. Have you tried calling it anywhere with something that has a FlatMap but no Monad? I'm mostly sure it will still work, but want to be 100% sure. (Maybe this encoding isn't as novel as I think.)

[satorg]

I've used package private to hide overloads, but can't recall ever using a protected. It seems to work.

Right, good catch. First I tried to make it private[cats] indeed. And it works in terms of binary compatibility too. But it makes testing of the new methods a way more sophisticated since apparently all the tests are also located inside the cats package and the compiler tries to route all the calls to unite and separate to the old methods instead of the new ones. Using protected instead of private[cats] basically solves the problem while preserving the binary compatibility.

Interesting observation: this trick with protected does not work for syntax – it breaks BC for Scala 2.12 (but still works for Scala 2.13 and Scala 3). I don't know exactly why.

[rossabaker]

Interesting observation: this trick with protected does not work for syntax – it breaks BC for Scala 2.12

Oh, so this is still going to fail MiMa even after #4048 is merged? 🤔

[satorg]

Oh, so this is still going to fail MiMa even after #4048 is merged? 🤔

No, I don't think so. I mean, all the checks had been succeeding before #4018. But later I rebased this PR onto main to test it against the latest version – and the checks began failing. I'm going to make the changes you requested and we will see what happens anyway :)

[satorg]

Have you tried calling it anywhere with something that has a FlatMap but no Monad?

Good catch, thanks. Seems I overlooked that it has to be explicitly tested. Should be good now.

[satorg]

@rossabaker thank you for keeping eye on this PR and helping me with polishing it, I appreciate it.

Yeah, as you might notice, I eventually decided to revert access for UniteOps and SeparateOps constructors back to public. Although I still believe it would be better to make them private[syntax], but the improvement would be so minutiae that it wouldn't be worth the compatibility risk it imposes.

[satorg]

Ironically, this PR had got conflicts with another my PR that have been merged recently.