-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
FreeT stack-safe transformer #1266
Conversation
Current coverage is 90.65% (diff: 93.42%)@@ master #1266 diff @@
==========================================
Files 235 236 +1
Lines 3606 3682 +76
Methods 3546 3622 +76
Messages 0 0
Branches 56 56
==========================================
+ Hits 3267 3338 +71
- Misses 339 344 +5
Partials 0 0
|
* Runs to completion, mapping the suspension with the given transformation | ||
* at each step and accumulating into the monad `M`. | ||
*/ | ||
def foldMap(f: S ~> M)(implicit M0: FlatMapRec[M], M1: Applicative[M]): M[A] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just a MonadRec
constraint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Will change that there and in a couple of methods more.
case Xor.Left(a) => MR.pure(Xor.Right(a)) | ||
case Xor.Right(sa) => MR.map(f(sa))(Xor.right) | ||
} | ||
case g @ Gosub(_, _) => g.a match { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are you not doing case GoSub(a, f) =>
here and preferring g @ GoSub(_, _)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The abstract type A
in GoSub
in which both a
and f
depend seem to cause inference to not work properly unless the whole thing is named :P.
This technique is also used in scalaz. Open to changes if anyone thinks there is a better way.
scalaz/scalaz#730
This looks good to me now, but I'm not really up-to-date on the details of the implementation here or how it lines up with the current |
Would it make sense to do some more reification as constructors, similarly to what has been done with baking coyoneda into WDYT? |
@ceedubs I would agree with that but given you need a |
in light of #1275 and the fact that this is based on scalaz's implementation, if there are significant portions of copy-paste here (I didn't check) can we git-blame the original file and add a comment to that effect? Also, it might be nice to link to a particular git-sha from which this was based. |
@johnynek out of a computer until tomorrow. I hope the tweet was not originated from this PR I clearly stated that it was based on the Scalaz impl in the PR description and at no time I wanted to discredit anyone for their work. Is the proper way to attribute them in the file header? I didn't feel that mentioning people there would be appropriate without their explicit permission. I'll run the git blame in the AM when I'm close to a computer. Where do those name belong since they are neither cats authors nor contributors? I thought that most of cats was based on the Scalaz design and mentioning the library in the PR was enough. Apologies if I skipped any protocol I was not aware of. |
I don't think you need to apologize. I am not 100% sure what has motivated all of it, and I guess history had some role to play, far beyond any one 1 PR. That said, I think no one can object to adding a link to the code it was based on an mentioning the authors in a comment. Others can voice their opinion. |
@johnynek Given the current status of affairs, what should I do to get this PR to comply with how you guys want to proceed with attributions?. It seems Brian has already been added to Authors and not sure if anything extra should be sone as part of this PR. Looking for directions to push this forward. |
@tailrec | ||
private def step: FreeT[S, M, A] = | ||
this match { | ||
case g @ FlatMapped(_, _) => g.a match { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the rationale for g @ FlatMapped(_, _) => g.a
rather than FlatMapped(a, _) =>
same below.
Also, why not:
this match {
case FlatMapped(FlatMapped(a, fn0), fn1) =>
a.flatMap(a => fn0(a).flatMap(fn1)).step
case nonNestedFlatMap => nonNestedFlatMap
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are for the same reasons as #1266 (comment) Inference does not work when referring to an abstract type member.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scala!!!!!!!!!!
Okay.
@johnynek @puffnfresh Added attribution based on the ones @ceedubs added in #1276 |
just a minor comment on the style of pattern matching (which I'm not personally crazy about), but otherwise this looks good to me. Thanks for porting this. I could have personally used it earlier in the week. |
👍 |
1 similar comment
👍 |
I think you need to merge master and make a couple of changes (MonadRec no longer exists). See |
* This Scala implementation of `FreeT` and its usages are derived from | ||
* [[https://github.com/scalaz/scalaz/blob/series/7.3.x/core/src/main/scala/scalaz/FreeT.scala Scalaz's FreeT]], | ||
* originally written by Brian McKenna. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might also be good to link to Stack Safety for Free here. Both as attribution to Phil Freeman and because this is the paper that inspired FreeT
arriving in Scala.
Thanks, @raulraja! This is looking great. I think that this might suffer from the toString issue that was fixed on Free. If so, shall we take the same approach we did there? |
|
||
implicit val listWrapperMonad: MonadRec[ListWrapper] with Alternative[ListWrapper] = ListWrapper.monadCombine | ||
|
||
implicit val intStateMonad: MonadRec[IntState] = throw("can't summon this instance!") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adelbertc @johnynek This is what I was mentioning in gitter. I can't find a way to summon the StateT
Monad with RecursiveTailRecM
instance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, I think i may have found the issue
def M = M0 | ||
} | ||
|
||
implicit def catsFreeCombineForFreeT[S[_], M[_]: Applicative: RecursiveTailRecM: SemigroupK]: SemigroupK[FreeT[S, M, ?]] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need RecursiveTailRecM
unless you also have FlatMap
or Monad
and I don't see that here.
👍 looks good to me now. Thanks! |
@johnynek @adelbertc @travisbrown @non @ceedubs I think this is ready. Let me know if there is anything extra you'd like me to add or change. Thanks! |
👍 |
LGTM 👍 Given the size the of the change and the number of changes since @non 's previous 👍 we should let him (or another maintainer) OK it before merging. |
Hey @raulraja I just merged #1289 - doesn't cause any merge conflicts but moving forward Cats will be using EDIT 👍 on Travis green |
Freet either
👍 |
Looks good to me. 👍 |
FreeT
is a stack safe monad transfer that allows interleaving effects and other monads along with the process of building a Free AST. Fixes #1158@ceedubs @adelbertc @non Can one of you review? Thanks for your consideration.
Includes instances for
Instances for
Foldable
andTraverse
are not included but possible. We need a way to derivefoldRight
andfoldLeft
fromfoldMap
which I will contribute after this one so those instances are stack safe as well. I believe the same applies toFree
which is currently lacking several instances.An example that illustrating interleaving
State
is shown below and also available in thefreemonads.tut
FreeT section included as part of this PR.Largely based on scalaz's FreeT and ideas from http://www.haskellforall.com/2012/07/free-monad-transformers.html