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 Free.foldMap stack-safe. #1075

Merged
merged 1 commit into from
May 31, 2016

Conversation

TomasMikula
Copy link
Contributor

@TomasMikula TomasMikula commented May 31, 2016

Fixes #721.

The fix is now possible by strengthening constraints on the target monad from Monad to MonadRec.

This is not binary compatible, and will not be source compatible for users that used this with a non-MonadRec monad.

@TomasMikula
Copy link
Contributor Author

To fix the tutorial, it will require at least MonadRec instance for State.

Quite ironically, the tutorial emphasizes stack-safety,

An important aspect of foldMap is its stack-safety. It evaluates
each step of computation on the stack then unstack and restart. This
process is known as trampolining.

As long as your natural transformation is stack-safe, foldMap will
never overflow your stack. Trampolining is heap-intensive but
stack-safety provides the reliability required to use Free[_] for
data-intensive tasks, as well as infinite processes such as streams.

but this has been wrong until this PR.

@non
Copy link
Contributor

non commented May 31, 2016

This looks great! Do you mind adding the necessary MonadRec instance? If not, I can take a crack at it in a different PR.

@TomasMikula
Copy link
Contributor Author

I can give it a go.

@TomasMikula
Copy link
Contributor Author

Turns out also MonadRec[Eval] is required, due to the way State is defined. See #1076.

@non
Copy link
Contributor

non commented May 31, 2016

I'm re-running Travis -- I think after your last PR this should be green now.

@non
Copy link
Contributor

non commented May 31, 2016

Looks like maybe there are conflicts -- but once this is green and ready-to-merge 👍 from me.

@codecov-io
Copy link

codecov-io commented May 31, 2016

Current coverage is 88.34%

Merging #1075 into master will decrease coverage by <.01%

  1. 3 files (not in diff) in .../src/main/scala/cats were deleted. more
  2. 3 files (not in diff) in ...main/scala/cats/data were modified. more
    • Misses -1
  3. 3 files (not in diff) in .../src/main/scala/cats were modified. more
    • Misses +1
    • Hits -1
  4. File ...Transformation.scala (not in diff) was created. more
@@             master      #1075   diff @@
==========================================
  Files           223        221     -2   
  Lines          2808       2804     -4   
  Methods        2751       2749     -2   
  Messages          0          0          
  Branches         52         50     -2   
==========================================
- Hits           2481       2477     -4   
  Misses          327        327          
  Partials          0          0          

Sunburst

Powered by Codecov. Last updated by 04b2501...5dc6518

@milessabin
Copy link
Member

👍

@milessabin
Copy link
Member

Needs rebasing ...

Fixes typelevel#721.

The fix is possible by strengthening constraints on the target monad
from Monad to MonadRec.
@TomasMikula TomasMikula force-pushed the free-foldmap-stack-safe branch from 5dc6518 to 07de292 Compare May 31, 2016 18:01
@TomasMikula
Copy link
Contributor Author

rebased

@ceedubs
Copy link
Contributor

ceedubs commented May 31, 2016

Thanks @TomasMikula!

@ceedubs ceedubs merged commit 2fb4d1d into typelevel:master May 31, 2016
@TomasMikula TomasMikula deleted the free-foldmap-stack-safe branch May 31, 2016 19:35
@mandubian
Copy link
Contributor

cool I had totallly missed this one :D

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.

Free.foldMap is not stack safe
6 participants