Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add Local[F, Span[F]] instance where F is Kleisli[F, Span[F], *] #713

Merged
merged 5 commits into from
Feb 8, 2023

Conversation

bpholt
Copy link
Member

@bpholt bpholt commented Jan 31, 2023

When fulfilling a effect-polymorphic function demanding an implicit Local[F, Span[F]] with [F[_], A] =>> Kleisli[F, Span[F], A], the implicit instances provided by cats-mtl don't quite fit. The code demands

Local[Kleisli[F, Span[F], *], Span[Kleisli[F, Span[F], *]]

but Local.baseLocalForKleisli returns

Local[Kleisli[F, Span[F], *], Span[F]]

(i.e., the E parameter is wrong), and Local.localForKleisli requires an implicit

Local[F, Span[Kleisli[F, Span[F], *]]

which we don't have.

AFAICT, it's safe to (effectively) imap the Local[Kleisli[F, Span[F], *], Span[F]] returned by Local.baseLocalForKleisli using Kleisli.liftK and Kleisli.applyK. Kleisli.liftK will turn Span[F] into Span[Kleisli[F, Span[F], *]], which, when run, will ignore any Span[F]s passed to each Kleisli[F, Span[F], *]. We take advantage of that by immediately mapKing it back to Span[F], applying a no-op span that we know will never actually be used.

(Alternatively, if there's a better way to do this, let me know and I'll change this PR to document that method instead.)

@mergify mergify bot added the mtl label Jan 31, 2023
@armanbilge
Copy link
Member

armanbilge commented Jan 31, 2023

Hmm, is the problem here?
https://github.com/tpolecat/natchez/blob/08d91de97f7ebd64712be36a30b3ee58d41015f1/modules/mtl/shared/src/main/scala/package.scala#L13-L17

I.e. should that be asking for a ev: Local[F, Span[G]] and G ~> F or something?

Edit: even better, the G ~> F can be implicitly fulfilled by MonadPartialOrder.
https://github.com/typelevel/cats-mtl/blob/384a4719bd154a5ad6aa4afad606ff863bdba147/core/src/main/scala/cats/mtl/MonadPartialOrder.scala#L30

We'll have to add a (low-priority?) instance for MonadPartialOrder[F, F] and then we can binary-compatibly replace this implicit with one that asks for Local[F, Span[G]] and MonadPartialOrder[G, F]. This will work for both Kleisli and IO.

@bpholt
Copy link
Member Author

bpholt commented Jan 31, 2023

Hmm, in principle I like that, but I'm not sure it solves my problem. If I'm understanding you correctly, I was able to implement

implicit def natchezMtlTraceViaLocal[F[_], G[_]](implicit ev: Local[F, Span[G]],
                                                 fk: MonadPartialOrder[G, F],
                                                 F: MonadCancelThrow[F],
                                                 G: MonadCancelThrow[G],
                                                ): Trace[F]

and then use it by changing the launch point of my program from

def foo[F[_] : MonadCancelThrow](implicit L: Local[F, Span[F]]): F[Unit]

to

def foo[F[_] : MonadCancelThrow, G[_] : MonadCancelThrow](implicit L: Local[F, Span[G]], 
                                                          fk: MonadPartialOrder[G, F]): F[Unit]

and then call it with one of

foo[Kleisli[IO, Span[IO], *], IO]
foo[IO, IO]

instead of one of

foo[Kleisli[IO, Span[IO], *]]
foo[IO]

which is obviously not as pleasant to use. Am I missing something?

@armanbilge
Copy link
Member

Thanks for trying that! Maybe we can use the partially-applied trick here? So that you only need to specify one of the type parameters.

@armanbilge
Copy link
Member

armanbilge commented Jan 31, 2023

Or a bespoke encoding.

trait LocalSpanAndMonadPartialOrderAndMonadCancelThrow[F[_]] {
  type G[_]
  def localSpan: Local[F, Span[G]]
  def monadPartialOrder: MonadPartialOrder[G, F]
  def monadCancelThrow: MonadCancelThrow[G]
}

object LocalSpanAndMonadPartialOrderAndMonadCancelThrow {
  type Aux[F[_], G0[_]] = LocalSpanAndMonadPartialOrderAndMonadCancelThrow[F] { type G = G0 }

  implicit def instance[F[_], G0[_]](
    implicit ls: Local[F, Span[G0]],
    mpo: MonadPartialOrder[G0, F]
    mct: MonadCancelThrow[G0]
  ): Aux[F, G0] = new LocalSpanAndMonadPartialOrderAndMonadCancelThrow[F] {
    type G = G0
    def localSpan: Local[F, Span[G]] = ls
    def monadPartialOrder: MonadPartialOrder[G, F] = mpo
    def monadCancelThrow: MonadCancelThrow[G] = mct
  }
}

implicit def natchezMtlTraceViaLocal[F[_]](
  implicit ev: LocalSpanAndMonadPartialOrderAndMonadCancelThrow[F],
  F: MonadCancelThrow[F],
): Trace[F] = ???

@bpholt
Copy link
Member Author

bpholt commented Jan 31, 2023

I think I got that working on Scala 2.13 and 3, but I haven't been able to get it to infer on 2.12. I also had to change the app code method definition to

def foo[F[_]: LocalSpanAndMonadPartialOrderAndMonadCancelThrow : MonadCancelThrow]: F[Unit]

which still doesn't feel ideal since it's a new thing that would have to be explained and understood.

I added a LocalTraceSpec to demonstrate what I'm trying to accomplish, if that helps.

@armanbilge
Copy link
Member

Hm I see 🤔 in that case, I wonder if the partially-applied trick would be better.


I think this boils down to:

  • Generic APIs, without hard-coding e.g. Kleisli.
  • No reference to secondary monad G[_] in user-facing code.
  • No "tricks", like partially-applied methods or LocalSpanAndMonadPartialOrderAndMonadCancelThrow.

Pick 2. Suffice to say, I see the merits of your original proposal 😅

Still, I think the implementation should be in terms of MonadPartialOrder, even if its exposed as a Kleisli-specific API.

@bpholt
Copy link
Member Author

bpholt commented Jan 31, 2023

Maybe I've been looking at this too long and I'm just missing something, but I updated the implementation to use MonadPartialOrder as much as I could see is possible.

(I did take it a lot farther on another branch, introducing a new thing I called InvariantMonadPartialOrder to encode the bidirectional F ~> G and G ~> F, and bringing in alleycats Empty to remove Span[F] in the instance of the InvariantMonadPartialOrder, but it felt like overkill, especially since it still ultimately needs to have Span[F] hardcoded in localSpanForKleisli.)

@armanbilge
Copy link
Member

I'm just missing something

Oh crap, no I think it's just me 😬 as soon as you mentioned Invariant it all came back. You're right; my original premise was wrong, G ~> F/MonadPartialOrder is not sufficient to implement this. You need the F ~> G as well.

I previously learned this in #448, but seems I forgot 🤦

So sorry for the wild goose chase, thanks for trying all my suggestions 😕

@bpholt
Copy link
Member Author

bpholt commented Feb 1, 2023

No worries! Given that, do you still think the changes in dcf71de (that introduced MonadPartialOrder here) are still worth it?

@armanbilge
Copy link
Member

armanbilge commented Feb 1, 2023

Still, I think the implementation should be in terms of MonadPartialOrder, even if its exposed as a Kleisli-specific API.

Let me revise this (and hopefully get it right this time :) — the implementation should be in terms of MonadPartialOrder[F, G] (or maybe just F ~> G) and G ~> F.

To be more concrete, can we implement this roughly signature?

  def natchezMtlTraceForLocal[F[_], G[_]](implicit
      local: Local[F, Span[G]],
      F: MonadCancel[F, _],
      lift: G ~> F,
      lower: F ~> G
  ): Trace[F] = ???

and then we just do something like this?

implicit def localSpanForKleisli[F[_]: MonadCancel] = natchezMtlTraceForLocal[Klesli, F]

and another one for the case where F == G.

@bpholt
Copy link
Member Author

bpholt commented Feb 1, 2023

I think that signature maybe misses the point of this PR? This is meant to handle the situation where I have a method

def doSomethingInNewRoot[F[_]](entryPoint: EntryPoint[F])(implicit L: Local[F, Span[F]])

and I want to call it with F[_] as Kleisli[F, Span[F], *].

In this situation, I don't need a Trace[F] until later. (And once I have a Local[F, Span[F]], I can already get a Trace[F] when it's needed).

@armanbilge
Copy link
Member

I think that signature maybe misses the point of this PR?

Gah 🤦 yes it does. I'm ... going to shut my mouth now 😅 the call site is Local[F, Span[F]] and we can't change that without degrading the ergonomics. Meanwhile, I agree, I don't see a way to break this down into other abstractions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants