Some clean up for Free (and friends). #1085
Conversation
This commit does a bunch of small things: * Introduces `foldLeftM` and `runTailRec` * Renames `mapSuspension` to `compile` * Removes `Applicative[F]` requirement from `Free.suspend` * Adds some new comments * Moves the Free companion to the end of the file * Minor formatting changes I think all of these are good, but I'm happy to revert any of them that seem like they are bad news. The main reason I removed `mapSuspension` is that its description was written using "compile" which seems like a more evocative name (there are already several "map" variants).
Current coverage is 87.96%
@@ master #1085 diff @@
==========================================
Files 224 224
Lines 2844 2848 +4
Methods 2787 2791 +4
Messages 0 0
Branches 52 52
==========================================
- Hits 2507 2505 -2
- Misses 337 343 +6
Partials 0 0
|
|
👍 really good move! |
I don't like the Eval.value calls, but I don't know of a way around them. The unit tests suggest that they aren't a problem due to the nature of `Free.runTailRec` and `FlatMapRec.tailRecM`.
Add a short-circuiting implementation of foldLeftM
|
@ceedubs Thanks! Great job. |
|
I'm thinking that we'll probably want to move |
|
@mandubian would you mind taking another look now that my change has been included in this PR? |
…to topic/free-additions-cleanup
|
OK, this should be good to merge once it gets sign-offs. |
|
@ceedubs code is good to me... Just need to think a bit more about this foldLeftM that is based on a foldRight and that becomes safe due to Free + MonadRec... |
|
👍 from me |
| def foldLeftM[F[_]: Foldable, G[_]: MonadRec, A, B](fa: F[A], z: B)(f: (B, A) => G[B]): G[B] = | ||
| unsafeFoldLeftM[F, Free[G, ?], A, B](fa, z) { (b, a) => | ||
| Free.liftF(f(b, a)) | ||
| }.runTailRec |
There was a problem hiding this comment.
Did I miss it, or does this method not use the G.tailRecM method at all?
Also, it seems like there is double trampolining (once via Free and once via Eval in F.foldRight)? I wonder if a more direct implementation (using either Free or Eval, but not both) is possible.
There was a problem hiding this comment.
@TomasMikula as the author of this code, I'll be the first to say that it is a bit bizarre. G.tailRecM should be being called by the runTailRec call on the Free structure. Eval makes an appearance due to the foldRight type signature, but really we are just using foldRight because of its laziness, and unsafeFoldLeftM is calling .value on the Eval, so we don't actually get any Eval trampolining here. It's effectively like we are using a scalaz-style foldRight that takes a byname parameter, since we aren't taking advantage Eval's trampolining.
If you can figure out a way to provide a more direct implementation, I'd definitely be interested. I think that this has to be done in terms of foldRight if we want laziness, though, which means Eval will definitely be in play. You may be able to use MonadRec and foldRight without Free, and if so that'd be great - as it stands I'm kind of convinced that we should move Free (the monad but not the other free structures) back into core for this sort of thing.
There was a problem hiding this comment.
One other note: it really isn't obvious to me that this implementation should be stack safe, but the tests kind of prove that it is ¯\_(ツ)_/¯
There was a problem hiding this comment.
somehow I completely missed that .runTailRec at the end 😊
|
I'm going to go ahead and merge, because this has some really good changes in it. @TomasMikula please feel free to experiment with |
|
I might try to experiment to avoid the use of |
|
@ceedubs so, I was not even trying to move the computation to the type level, but I just managed to cause a stack overflow in the compiler 😄 |
|
See #1117. |
This commit does a bunch of small things:
foldLeftMandrunTailRecmapSuspensiontocompileApplicative[F]requirement fromFree.suspendI think all of these are good, but I'm happy to revert
any of them that seem like they are bad news. The main
reason I removed
mapSuspensionis that its descriptionwas written using "compile" which seems like a more
evocative name (there are already several "map" variants).