Skip to content

Add Async#syncStep - #2835

Merged
djspiewak merged 13 commits into
typelevel:series/3.xfrom
armanbilge:feature/async-sync-step
Feb 27, 2022
Merged

Add Async#syncStep#2835
djspiewak merged 13 commits into
typelevel:series/3.xfrom
armanbilge:feature/async-sync-step

Conversation

@armanbilge

@armanbilge armanbilge commented Feb 23, 2022

Copy link
Copy Markdown
Member

Takes the ideas from #1945 and #2613 and replays it for Async[F]. I think this is the law we are aiming for:

F.syncStep[G, A](fa, limit).to[F].flatMap {
  case Left(fa) => fa
  case Right(a) => F.pure(a)
} <-> fa

We should also consider adding Dispatcher#unsafeRunSyncToFuture and Dispatcher#unsafeRunSyncToPromise to match the methods on IO. Slightly annoying is the need to explicitly pass a limit since it cannot be retrieved from the IORuntime in this case.

interpret(ioe, limit - 1)
.map {
case Left(_) => Left(io)
case Right((a, limit)) => Right((a.asRight[Throwable].asInstanceOf[B], limit))

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I couldn't figure out how to get the compiler to like this without the cast 😕

Comment on lines +434 to +440
override def syncStep[G[_], A](fa: IorT[F, L, A], limit: Int)(
implicit G: Sync[G]): G[Either[IorT[F, L, A], A]] =
G.map(F.syncStep[G, Ior[L, A]](fa.value, limit)) {
case Left(ior) => Left(IorT(ior))
case Right(Ior.Right(a)) => Right(a)
case Right(right) => Left(IorT.fromIor(right))
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The transformers were quite interesting to implement :) the law is actually quite important here in guiding the right behavior: we can only return A if we pattern match the pure representation for the underlying datatype.

@djspiewak

Copy link
Copy Markdown
Member

Will look at this in more detail in a bit. I want to leave it open for discussion for now though since this would be a pretty significant addition to Async. Paging @SystemFw for galaxy brain thoughts, as well as absolutely anyone else who wants to weigh in.

@armanbilge

Copy link
Copy Markdown
Member Author

I just added a slightly tweaked version of my law from above. I didn't realize SyncIO#to was not defined on Sync itself (makes sense in retrospect) but actually there's nothing to stop us from syncSteping into the same effect 😁

def syncStepIdentity[A](fa: F[A], limit: Int) =
F.syncStep[F, A](fa, limit)(syncF).flatMap {
case Left(fa) => fa
case Right(a) => F.pure(a)
} <-> fa
// a mild hack, in case a `syncStep[G]` implementation
// special-cases `F eq G` and thus short-circuits the law
private[this] def syncF: Sync[F] = new Sync[F] {

Comment thread kernel/shared/src/main/scala/cats/effect/kernel/Async.scala Outdated
Comment thread kernel/shared/src/main/scala/cats/effect/kernel/Async.scala
Comment thread laws/shared/src/main/scala/cats/effect/laws/AsyncLaws.scala Outdated
Co-authored-by: Daniel Spiewak <djspiewak@gmail.com>
@djspiewak

Copy link
Copy Markdown
Member

Approved once the above two minor nits are addressed. I think this is a good change overall.

@djspiewak

Copy link
Copy Markdown
Member

Oh actually, one further request: can you add this to AsyncSyntax?

Comment on lines +1650 to +1651
type G0[+A] = G[A @uncheckedVariance] // helps with type inference
def interpret[B](io: IO[B], limit: Int): G0[Either[IO[B], (B, Int)]] = {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sorry, one last hack. I'd rather do this than the asInstanceOf below: type-erasure makes this okay, but unsafe casts are a footgun.

Thoughts?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Nevermind, Scala 3 doesn't like this.

@armanbilge

Copy link
Copy Markdown
Member Author

We should also consider adding Dispatcher#unsafeRunSyncToFuture and Dispatcher#unsafeRunSyncToPromise to match the methods on IO. Slightly annoying is the need to explicitly pass a limit since it cannot be retrieved from the IORuntime in this case.

I tried doing this, but realized we basically can't.

  1. We can't provide default implementations of these without requiring an Async[F] constraint on the methods.
  2. Without SyncIO we have no way to unsafeRunSync anyway 🙃

In practice this is not a blocker since these can be implemented in user-land, but it is kind of annoying. Maybe could add them as syntax for Dispatcher in core?

@djspiewak

Copy link
Copy Markdown
Member

In practice this is not a blocker since these can be implemented in user-land, but it is kind of annoying. Maybe could add them as syntax for Dispatcher in core?

Hmm, let's tackle this one in a follow up. We basically have to weigh whether a syntax enrichment in core is better or worse than a significantly-altered signature in std (e.g. taking a function of type G[A] => A as a parameter).

@djspiewak
djspiewak merged commit 6709a1d into typelevel:series/3.x Feb 27, 2022
@armanbilge

Copy link
Copy Markdown
Member Author

taking a function of type G[A] => A as a parameter

It would have to be G[A] => Either[Throwable, A] I think, but I suppose that's an option too 🤔

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.

2 participants