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

Translate SyncIO to F[_]: Sync #1953

Merged
merged 14 commits into from
Jul 7, 2021
Merged

Translate SyncIO to F[_]: Sync #1953

merged 14 commits into from
Jul 7, 2021

Conversation

vasilmkd
Copy link
Member

@vasilmkd vasilmkd commented May 9, 2021

Resolves #1790.

@vasilmkd vasilmkd changed the title synciotoio Add a translation mechanism between SyncIO and IO May 9, 2021
Copy link
Member

@djspiewak djspiewak left a comment

Choose a reason for hiding this comment

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

Aside from my one note, I quite like this!

core/shared/src/main/scala/cats/effect/IO.scala Outdated Show resolved Hide resolved
@vasilmkd vasilmkd requested a review from djspiewak May 10, 2021 21:50
@oleg-py
Copy link
Contributor

oleg-py commented May 15, 2021

Looks like the translation can work for any F[_]: Sync, avoiding the SyncIO <-> IO dependency question entirely.

Another thing is that we might want to make it uncancelable. If we consider code like this valid:

def thing[F[_]](implicit F: MonadCancelThrow[F]) = F.rootCancelScope match {
  case Uncancelable =>
    // We aren't gonna be cancelled, so no need for special handling
    F.delay(...).flatMap(...).attempt.flatMap(...).rethrow
  case Cancelable => F.raiseError(new Exception("Operation not supported for cancelable monads"))
}

then doing thing[SyncIO].toIO would break the assumption of thing that it can't be canceled.

@vasilmkd
Copy link
Member Author

vasilmkd commented May 15, 2021

Oof, you're so right about this being uncancelable. I don't know if we want to code it against Sync though. We had a to[F[_]: Async] on IO before and we deleted it, because there is no way for effects that support more features than the Async interface to translate all of their features in terms of Async. I understand that SyncIO is not that complicated, but still, there is a precedent where we had this and there was a decision to remove it.

cc @djspiewak

@vasilmkd
Copy link
Member Author

Made the resulting IO uncancelable.

@vasilmkd
Copy link
Member Author

vasilmkd commented Jun 9, 2021

@djspiewak I rebased this branch and addressed the comments.

I need your resolution on @oleg-py's comment about to[F[_]: Sync]. IMO, we removed this method on IO, because it was tying us to the typeclass hierarchy, so there is precedent to not proceed with this suggestion.

If that is the case, this PR is ready for merging.

@oleg-py
Copy link
Contributor

oleg-py commented Jun 9, 2021

IMO, we removed this method on IO, because it was tying us to the typeclass hierarchy, so there is precedent to not proceed with this suggestion.

"Tying to typeclass hierarchy" sounds odd as a justification. Sync is an "interface" both SyncIO and IO know about (yes, you "implement" it in the companion object, but it's already tied there), whereas there's no reason they, as concrete implementations, should know about each other. Isn't the point of typeclasses to allow reasonable abstraction over implementations?

We had a to[F[_]: Async] on IO before and we deleted it, because there is no way for effects that support more features than the Async interface to translate all of their features in terms of Async

It's the other way around, I suppose. You can't translate all of IO features (*looks at IOLocal*) in terms of Async alone. However it seems that, especially with changes to clock-related methods, SyncIO has become just free/initial encoding (+ an impure interpreter) of a Sync algebra, hasn't it? And therefore it should be perfectly reasonable to interpret it to any other Sync[F], also leaving our hands less tied w.r.t. #1861.

I guess the question is whether SyncIO is going to remain just a simplest Sync possible, or it's gonna have more custom features added. But 1) please don't and 2) SyncIO is most useful in places that are really different from places where IO is useful (notably places where you can't code to Sync[F] algebra).

@vasilmkd
Copy link
Member Author

vasilmkd commented Jun 9, 2021

I agree with these points. Do you think it would be strange to have a SyncIO#to[F[_]: Sync] but no IO#to[F[_]: Async]? If we can live with that difference, then I'm all for it. I also personally don't see SyncIO evolving any further than what it supports at the moment.

@oleg-py
Copy link
Contributor

oleg-py commented Jun 9, 2021

Do you think it would be strange to have a SyncIO#to[F[]: Sync] but no IO#to[F[]: Async]?

I think SyncIO.to[F[_]: Sync] fits "simplest Sync that's still handy" that SyncIO came out to be. That point doesn't apply to IO since CE3 has shifted its IO to being "best main effect type of your app", instead of "simplest XXX" that CE2 and before tried to have.

Also users will be asking about conversions of IO <-> Async[F] whether SyncIO has this method or not 😅

@vasilmkd
Copy link
Member Author

vasilmkd commented Jun 9, 2021

Great! Thanks for the discussion @oleg-py. I'll push out the changes later.

@vasilmkd vasilmkd marked this pull request as draft June 9, 2021 14:34
@vasilmkd vasilmkd changed the title Add a translation mechanism between SyncIO and IO Translate SyncIO to F[_]: Sync Jun 10, 2021
@vasilmkd vasilmkd marked this pull request as ready for review June 12, 2021 12:46
@vasilmkd
Copy link
Member Author

This is ready for merging.

@vasilmkd vasilmkd added this to the 3.2.0 milestone Jun 18, 2021
@djspiewak djspiewak merged commit 2e26a08 into typelevel:series/3.x Jul 7, 2021
@vasilmkd vasilmkd deleted the synciotoio branch July 8, 2021 21:54
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.

SyncIO -> IO conversion missing
4 participants