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

Opaque transformers types #3919

Closed
wants to merge 3 commits into from
Closed

Opaque transformers types #3919

wants to merge 3 commits into from

Conversation

diesalbla
Copy link
Contributor

@diesalbla diesalbla commented Jun 5, 2021

This Pull Request adds a implementation of the monad transformers OptionT, IdT, and Kleisli, for Scala 3. This new implementation uses the new feature of Scala 3, Opaque Types, which may allows us to have them implemented without overhead on the runtime memory.

This may slightly improve performance for libraries that are based on these monad transformers, such as http4s.

diesalbla added 3 commits June 5, 2021 03:35
Scala-3 comes with its own zero-cost Newtype implementation, as Opaque
Types. This is an improvement over existing case classes, or Newtype
encodings we are using.

In this commit, we bifurcate the `IdT` wrapper in two implementations:
the existing one goes to the Scala-2.x branch, a new one with newtype
is used on the new
Copy link
Member

@ChristopherDavenport ChristopherDavenport left a comment

Choose a reason for hiding this comment

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

I can really tell how well this source aligns. Are there source incompatibilities introduced with this?

@rossabaker
Copy link
Member

This is similar to #3834. I still like the idea, but I remain concerned about the maintenance burden.of two duplicate implementations. Could we mitigate that with some tests that assure compatibility across Scala 2 and Scala 3? Or maybe our test coverage is already sufficient?

Copy link
Contributor

@johnynek johnynek left a comment

Choose a reason for hiding this comment

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

I think this is worth pursuing personally.

I think we should relax our binary compat for scala 3 to be able to merge something like this (as long as we can avoid any special casing in the tests and we expect source compatibility).

* res0: ParseResult[Int] = Right(12)
* }}}
*/
def local[AA](f: AA => A): Kleisli[F, AA, B] = aa => x(f(aa))
Copy link
Contributor

Choose a reason for hiding this comment

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

what about f.andThen(x)

c => F.map(x(f(c)))(g)

def mapF[N[_], C](f: F[B] => N[C]): Kleisli[N, A, C] =
a => f(x(a))
Copy link
Contributor

Choose a reason for hiding this comment

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

x.andThen(f)

@smarter
Copy link
Contributor

smarter commented Jan 27, 2022

I think we should relax our binary compat for scala 3

There's many libraries published for scala 3 already that depends on cats, are they all OK with throwing bincompat out of the window for hypothetical performance benefits? IMO this belongs in a cats 3 branch or in https://github.com/typelevel/spotted-leopards

@mpilquist
Copy link
Member

Totally agree with @smarter on bin compatibility. If we wanted to get this in to cats 2.x, we'd have needed to do it prior to Scala 3 final release. I'd really like to see this included in the future though (perhaps Cats 3) along with built-in support for derivation (not via an add-on project).

@armanbilge
Copy link
Member

Btw, for anyone who cares about preserving bin-compat on Scala 3, we still have to enable MiMa for it. My PR needs feedback at #4063 (comment).

@armanbilge
Copy link
Member

I don't think this one is happening until Cats 3.

@armanbilge armanbilge closed this May 7, 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.

7 participants