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

Polyfunction revamp #555

Merged
merged 9 commits into from
Oct 28, 2022
Merged

Polyfunction revamp #555

merged 9 commits into from
Oct 28, 2022

Conversation

Baccata
Copy link
Contributor

@Baccata Baccata commented Oct 26, 2022

This is a revamp of a few things, in an effort to increase consistency in the various kinds of polymorphic functions that Smithy4s (and its users) deal with. In particular, it unifies the patterns between PolyFunction and what used to be Transformation (now called PolyFunction5) by means of build-time generation.

It also recycles the term Transformation, making it a function-like capability that allows to provide a unified user-experience across polymorphic things without requiring the user to memorise ungodly names like "mapK, mapK5", etc ... : generated Services all receive a transform method that can take anything that has an associated Transformation, be that Polyfunction5, PolyFunction, and in the future cat's FunctionK, or other things such as error amendments, or transformations allowing to go from monofunctors to bifunctors (or vice-versa).

  • Things in the kinds package are not meant to be used/viewed by most users. It's mostly for us, implementors of interpreter layers. bikeshedding around names is welcome, naming these things is hard, so I want to make sure our small group of maintainers is roughly on the same page.

  • Try to make working with various kinds more consistent

  • Use code generation to generate consistent versions of PolyFunction for various kinds

  • Add FunctorK typeclasses for all kinds we're interested to deal with.

  • Change the concept of Transformation altogether to be able to offer a unified developer experience where algebra could be transformed using various functions and natural transformations, without the end user having to remember complicated names.

  • Change Service interface to expose toPolyFunction and fromPolyFunction functions, allowing to turn an Algebra into a PolyFunction5 and vice-versa.

  • Paves the road for an easier BiFunctor integration (EitherT, ZIO, MonixBIO, etc) by adding a few constructs and aliases.

TODO

  • some scaladoc around the various constructs and types
  • going through the docs and amend stale/obsolete reference to names
  • more transformation tests

* Try to make working with kinds more consistent
* Use code generation to generate consistent versions of PolyFunction
for various kinds
* Change the concept of Transformation altogether to be able to offer
a unified developer experience where algebra could be transformed using
various functions and natural transformations, without the end user
having to remember complicated names.
* Change Service interface to expose "toPolyFunction" and
"fromPolyFunction" functions, allowing to turn an Algebra into a
PolyFunction5 and vice-versa.
* Add FunctorK typeclasses for all kinds we're interested to deal with.
@Baccata Baccata added this to the 0.17.0 milestone Oct 26, 2022
case object Empty extends Throwable

// Not ascribing the type to get a
val transformed = stub.transform(new PolyFunction[Option, Try] {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is one of the motivations for this PR : we want for users to be able to apply polymorphic functions of various shapes with a single transform method, and for it to work reasonably well wrt type inference.

  • TODO : need to complete the comment above this line

@Baccata
Copy link
Contributor Author

Baccata commented Oct 26, 2022

@jona7o, I'd like to extend you the courtesy of asking for your opinion on naming and stuff, as this is both source and binary breaking and will induce some changes to your libraries (nothing fundamental, mostly mechanical things).

*
* Used to reduce the noise of transformations
*/
trait Transformation[Func, Input, Output] {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is probably the most complicated thing here : essentially this is a construct that says "I, Func, am a polymorphic function of some kind, and when given an input, I can produce an output". For example, we use it to express the fact that we know how to apply a good old F ~> G (with F[_] and G[_]) on something that looks like Foo[F[_, _, _, _, _]].

This allows to make it simpler for the user to apply generic transformations on the instances of the generated interfaces, without having to worry about the kinds at hand.

TBH : I'm not fully certain it's a good idea. I just want to avoid having the users having to worry about the humongous kinds we deal with in this library, and for cat's FunctionK to just "work" with it modulo a simple import.

)(implicit
serviceProvider: smithy4s.Service.Provider[Alg, Op],
F: EffectCompat[F]
): RouterBuilder[Alg, Op, F] = {
val service = serviceProvider.service
new RouterBuilder[Alg, Op, F](
service,
service.asTransformation[GenLift[F]#λ](impl),
service.toPolyFunction[Kind1[F]#toKind5](impl),
Copy link
Contributor

Choose a reason for hiding this comment

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

I love this

Copy link
Contributor

@lewisjkl lewisjkl left a comment

Choose a reason for hiding this comment

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

I'm sure you're already thinking along these lines, but since one motivation for the PR is to make transformations (er polyfunctions) easier to implement in user-land, we should make sure we document what that looks like. Happy for that to be in a different PR though.

Comment on lines -260 to +251
line"def transform[P[_, _, _, _, _]](transformation: $Transformation_[$opTraitNameRef, P]): $genNameRef[P] = reified.transform(transformation)",
line"def mapK5[P[_, _, _, _, _], P1[_, _, _, _, _]](alg: $genNameRef[P], f: $PolyFunction5_[P, P1]): $genNameRef[P1] = new $Transformed_(alg, f)",
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this much more 👍

service.opToEndpoint.andThen[GenLift[F]#λ](
new Transformation[Endpoint[Op, *, *, *, *, *], GenLift[F]#λ] {
): FunctorAlgebra[Alg, F] = {
service.fromPolyFunction[Kind1[F]#toKind5] {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah this is a big improvement over GenLift imo

* @author Miles Sabin
* @author Kevin Wright
*/
object Boilerplate {
Copy link
Contributor

Choose a reason for hiding this comment

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

🙌 nice

Comment on lines +163 to +168
else
commonCompilerOptions
// ++ Seq(
// "-Xsource:3",
// "-P:kind-projector:underscore-placeholders"
// )
Copy link
Contributor

Choose a reason for hiding this comment

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

Extra comment?

Comment on lines +7 to +9
type FunctorInterpreter[Op[_, _, _, _, _], F[_]] = PolyFunction5[Op, Kind1[F]#toKind5]
type BiFunctorAlgebra[Alg[_[_, _, _, _, _]], F[_, _]] = Alg[Kind2[F]#toKind5]
type BiFunctorInterpreter[Op[_, _, _, _, _], F[_,_]] = PolyFunction5[Op, Kind2[F]#toKind5]

Choose a reason for hiding this comment

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

Whats your take on that FunctorTransformer/FunctorTransformation would be a more fitting type alias for FunctorInterpreter
@Baccata

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmm in the current branch, the Transformation construct is decoupled from Kind5, so I'd rather not use that particular terminology. Internally we've been using Interpreter because we use them to express transformations that take a reified operation instance and "interpret" as some effect, which is similar to what happens in the free-monad pattern. But I can agree that it may not be the best name in this location.

To be as thorough/accurate as possible, it'd be FunctorProjection5 and BiFunctorProjection5, as it doesn't presume to produce an effect, just projects a type of kind 5 into a type of kind 1 or kind 2.

Maybe it should be Kind5[Op]#toKind1[F] and Kind5[Op]#toKind2[F] ?

What do you think, @disneystreaming/smithy ?

(naming is freaking hard)

Choose a reason for hiding this comment

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

I like the idea of FunctorProjection5 and BiFunctorProjection5. Seems more fitting than Interpreter in this use case. Taking the free-monad in consideration the current naming also makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea of FunctorProjection as well. I am usually fine with long names though if they are more accurate or descriptive.

Copy link
Member

Choose a reason for hiding this comment

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

I like FunctorInterpreter because I feel like the "interpreter" part sort of explains what it does, not just what it is technically - a more semantic name than "projection". I'm fine with either though

@MoeQuadrat
Copy link

@jona7o, I'd like to extend you the courtesy of asking for your opinion on naming and stuff, as this is both source and binary breaking and will induce some changes to your libraries (nothing fundamental, mostly mechanical things).

Was no hassle to built in the current polyfunction revamp in our library.

@Baccata
Copy link
Contributor Author

Baccata commented Oct 27, 2022

but since one motivation for the PR is to make transformations (er polyfunctions) easier to implement in user-land, we should make sure we document what that looks like

Yup, will defo add some docs in a subsequent PR. As a matter of fact I'm thinking about having a smithy4s-cats module and offer a few OOTB transformations there. The documentation should tie into that

@kubukoz
Copy link
Member

kubukoz commented Oct 28, 2022

LGTM (just make sure to update with main before merging). I'd just suggest that we have a simple lookup map for old->new names in the release notes.

sth like:

old new
smithy4s.Monadic smithy4s.kinds.FunctorAlgebra
smithy4s.Interpreter smithy4s.kinds.FunctorInterpreter
asTransformation toPolyFunction

Playground seems happy with just the name swap: kubukoz/smithy-playground#129

As a matter of fact I'm thinking about having a smithy4s-cats module and offer a few OOTB transformations there.

I'm even thinking about an -effect module so that we could include stuff like this (requires CE-kernel):

def liftFunctorInterpreterResource[Op[_, _, _, _, _], F[_]: MonadCancelThrow](
  fir: Resource[F, FunctorInterpreter[Op, F]]
): FunctorInterpreter[Op, F] =
  new FunctorInterpreter[Op, F] {

    def apply[I, E, O, SI, SO](
      fa: Op[I, E, O, SI, SO]
    ): F[O] = fir.use(_.apply(fa))

  }

A reflective service is a service the algebra of which is a polymorphic
function (as opposed to an object-oriented interface). It helps reduce
some boilerplate that have to do with dynamic usage.
@Baccata Baccata marked this pull request as ready for review October 28, 2022 10:11
@Baccata
Copy link
Contributor Author

Baccata commented Oct 28, 2022

Alright, after deliberation, I think I'm gonna stay with FunctorInterpreter, for this PR anyway. I'm still open to changing, but considering it's just an alias, it's not the end of the world, and the intended usage is very much to be able to extend FunctorInterpreter, which communicates the intent better than FunctorProjection.

I might make it package private before the release, to encourage third parties to apply whatever aliases float their boats. Not quite sure.

@Baccata
Copy link
Contributor Author

Baccata commented Oct 28, 2022

Merging this to create a basis for 0.17, will address the todos in later PRs

@Baccata Baccata merged commit d57f7d1 into series/0.17 Oct 28, 2022
@Baccata Baccata deleted the polyfunction-revamp branch October 28, 2022 10:43
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.

6 participants