-
Notifications
You must be signed in to change notification settings - Fork 521
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
cats.mtl.Local[IO, E] from IOLocal[E] #3429
base: series/3.x
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs docs and such, but I wanted to float the interface first.
@@ -319,4 +332,6 @@ object IOLocal { | |||
underlying.get.flatMap(s => underlying.reset.as(getter(s))) | |||
} | |||
|
|||
def local[E](e: E): IO[Local[IO, E]] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
otel4s has integrated LiftIO
, but I'm not sure this is necessary: cats.mtl already lifts a Local[F, E]
to all the common transformers over F
. Everyone would pay the LiftIO
tax just to cover any LiftIO
instances not covered by Local
.
def local[B](iob: IO[B])(f: A => A): IO[B] = | ||
self.modify(e => f(e) -> e).bracket(Function.const(iob))(self.set) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could conceivably be made a method on IOLocal
itself as well, with this becoming a simple delegate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like thin instances, but if we're discouraging non-Local use of IOLocal
, maybe we shouldn't make it easier to use.
@@ -319,4 +332,6 @@ object IOLocal { | |||
underlying.get.flatMap(s => underlying.reset.as(getter(s))) | |||
} | |||
|
|||
def local[E](e: E): IO[Local[IO, E]] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love the name.
implicit val local: Local[IO, Int] = | ||
// Don't try this at home | ||
unsafeRun(IOLocal.local(0)).fold( | ||
throw new CancellationException("canceled"), | ||
throw _, | ||
_.get | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also use syncStep
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The left seems as messy as this, but I might be missing something.
def local[E](e: E): IO[Local[IO, E]] = | ||
IOLocal(e).map(_.toLocal) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be interesting to see this as a method on IO
so you can do IO.local(e): IO[Local[IO, E]]
. It also insulates the API from the more complex stuff going on in IOLocal
—thinking from the perspective of users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's exactly where I wanted to put it, but it creates a cycle between IO
and IOLocal
that isn't already there. I don't think I mind: I can't imagine those ever not being in the same module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They'll definitely always be in the same module. There's already an indirect cycle (through the IO
case class structure and IOFiber
) due to the implementation, so I don't think there's any real concern about making the ABI directly cyclic.
This is the one thing that gives me pause. I'm concerned about it mostly because I suspect there are breaking changes we will want to make to Cats MTL in the near-ish term, which transitively breaks Cats Effect. However, as you pointed out, testkit already implies this version dependency, which is unfortunate but definitely undercuts my concern. |
Particularly with an eye towards otel4s, are we sure we want to privilege bare |
I don't see that otel4s privileging bare I do want to be cautious here, because if cats-mtl is not stable, this PR makes everything unstable. But we hope to integrate otel4s into the major networking libraries, and without cats-mtl, we're missing a typeclass and then really are privileging My two questions:
|
I imagine very little changing in the typeclasses. I'm mostly interested in smoothing the introduction/elimination of capabilities, particularly those with lexical scopes (i.e. all the interesting ones). That's probably something that can be done in a binary-compatible fashion, but I don't know for sure until I really go deep on it. |
otel4s could kick this can down the road by not depending on cats-mtl, restricting itself to IOLocal operations representing local semantics, and either removing support or massively duplicating code for Kleisli and all transformers. natchez-core is an approximation of what that road looks like, and it would be nice not to repeat that aspect of it. When you say "ossifies a very particular propagation semantic," otel4s is pretty much going to have to do so with respect to tracing. We can have a tracer that uses local semantics, which sucks for spanning |
Note that there is also lawful |
I'm not sure I get the problem - one fiber's view of an |
Can we get a non-draft version of this? |
cats.mtl.Local
's semantics are a safe and useful subset of what can be done withIOLocal
. This provides easy access to aLocal[F, E]
instance from anIOLocal[E]
value.IOLocal#asLocal
to get a Local view of anIOLocal
valueIOLocal.local
to create anIOLocal
and return a local view of it, insideIO
.A similar view is already in use in otel4s.
Fixes #3385.