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

Scalaz 8 typeclass encoding. WIP. #52

Closed

Conversation

kamilkloch
Copy link
Contributor

First draft towards #26.
New encodings for:

  • ApplicativeAsk
  • ApplicativeLocal
  • MonadChronicle


import cats.Applicative

import acyclic.skipped
Copy link
Member

Choose a reason for hiding this comment

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

What does this do? Do we need it? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Result of scalacOptions += "-P:acyclic:force"

@LukaJCB
Copy link
Member

LukaJCB commented Aug 28, 2018

Looks good so far, thank you Kamil!
I wonder if we could get some good benchmarks to really motivate this :)

@kamilkloch
Copy link
Contributor Author

@LukaJCB please take a look into https://gitter.im/scalaz/scalaz, @edmundnoble shed some light.

@LukaJCB
Copy link
Member

LukaJCB commented Aug 29, 2018

Thank you @kamilkloch! I had a look and now I'm thinking, do we actually need to use composition in the mtl-subset at all, since ApplicativeAsk doesn't extend from Applicative, the only ambiguity that could occur from our side if ApplicativeLocal would extend from ApplicativeAsk would be to have something like this:

def foo[F[_], L](implicit ask: ApplicativeAsk[F, L], local: ApplicativeLocal[F, L]): F[L] = ???

I don't think this ever happens.

Maybe we could get rid of the BaseHierarchy altogether and just let ApplicativeLocal extend ApplicativeAsk, FunctorListen extend FunctorTell and ApplicativeHandle extend FunctorRaise.

Does that make sense?

@kamilkloch
Copy link
Contributor Author

@LukaJCB You are right. This would simplify the code at the cost of adding a highly unlikely potential implicit ambiguity. I was thinking of yet another thing: is there any way to actually inherit (not aggregate) from cats' typeclasses? I mean ApplicativeAsk[F[_], E] extends Applicative[F] ;)

@SystemFw
Copy link
Contributor

is there any way to actually inherit (not aggregate) from cats' typeclasses? I mean ApplicativeAsk[F[_], E] extends Applicative[F]

No, that would bring you the ambiguous implicit problem scato solves, since e.g. F[_]: ApplicativeAsk : ApplicativeLocal would have two Applicatives

@LukaJCB
Copy link
Member

LukaJCB commented Aug 29, 2018

We could use the instanceOf pattern for ApplicativeAsk and Applicative, though I'm sceptical that would buy us all too much.

@LukaJCB
Copy link
Member

LukaJCB commented Aug 29, 2018

@SystemFw, what do you think, should we let ApplicativeLocal extend ApplicativeAsk, FunctorListen extend FunctorTell and ApplicativeHandle extend FunctorRaise, and thereby get rid of the Basehierarchy?

@SystemFw
Copy link
Contributor

We can do that, but it kinda constrains to not add any more leaves right? Unsure if it's a problem or not, practically

@kamilkloch
Copy link
Contributor Author

F[_]: ApplicativeAsk : ApplicativeLocal would also fail if ApplicativeLocal would plainly extend ApplicativeAsk, as suggested by @LukaJCB . I was more thinking of using the hidden inheritance pattern from scalaz also for Applicative, for example

ApplicativeAskClass[F[_], E] extends ApplicativeClass[F]

(i.e., switch fully to the ineritance mode by adding some instanceOf plumbing, just an idea)

@LukaJCB
Copy link
Member

LukaJCB commented Aug 29, 2018

We can do that, but it kinda constrains to not add any more leaves right? Unsure if it's a problem or not, practically

@SystemFw if we do add more leaves, we can just scato for that leaf again.

@codecov-io
Copy link

codecov-io commented Aug 29, 2018

Codecov Report

Merging #52 into master will decrease coverage by 0.46%.
The diff coverage is 90.9%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #52      +/-   ##
==========================================
- Coverage   94.68%   94.22%   -0.47%     
==========================================
  Files          71       71              
  Lines         734      727       -7     
  Branches        5        3       -2     
==========================================
- Hits          695      685      -10     
- Misses         39       42       +3
Impacted Files Coverage Δ
...ore/src/main/scala/cats/mtl/ApplicativeLocal.scala 100% <ø> (ø) ⬆️
...ain/scala/cats/mtl/laws/ApplicativeLocalLaws.scala 100% <ø> (ø) ⬆️
.../main/scala/cats/mtl/hierarchy/BaseHierarchy.scala 100% <ø> (ø) ⬆️
.../src/main/scala/cats/mtl/instances/chronicle.scala 100% <ø> (ø) ⬆️
...ts/mtl/laws/discipline/ApplicativeLocalTests.scala 100% <ø> (+10%) ⬆️
core/src/main/scala/cats/mtl/instances/local.scala 90% <90.9%> (-1.18%) ⬇️
core/src/main/scala/cats/mtl/instances/ask.scala 0% <0%> (-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 af123f0...596f0f8. Read the comment docs.

@LukaJCB
Copy link
Member

LukaJCB commented Aug 29, 2018

F[]: ApplicativeAsk : ApplicativeLocal would also fail if ApplicativeLocal would plainly extend ApplicativeAsk, as suggested by @LukaJCB . I was more thinking of using the hidden inheritance pattern from scalaz also for Applicative, for example
ApplicativeAskClass[F[
], E] extends ApplicativeClass[F]
(i.e., switch fully to the ineritance mode by adding some instanceOf plumbing, just an idea)

I'm not sure if this buys us a lot, unless we provide an implicit conversion from e.g. ApplicativeAsk to Applicative. Because right now, cats-mtl has you write code like this:

def foo[F[_]: Monad: ApplicativeAsk[?[_], Config]: F[A] = ???

@LukaJCB
Copy link
Member

LukaJCB commented Aug 29, 2018

And what that means is that the only place that really calls ApplicativeAsk#applicative is the laws

@kamilkloch
Copy link
Contributor Author

I agree. For consideration, last commit is with plain inheritance.

@LukaJCB
Copy link
Member

LukaJCB commented Aug 29, 2018

Yes, I think it looks good this way 👍

@kamilkloch
Copy link
Contributor Author

If we are to proceed, shouldn't we do it in a new PR?

@LukaJCB
Copy link
Member

LukaJCB commented Aug 29, 2018

Sure Sounds good to me :)

@LukaJCB
Copy link
Member

LukaJCB commented Aug 30, 2018

I'll close this for now, we can always reopen if we reconsider :)

@LukaJCB LukaJCB closed this Aug 30, 2018
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.

None yet

4 participants