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

FiberLocal typeclass #1847

Closed
wants to merge 5 commits into from

Conversation

TimWSpence
Copy link
Member

I attempted to capture the current behaviour in a typeclass with laws to get the ball rolling on this. Pretty much all of it is up for debate!

It's also not binary-compatible so keen for feedback on what the cleanest way to restore bin-compat is


package cats.effect

sealed trait IOLocal[A] {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point we can't remove this anymore without breaking binary compatibility, IMO it can stay and we can just forward to it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worst case make it private[effect] but still use it, if we want to discourage direct usage.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Yeah, I noted that in the message above but thought it wasn't worth dealing with until there is actually agreement on the design.

How does making it private[effect] not break bincompat sorry?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class and methods will still be there at runtime, so 2rd party libraries built with 3.0.0 can continue to use them, but users who upgrade won't be able to touch them (because of the source level private modifier)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shockingly TIL visibility modifiers are only checked at compile time 🤦 😂 I did actually wonder if that was what you were saying but thought surely I would know that by now 😂

Thanks! 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think private might actually make a difference, but the qualified variant (private[pkg]) won't :D


trait FiberLocal[F[_]] {

def F: Concurrent[F]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can probably be abstracted to GenConcurrent[F, _]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think FiberLocal could also be renamed to GenLocal with the appropriate aliases. Then we do FiberLocal and FiberRef for the data structures

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Does the Throwable alias just become Local then?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think LocalThrow ?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh no Local you are right

Copy link

@RaasAhsan RaasAhsan Mar 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh I didn't realize you were doing the scato encoding here 👍 that avoids coherence issues

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also doesn't seem that useful for the core algebra, but maybe there's something we can find there (e.g. do any other operators derive from this)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it would have to live as a subclass of GenConcurrent and then we'd have to impose an arbitrary subtyping relationship between GenTemporal and GenLocal or vice versa so it is probably better off to the side

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't understand this part

Why do we need that at all? Errors aren't part of the algebra

Seems to me like we're unnecessarily abstracting on the error here instead of quantifying existentially (GenConcurrent[F, _]). Unless we want to use errors / failed effects in laws?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry yeah, good point - we're not using them in the laws (actually we're not using Concurrent either any more - I think it can be downgraded to Spawn) 👍 @RaasAhsan happy with this?

@RaasAhsan
Copy link

#1822 is related to this as well

@RaasAhsan
Copy link

BTW there was a lot of discussion around whether we should have this as late as yesterday, but the main point of contention was that we're not sure if Monix and ZIO's local semantics will support ours. I believe we got confirmation that Monix doesn't, but Monix on CE3 doesn't exist yet and @Avasil expressed interest in a typeclass, so that could be up in the air. I think ZIO does, but we should double check.


def F: Concurrent[F]

def local[A](default: A): F[Local[F, A]]
Copy link

@RaasAhsan RaasAhsan Mar 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if FiberRef is 👍 , we can add a default constructor for it here in terms of local and Concurrent[F] (in a bincompat manner even, if it comes later)

@kubukoz
Copy link
Member

kubukoz commented Mar 30, 2021

I'll make it a draft for now to avoid accidental merges, but feel free to undraft when we have that Spawn stuff figured out :)

@kubukoz kubukoz marked this pull request as draft March 30, 2021 13:55
@kubukoz kubukoz mentioned this pull request Apr 4, 2021
@vasilmkd vasilmkd added this to the 3.2.0 milestone Jun 18, 2021
@djspiewak djspiewak removed this from the 3.2.0 milestone Jul 16, 2021
@djspiewak djspiewak closed this Feb 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants