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

Kleisli contravariant on input type A #2843

Merged
merged 2 commits into from May 28, 2019

Conversation

Projects
None yet
7 participants
@jcouyang
Copy link
Contributor

commented May 14, 2019

Rationale: #2749
also as described in the test, it's more composable

    val program = for {
      k1 <- Kleisli((a: A1) => List(1))
      k2 <- Kleisli((a: A2) => List("2"))
      k3 <- Kleisli((a: A3) => List(true))
    } yield (k1, k2, k3)

scala can infer type to be Kleisli[List, A1 with A2 with A3, (Int, String, Boolean)]
if not using scala subtyping it would looks like

    val program = for {
      k1 <- Kleisli((a: A1) => List(1)).local(identity[A1 with A2 with A3])
      k2 <- Kleisli((a: A2) => List("2")).local(identity[A1 with A2 with A3])
      k3 <- Kleisli((a: A3) => List(true)).local(identity[A1 with A2 with A3])
    } yield (k1, k2, k3)

jcouyang added some commits May 14, 2019

@codecov-io

This comment has been minimized.

Copy link

commented May 14, 2019

Codecov Report

Merging #2843 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2843   +/-   ##
=======================================
  Coverage   94.21%   94.21%           
=======================================
  Files         368      368           
  Lines        6944     6944           
  Branches      301      301           
=======================================
  Hits         6542     6542           
  Misses        402      402
Impacted Files Coverage Δ
core/src/main/scala/cats/data/Kleisli.scala 90.99% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 912a14a...9294d3b. Read the comment docs.

@LukaJCB
Copy link
Member

left a comment

This seems like a pretty reasonable change, given that we already have variance annotations for other data types in cats.data, thanks a bunch for filing this PR :)
Interested in what other maintainers think

@kailuowang

This comment has been minimized.

Copy link
Member

commented May 16, 2019

I am a little concerned with the source breaking widening, although the chance of them actually breaking on usesite is probably not very high.
To avoid source breaking change, an alternative might be adding a narrow method to Kleisli. The narrow syntax comes with Contravariant doesn't work due to fact that the contravariant type A is not the most right, and as a result, partial unification doesn't help.
We can still add a def narrow[AA <: A]: Kleisli[F, AA, B] = this.asInstanceOf, then user can manually call kleisli.narrow when needed. That would probably make life easier for a lot of use cases.

I would like to invite feedbacks from @rossabaker and @ChristopherDavenport on this one, given that Kleisli is the primitive in Http4s.

@rossabaker

This comment has been minimized.

Copy link
Member

commented May 16, 2019

I don't have a great intuition for inference problems. I'll try to publish a local version of cats and build http4s against that and look for trouble this week.

@kailuowang

This comment has been minimized.

Copy link
Member

commented May 16, 2019

Thanks a lot @rossabaker

@jdegoes

This comment has been minimized.

Copy link

commented May 27, 2019

@jcouyang 👍

Not only should Kleisli be contravariant in A, but it should also be covariant on B.

It will most likely be source breaking, because in some cases users will have to annotate methods with the newly-added type parameter.

@LukaJCB

This comment has been minimized.

Copy link
Member

commented May 27, 2019

@jdegoes I disagree, B can only be covariant if F has a Functor instance and vice versa for covariant. This means that by default Kleisli can only be invariant, unless we want to specialize it to only covariant functors.

@jdegoes

This comment has been minimized.

Copy link

commented May 27, 2019

@LukaJCB Yes, I believe it's the correct decision to specialize for covariant functors.

If anyone uses Kleisli with contravariant/invariant functors — and I haven't seen that in the wild — then this use case should be specialized as ContraKleisli, for example, with contravariant annotation on B.

This is one of the many cases where embracing specialization results in unquestionably higher ergonomics at the loss of abstraction of questionable value that most users don't care about.

@kaishh

This comment has been minimized.

Copy link

commented May 27, 2019

What about revisiting the stance on covariance in monad transformers while we're at it?
I don't find the arguments in the FAQ particularly convincing, as ALL monads must be covariant by definition, if we desire to apply a monad-transformer like structure to a non-monad, maybe we ought to just make a different type? e.g. OptionNested, KleisliLike, etc.

EDIT: Sorry, I did not update the page when submitting and did not see John's post which is more-or-less the same

@jdegoes

This comment has been minimized.

Copy link

commented May 27, 2019

@kaishh 👍 Major surgery required.

@jcouyang

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

Thanks @jdegoes, yeah I think as well it should be -A and +B
while for now contravariant has more practical use case(it's hard to predict wisely what should a composition of various Kleisli take as input type beforehand, but it's relatively easy to reason and adapt output type) and less likely widely breaking source (I published local and one of my project that using http4s still compiled, like to see what outcome @rossabaker get though )

@rossabaker
Copy link
Member

left a comment

This compiles with no changes on http4s-0.20.x when cherry-picked onto cats-1.6.0, under both Scala 2.11.12 and Scala 2.12.8.

@kailuowang kailuowang added this to the 2.0.0-RC1 milestone May 28, 2019

@rossabaker rossabaker merged commit 4f92efe into typelevel:master May 28, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kailuowang kailuowang modified the milestones: 2.0.0-RC1, 2.0.0-M2, 2.0.0-M3 Jun 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.