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

Make Sync extend Defer #296

Merged
merged 6 commits into from
Aug 15, 2018
Merged

Make Sync extend Defer #296

merged 6 commits into from
Aug 15, 2018

Conversation

Avasil
Copy link

@Avasil Avasil commented Aug 6, 2018

Fixes #286

@LukaJCB
Copy link
Member

LukaJCB commented Aug 6, 2018

Adding a new supertype to the hierarchy will very likely break binary compatability on 2.11, so be sure to add those MiMa exceptions :)

@alexandru
Copy link
Member

Should we make Bracket extend Defer?

@alexandru
Copy link
Member

No, actually that's a bad idea.

@Avasil
Copy link
Author

Avasil commented Aug 6, 2018

@LukaJCB Thanks, hopefully it will be ok now - is there a way to run it locally for all supported versions?

@codecov-io
Copy link

codecov-io commented Aug 6, 2018

Codecov Report

Merging #296 into master will decrease coverage by 0.15%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #296      +/-   ##
==========================================
- Coverage   86.97%   86.81%   -0.16%     
==========================================
  Files          64       64              
  Lines        1797     1798       +1     
  Branches      186      187       +1     
==========================================
- Hits         1563     1561       -2     
- Misses        234      237       +3

@LukaJCB
Copy link
Member

LukaJCB commented Aug 6, 2018

@Avasil, you can run + mimaReportBinaryIssues :)

@@ -25,8 +25,8 @@ import cats.syntax.all._
* A monad that can suspend the execution of side effects
* in the `F[_]` context.
*/
@typeclass
trait Sync[F[_]] extends Bracket[F, Throwable] {
@typeclass(excludeParents = List("Defer"))
Copy link
Member

Choose a reason for hiding this comment

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

Why is excludeParents necessary here?

Copy link
Author

Choose a reason for hiding this comment

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

Compiler was complaining with type AllOps is not a member of object cats.Defer, I've got fix from typelevel/simulacrum#38

@mpilquist mpilquist self-requested a review August 7, 2018 01:42
Copy link

@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.

Thanks for taking this.

* Alias for `suspend` that suspends the evaluation of
* an `F` reference and implements `cats.Defer` typeclass.
*/
override def defer[A](fa: => F[A]): F[A] = suspend(fa)
Copy link
Member

Choose a reason for hiding this comment

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

IMO we should make this a final, as we should never allow defer to not be an alias of suspend.

It also makes it easier for people implementing Sync when overriding methods for performance reasons, as defer will no longer be shown by the IDE.

Copy link
Author

Choose a reason for hiding this comment

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

That makes sense. 👍

@rossabaker
Copy link
Member

I hate to suggest another change as we're trying to stabilize, but given that it's a final alias, what if we retired the name suspend and just used defer? I imagine we could deprecate suspend now and remove it in 2.x.

@johnynek
Copy link

johnynek commented Aug 9, 2018

@rossabaker I don't like this idea... defer is about laziness, suspend talks about effects.

If we retire suspend we shouldn't tell people to put effects there, they should do so in delay.

I don't know why you should use suspend to do effects, but the comment mentions it. If we can remove that idea and cases in the code where we do it, I'm +1.

@mpilquist
Copy link
Member

mpilquist commented Aug 9, 2018

I'm in favor of deleting suspend entirely. It's a weird method, which let's you do some side effects in the creation of an F[A]. In the rare cases where that's actually what you want, F.delay(rareCase).flatten seems sufficient to me. This also underscores the "specialness" of delay as the FFI to side effecting code.

@alexandru
Copy link
Member

alexandru commented Aug 12, 2018

My 2¢, as I already expressed when we talked about the terminology of suspend:

  1. has been in use in Scalaz 7's Task, which has been the de facto IO monad for a long time
  2. has been in use in Scalaz's Free encoding since 2012 at least
  3. it's used in Cats' Free algebra as well

Keeping suspend is not the kind of hill I want to die on, if you think we should remove it that's fine, however as an anecdote if I look at my own code, I think I use suspend far more than I use delay. Two reasons for that:

