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

IOLocal: MTL Local instance & semantics #3385

Closed
armanbilge opened this issue Jan 27, 2023 · 10 comments · Fixed by #3429
Closed

IOLocal: MTL Local instance & semantics #3385

armanbilge opened this issue Jan 27, 2023 · 10 comments · Fixed by #3429
Milestone

Comments

@armanbilge
Copy link
Member

@bpholt recently brought up the issue of providing an MTL Local instance for IOLocal in armanbilge/oxidized#9 (comment). That repo exists because Cats MTL can't provide it (since it creates a dep cycle with CE), and Cats Effect hasn't provided it (so far). Here are some reasons we should reconsider.

  1. As noted, Cats Effect already has a dependency on MTL, via the testkit. So it's not technically increasing our bincompat vulnerability.1
  2. http4s is pursuing MTL-based middleware for 1.0. So MTL will likely end up on the classpath of all practical Typelevel applications anyway.

Furthermore: there is a lot of frustration about the semantics to expect from IOLocal e.g. #3100, typelevel/fs2#2842.

The interesting thing is, if you use IOLocal only as a Local (i.e. none of its lower-level methods) I am pretty sure you won't run into weird behavior like that. Local encodes semantics that make it safe to use IOLocal, without leaking implementation details about how Stream or whatever uses Fibers under-the-hood. Of course, it is less powerful when used this way, but that's par-for-the-course.

So I think there's an argument to push Local as the blessed way to interact with an IOLocal, unless you are specifically doing low-level stuff. Think .background to .start. And that could be another good reason to provide such an instance directly.

Footnotes

  1. although I suppose in the current state we could break the testkit if MTL had to break, and it would be less bad than breaking core.

@armanbilge armanbilge changed the title IOLocal: MTL Local instance semantics IOLocal: MTL Local instance & semantics Jan 27, 2023
@rossabaker
Copy link
Member

Would you construct it from an extant IOLocal or create an encapsulated one from an initial state so nobody else can do weird shit with it?

@armanbilge
Copy link
Member Author

create an encapsulated one from an initial state so nobody else can do weird shit with it?

IMO this should be the "blessed" way, but I think the alternative constructor is good to have around too.

@rossabaker
Copy link
Member

rossabaker commented Feb 6, 2023

An attractive feature of MTL local is that it surfaces the inherent limitations at compile time. For example, scoping the use of a resource. By signature, you can't!

    def localK[F[_], E](f: E => E)(implicit L: Local[F, E]) =
      new (F ~> F) {
        def apply[A](fa: F[A]): F[A] =
          L.local(fa)(f)
      }

    implicit def effectLocal[E](implicit ioLocal: IOLocal[E]) = new Local[IO, E] {
      val applicative = Applicative[IO]
      def ask[E2 >: E] = ioLocal.get
      def local[A](fa: IO[A])(f: E => E): IO[A] =
        ioLocal.modify(e => (f(e), e)).bracket(_ => fa)(ioLocal.set)
    }

    implicit def streamLocal[E](implicit ioLocal: IOLocal[E]) = new Local[Stream[IO, *], E] {
      val applicative = Applicative[Stream[IO, *]]
      def ask[E2 >: E]: Stream[IO, E2] = Stream.eval(ioLocal.get)
      def local[A](stream: Stream[IO, A])(f: E => E): Stream[IO, A] =
        stream.translate(localK(f)(effectLocal(ioLocal)))
    }

    implicit def resourceLocal[E](implicit ioLocal: IOLocal[E]) = new Local[Resource[IO, *], E] {
      val applicative = Applicative[Resource[IO, *]]
      def ask[E2 >: E]: Resource[IO, E2] = Resource.eval(ioLocal.get)
      def local[A](r: Resource[IO, A])(f: E => E): Resource[IO, A] =
        r.mapK(localK(f)(effectLocal(ioLocal)))
    }

    IOLocal(0).flatMap { implicit ioLocal =>
      val r = Local[Resource[IO, *], Int].scope(Resource.unit)(1)
        // use/surround has to happen outside scope, will be 0
        .surround(Local[IO, Int].ask[Int])
      val s = Local[Stream[IO, *], Int].scope(
        // ask is inside scope, will be a stream of 1
        Local[Stream[IO, *], Int].ask[Int])(1)
        .compile.toVector
      (r, s).tupled
    }

@rossabaker
Copy link
Member

I'm increasingly convinced this is a good idea. Would a concrete PR to discuss be helpful?

@armanbilge
Copy link
Member Author

I'm obviously in favor of moving forward with this, and I think a PR would be a great next step :)

A question at the back of my mind has been where to expose this, so that it doesn't get muddied with the lower-level IOLocal. I wonder if we should put this on the IO companion object? That seems like a friendly, "high-level" location for it.

object IO {
  def local[A](initial: A): IO[Local[IO, A]] = ???
}

@iRevive
Copy link
Contributor

iRevive commented Feb 7, 2024

What is the current state of this issue? Is there any chance we can have it in the upcoming 3.6.0 release?

@djspiewak
Copy link
Member

I'm still hemming and hawing about the direct Cats MTL dependency from Core. But the reality is this is probably a very good idea.

@kubukoz
Copy link
Member

kubukoz commented Sep 11, 2024

Can we apply some democracy 🦅 here and overthrow Daniel's hemming and hawing? 😅

@armanbilge
Copy link
Member Author

@kubukoz last that Daniel and I talked I think he's come around on adding this.

@djspiewak
Copy link
Member

Confirming that we should just add this. I increasingly think that Cats MTL is a pretty important pillar of improving the ergonomics of our ecosystem, so it kind of makes sense that it ends up everywhere.

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

Successfully merging a pull request may close this issue.

5 participants