Cats: Kleisli ap method in Applicative has arguments reversed

[justinhj]

Most Applicative instances have an ap which implements function application where both the function and the parameter are in an effect type. Most instances take the function first and the parameter second, but the Kleisli (and therefore the Reader) instances have the order of arguments swapped.

Concretely:

@ Applicative[IntReader].pure(10).ap(Applicative[IntReader].pure((a: Int) => a + 1)).run(100)
res16: Id[Int] = 11

@ Applicative[Option].pure((a: Int) => a + 1).ap(Applicative[Option].pure(10))
res17: Option[Int] = Some(11)

Having spoken to @djspiewak on gitter about this, it's recommended to fix it. The order of parameters should be changed in the syntax enrichment. This should preserve binary compatibility...

"by adding an overload and setting the existing one to private[data]"

[justinhj]

Will take a stab at this.

[dmndpl]

I tried adding the following
def ap[B, C](fa: Kleisli[F, A, B])(f: Kleisli[F, A, B => C]): Kleisli[F, A, C] = fa.ap(f) in KleisliApply[F[_], A] and setting the previous ap to private[data] however the compiler is not happy.

double definition: override def ap[B, C](f: cats.data.Kleisli[F,A,B => C])(fa: cats.data.Kleisli[F,A,B]): cats.data.Kleisli[F,A,C] at line 620 and def ap[B, C](fa: cats.data.Kleisli[F,A,B])(f: cats.data.Kleisli[F,A,B => C]): cats.data.Kleisli[F,A,C] at line 623 have same type after erasure: (f: cats.data.Kleisli, fa: cats.data.Kleisli)cats.data.Kleisli

To my understanding, this was the proposed solution, is there an alternative maybe?

[djspiewak]

To my understanding, this was the proposed solution, is there an alternative maybe?

The definition in KleisliApply is actually correct. The definition in Kleisli itself is what's wrong. What we need to add is the following:

  def ap[C, D, AA <: A](f: Kleisli[F, AA, C])(implicit F: Apply[F], ev: B As (C => D)): Kleisli[F, AA, D] = ???

Or something like it.

[justinhj]

That's right, I've spent a bit of time trying to make this work but the types blow my mind a bit. The "As" syntax above looks perfect but it doesn't seem to compile. I've been trying to do something with asInstanceOf which is what the generic version of ap does in Apply.scala

[djspiewak]

The "As" syntax above looks perfect but it doesn't seem to compile.

It doesn't? It should. What error do you get?

[justinhj]

"Not found type As"
I'm looking at it in VS Code, so maybe bloop is having a problem with it that sbt won't. I'll try it there.

[justinhj]

Hmm yeah I get the same error in a REPL

@   def ap2[F[_], A, C, D, AA <: A](f: Kleisli[F, AA, C])(implicit F: Apply[F], ev: B As (C => D)): Kleisli[F, AA, D] = ???
cmd2.sc:1: not found: type As
def ap2[F[_], A, C, D, AA <: A](f: Kleisli[F, AA, C])(implicit F: Apply[F], ev: B As (C => D)): Kleisli[F, AA, D] = ???
                                                                                  ^
Compilation Failed

I haven't see that syntax before, is it regular Scala or am I maybe missing a plugin?

[djspiewak]

Ah. :-) import cats.evidence.As

[justinhj]

Working on a draft PR. I think I should probably not need the instanceOf below, but not sure how to leverage that "As" implicit to do so...

  def ap[C, D, AA <: A](fc: Kleisli[F, AA, C])(implicit F: Apply[F], ev: B As (C => D)): Kleisli[F, AA, D] = {
    Kleisli{
      (a: AA) => {
        **val what: F[C => D] = run(a).asInstanceOf[F[C => D]]**
        F.ap(what)(fc.run(a))
      } 

[dmndpl]

I managed to get the compiler to play nicely with the following:

  def ap[C, D, AA <: A](f: Kleisli[F, AA, C])(implicit F: Apply[F], ev: B As (C => D) ): Kleisli[F, AA, D] = Kleisli { a => 
    val fb: F[C => D] = F.map(run(a))(ev.coerce)
    val fc: F[C] = f.run(a)
    F.ap(fb)(fc)
  }

[djspiewak]

Looks about right to me!

[justinhj]

Thanks @dmndpl ! I have updated the PR with this change and also restored the old function as a private overload for backwards compatibility. Will run the tests before removing draft status

[justinhj]

Fixed in #3462