  1. suspend can model recursions more naturally than flatMap, the same reason for why defer exists, except that with suspend you can do side effects too ... for example dealing with AtomicReference compare-and-set loops
  2. triggering errors in a way that doesn't involve throw

Also I think defer is useful, however consider that recursions often involve errors and even side effects. When we talk of pure expressions, we often disregard memory allocations, yet memory allocations are side effects. This is very visible for example in Haskell where people have sometimes used IO to suspend the allocation of memory, even if the suspended computation is pure. Or in Scala where we can use suspend to model pure recursions that would otherwise throw StackOverflowError. The fact that you can describe a pure expression that can throw a memory error points to the fact that such expressions are not pure at all.

In other words we can pretend that defer won't get used for suspending side effects, but the de facto use of it will be for suspending side effects. As from an implementation standpoint delay(thunk).flatten will always be equivalent with defer(thunk). There are no optimizations that you can make in Scala to make that equivalence false.

@alexandru
Copy link
Member

Btw, if you have a problem with the naming, just as we have async and asyncF, I guess we can rename them to sync and syncF and express one in terms of the other.

But that doesn't actually solve anything imo.

@SystemFw
Copy link
Contributor

This is very visible for example in Haskell where people have sometimes used IO to suspend the allocation of memory, even if the suspended computation is pure.

Do you have an example of this? Doesn't make sense to me (this is just for curiosity, not for the sake of arguing in this thread :) ). Computations are already all suspended in Haskell, IO makes them more strict actually.

Or in Scala where we can use suspend to model pure recursions that would otherwise throw StackOverflowError. The fact that you can describe a pure expression that can throw a memory error points to the fact that such expressions are not pure at all.

Well, I think it says more about the broken recursion model in Scala tbh.

In other words we can pretend that defer won't get used for suspending side effects, but the de facto use of it will be for suspending side effects.

I don't think this argument is that strong, it's the same thing with Eval, or with side effects in map.

@alexandru
Copy link
Member

@SystemFw

Do you have an example of this? Doesn't make sense to me (this is just for curiosity, not for the sake of arguing in this thread :) ). Computations are already all suspended in Haskell, IO makes them more strict actually.

Not all computations are suspended in Haskell, Haskell isn't actually lazy and its non-strict model can break in non-obvious ways.

Here's an example: https://twitter.com/pasiphae_goals/status/966730065345241089

Or in Scala where we can use suspend to model pure recursions that would otherwise throw StackOverflowError. The fact that you can describe a pure expression that can throw a memory error points to the fact that such expressions are not pure at all.

Well, I think it says more about the broken recursion model in Scala tbh.

No, I don't think you can blame this one on Scala.

In other words we can pretend that defer won't get used for suspending side effects, but the de facto use of it will be for suspending side effects.

I don't think this argument is that strong, it's the same thing with Eval, or with side effects in map.

For Eval at the very least you can point at its Comonad instance as an argument for why it cannot be used for side effects. IO has no such thing.

@SystemFw
Copy link
Contributor

SystemFw commented Aug 12, 2018

Not all computations are suspended in Haskell, Haskell isn't actually lazy and its non-strict model can break in non-obvious ways.

Haskell is non-strict which means you can't have a valid implementation of it that works like if it was strict (so, not suspended). The only time this should break in the way you mention in when doing FFI, and indeed the example you mention fits in that category, but that makes sense since the code you are evaluating is not haskell code and doesn't fit Haskell's execution model.

No, I don't think you can blame this one on Scala.

Not on Scala, no, that's harsh, but definitely on strict languages on the JVM. If your recursion model can't express infinite recursion, then it's broken.

For Eval at the very least you can point at its Comonad instance as an argument for why it cannot be used for side effects. IO has no such thing.

That's a good point :) But for example scalaz has pure being by-name, or cats.effect.IO suspends in map, and I still don't think we should encourage side-effects being done in them. Even though the way to delay effectful code in Scala is through call-by-name, I thought we had agreed that such a usage was a special prerogative of delay, like Michael mentions earlier.

@johnynek
Copy link

If we really want to keep effects in suspend I think the PR is good as is.

@alexandru
Copy link
Member

Guys, if you want to remove suspend to simplify the API, I’m fine with it, however our API for FFI isn’t minimal.

We have async and asyncF. We have bracket and cancelable. async and cancelable are pretty cool optimizations in the context of the JVM, even if they can be derived, for performance and because they have the signature that users expect.

@alexandru alexandru added this to the 1.0.0-RC3 milestone Aug 13, 2018
@alexandru alexandru merged commit 2705369 into typelevel:master Aug 15, 2018
@Avasil Avasil deleted the syncExtendsDefer branch September 8, 2018 09: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.

8 participants