Remove RecursiveTailRecM, move stack safety checks into MonadLaws - f…

[adelbertc]

…ixes #1278 and fixes #1283

cc @johnynek @non

[adelbertc]

Hm it seems it fails for Future in the JS tests since you need to Await on the Future to test for equality, but that gets tricky in JS land.. is there any way around this?

[guersam]

Should foldMapUnsafe be deprecated then?

[johnynek]

this should just be removed if you ask me. I doubt anyone is using it and removing RecursiveTailRecM is at least as big of an issue.

[kailuowang]

Should the unsafe defaultTailrecM be removed?
https://github.com/typelevel/cats/blob/master/core/src/main/scala/cats/Monad.scala#L24

[non]

@adelbertc in the worst-case you can make the test platform-specific using if (Platform.isJvm) .... This might be a good candidate for that.

I know ScalaTest supports a style of asynchronous testing that works with futures in JS, so you could also look into that.

[adelbertc]

@non I looked into the async testing support in ScalaTest but it seems like due to how our tests are constructed it's not really compatible? My understanding is the new async stuff lets you give ScalaTest a Future[Assertion] instead of just an Assertion, but our laws use Eq which want Boolean ?

[non]

Looks like the build failed because some of our .tut files need updating.

@adelbertc Understood. It seems like it would be a big undertaking to try to change our test code to be able to take advantage of that feature. What you have here seems good for now.

[codecov-io]

Current coverage is 91.72% (diff: 100%)

Merging #1370 into master will increase coverage by <.01%

@@             master      #1370   diff @@
==========================================
  Files           239        238     -1   
  Lines          3611       3600    -11   
  Methods        3484       3474    -10   
  Messages          0          0          
  Branches        126        125     -1   
==========================================
- Hits           3312       3302    -10   
+ Misses          299        298     -1   
  Partials          0          0          

Powered by Codecov. Last update 155f7f5...5a5ebb9

[johnynek]

Of course, I'm +1. I think asking for stack safety (given it seems always achievable) is fine.

I do know of a Monad that was not made stack safe (yet?) Enumeratee, I think it was in iteratee-io (@travisbrown). I have not looked too hard at making it stack safe.

One issue is that the worst case may require trampolines and some library authors might not like that. I wonder if we could devise a trampoline-like approach that only impacts tailRecM and does not hurt the performance of flatMap.

[adelbertc]

@johnynek The more I think of it the more convinced I am tailRecM should be enforced and monadic recursion in Cats should be stack safe (assuming callers implement lawful instances). I think I am OK worst case having users go through a trampoline.

I am curious about that data type you talked about in @travisbrown 's iteratee lib though.

@kailuowang @non Do I have a 👍 from either of you? :D

[travisbrown]

@johnynek @adelbertc io.iteratee.Enumerator has a monad instance but the tailRecM is not (currently) stack-safe. The approach that Cats uses in e.g. the instance for Vector should work, but it was non-trivial and recursive binding for enumerators isn't a use case I've ever run into, so I passed on it in the 0.6 release.

[kailuowang]

This comment mentioning the defaultTailRecM impl should be removed
https://github.com/typelevel/cats/blob/master/core/src/main/scala/cats/FlatMap.scala#L94-L108

[kailuowang]

👍