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

WIP: add some missing applicative and monad ops #1216

Closed
wants to merge 3 commits into from

Conversation

tpolecat
Copy link
Member

This pulls in some combinators from scalaz that never made it into cats. These are very useful when working with IO-like data types. I need some help with style and conventions before I put more time into this:

  • verify that the syntax approach is correct (class-per-operation).
  • verify that naming is sensible.
  • verify calling conventions (strictness and currying).

Also:

  • add tests.
  • update tut doc.

👎 pending (at least) items above.

@@ -38,6 +38,14 @@ import simulacrum.typeclass
def replicateA[A](n: Int, fa: F[A]): F[List[A]] =
sequence(List.fill(n)(fa))

/** Returns the given argument if `cond` is `false`, otherwise, unit lifted into F. */
def unlessA[A](cond: Boolean)(f: => F[A]): F[Unit] =
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the story on call-by-name vs Eval[F[A]]. Seems like the later is more the cats style (maybe have both, strict and Eval)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok I'll change those to strict + Eval. Honestly I have never had a case where it mattered, but scalaz does it this way so it's probably worthwhile to have the nonstrict version.

@peterneyens
Copy link
Collaborator

I think the FlatMap syntax has three *Ops classes because there are F[A], F[F[A]] and F[Boolean] (for respectively flatMap, flatten and ifM).

All the syntax methods on the same type (like most of them seemed to be on F[A] here), can be in the same *Ops class I think ?

@tpolecat
Copy link
Member Author

@peterneyens ok, if it's just based on shape that simplifies things. I'll do that.

@ceedubs
Copy link
Contributor

ceedubs commented Jul 18, 2016

@tpolecat as a follow-up to @peterneyens's comment, for the common cases, Simulacrum will generate the Ops for you. I think you got unlucky with Applicative and Monad being two exceptions where for some reason we currently aren't using simulacrum Ops. See the Apply syntax for an example of what I'm talking about.

@johnynek
Copy link
Contributor

It looks like many of these really want MonadRec don't they? You are usually doing a recursive flatMap. Can we rewrite them in terms of tailRecM?

@codecov-io
Copy link

Current coverage is 88.59%

Merging #1216 into master will decrease coverage by 0.59%

@@             master      #1216   diff @@
==========================================
  Files           234        235     +1   
  Lines          3144       3165    +21   
  Methods        3085       3110    +25   
  Messages          0          0          
  Branches         57         52     -5   
==========================================
  Hits           2804       2804          
- Misses          340        361    +21   
  Partials          0          0          

Sunburst

Powered by Codecov. Last updated by 885acf6...0048b99

@tpolecat
Copy link
Member Author

@johnynek possibly re: MonadRec ... I haven't used it.

Thanks all for your comments, I'll rework a few things tonight. 🐔

@adelbertc
Copy link
Contributor

Re: MonadRec, it seems all of the ops introduced don't necessarily recurse infinitely unlike say, forever. I can see a strong case for requiring MonadRec on forever where stack safety is paramount, but for stuff like whileM I think the argument is weaker.

All of my reservations however lie in the assumption that there are data types with Monad instances where you would want to use whileM and co., but no MonadRec.. is there such a case?

@ceedubs
Copy link
Contributor

ceedubs commented Jul 19, 2016

@adelbertc I've wondered the same about instances with Monad but no MonadRec. One argument for the non-Rec version is that when stack safety isn't a concern it's more performant. Whether that's worth the trouble of having two versions of methods and risking the stack blowing up when your app changes is a different question, though.

In #1117 (which by the way it would be great if we could get another reviewer or two on there), the existing foldLeftM is stack-safe but requires always traversing the entire structure. @TomasMikula and I went with keeping the existing foldLeftM but providing a separate foldLeftMRec which has the performance overhead (and additional constraint) of MonadRec but also allows for short-circuiting. I think in some cases it may be handy to have both Monad and MonadRec versions of methods.

@tpolecat
Copy link
Member Author

So do I need 4 versions of every method, with/without Eval and with/without MonadRec? That's starting to seem kind of nutty.

@adelbertc
Copy link
Contributor

I would just not have MonadRec

On Jul 19, 2016 12:50, "Rob Norris" notifications@github.com wrote:

So do I need 4 versions of every method, with/without Eval and
with/without MonadRec? That's starting to seem kind of nutty.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1216 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABRW9I1cx0nEf8Q0ugQePVdxqgtIkA5uks5qXSqUgaJpZM4JPC9n
.

@TomasMikula
Copy link
Contributor

Seems like the methods (whileM, untilM, ...) can stay in Monad as they are and be overridden in MonadRec using tailRecM?

@johnynek
Copy link
Contributor

johnynek commented Aug 4, 2016

So, I would actually argue that any time we do a recursive flatMap we should ONLY use FlatMapRec/MonadRec, and all the whileM untilM should go in MonadRec not Monad.

Then, we could have something like:

  def toRecursiveUnsafe[M[_]](m: Monad[M]): MonadRec[M] = // just implements tailRecM with a recursive flatMap otherwise proxy
  def toRecursiveUnsafe[M[_]](m: FlatMap[M]): FlatMapRec[M] = // just implements tailRecM with a recursive flatMap, otherwise proxy.

I think it is simpler to have this rule than to maybe blow the stack unless we have the right thing. Let the caller explicitly opt in to the risk.

In fact, many Monads have MonadRec (collections, Try, Future, Free, Eval,....) so I think this probably the right move.

If we don't think this is needed, why not put tailRecM on Monad and sometimes override it in MonadRec?

It may be a bit extreme, but to me this is the whole point of having MonadRec.

@johnynek
Copy link
Contributor

here is a PR to remove FlatMapRec so we don't have this duplication of methods to get the nice IO methods you want. I think everything can just implement tailRecM: #1280

@tpolecat
Copy link
Member Author

I'll come back to this at some point. No time now.

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