FreeT stack-safe transformer #1266

raulraja · 2016-08-04T17:49:51Z

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 and Traverse are not included but possible. We need a way to derive foldRight and foldLeft from foldMap which I will contribute after this one so those instances are stack safe as well. I believe the same applies to Free which is currently lacking several instances.

An example that illustrating interleaving State is shown below and also available in the freemonads.tut FreeT section included as part of this PR.

import cats.free._
import cats._
import cats.data._

/* A base ADT for the user interaction without state semantics */
sealed abstract class Teletype[A] extends Product with Serializable
final case class WriteLine(line : String) extends Teletype[Unit]
final case class ReadLine(prompt : String) extends Teletype[String]

type TeletypeT[M[_], A] = FreeT[Teletype, M, A]
type Log = List[String]

/** Smart constructors, notice we are abstracting over any MonadState instance
 *  to potentially support other types beside State 
 */
class TeletypeOps[M[_]](implicit MS : MonadState[M, Log]) {
  def writeLine(line : String) : TeletypeT[M, Unit] =
    FreeT.liftF[Teletype, M, Unit](WriteLine(line))
  def readLine(prompt : String) : TeletypeT[M, String] =
    FreeT.liftF[Teletype, M, String](ReadLine(prompt))
  def log(s : String) : TeletypeT[M, Unit] =
    FreeT.liftT[Teletype, M, Unit](MS.modify(s :: _))
}

object TeletypeOps {
  implicit def teleTypeOpsInstance[M[_]](implicit MS : MonadState[M, Log]) : TeletypeOps[M] = new TeletypeOps
}

type TeletypeState[A] = State[List[String], A]

def program(implicit TO : TeletypeOps[TeletypeState]) : TeletypeT[TeletypeState, Unit] = {
  for {
    userSaid <- TO.readLine("what's up?!")
    _ <- TO.log(s"user said : $userSaid")
    _ <- TO.writeLine("thanks, see you soon!")
  } yield () 
}

def interpreter = new (Teletype ~> TeletypeState) {
  def apply[A](fa : Teletype[A]) : TeletypeState[A] = {  
    fa match {
      case ReadLine(prompt) =>
        println(prompt)
        val userInput = "hanging in here" //scala.io.StdIn.readLine()
        StateT.pure[Eval, List[String], A](userInput)
      case WriteLine(line) =>
        StateT.pure[Eval, List[String], A](println(line))
    }
  }
}

import TeletypeOps._

val state = program.foldMap(interpreter)
val initialState = Nil
val (stored, _) = state.run(initialState).value

Largely based on scalaz's FreeT and ideas from http://www.haskellforall.com/2012/07/free-monad-transformers.html

codecov-io · 2016-08-04T18:46:52Z

Current coverage is 90.65% (diff: 93.42%)

Merging #1266 into master will increase coverage by 0.05%

@@             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

Powered by Codecov. Last update 34360f8...c416127

travisbrown · 2016-08-04T19:23:16Z

free/src/main/scala/cats/free/FreeT.scala

+    * 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] = {


Why not just a MonadRec constraint?

Makes sense. Will change that there and in a couple of methods more.

johnynek · 2016-08-04T21:51:14Z

free/src/main/scala/cats/free/FreeT.scala

+          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 {


why are you not doing case GoSub(a, f) => here and preferring g @ GoSub(_, _)?

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

travisbrown · 2016-08-05T14:05:25Z

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 Free, so I think we should wait for two other maintainer sign-offs.

ceedubs · 2016-08-07T16:17:27Z

Would it make sense to do some more reification as constructors, similarly to what has been done with baking coyoneda into Free? For example, currently the pure method requires an Applicative[M] instance and ends up using the Suspend constructor. Should there be a Pure constructor and potentially SuspendS, SuspendM, etc? That seems like it would reduce some of the constraints of the initialization methods. It could also possibly lead to minor performance improvements, but probably not enough so to be a good reason to do it.

WDYT?

raulraja · 2016-08-08T05:20:24Z

@ceedubs I would agree with that but given you need a MonadRec[M] to do anything useful with FreeT and that is a harder constrain than Applicative not sure what the real benefit is since in most if not all cases you would already have those instances in scope. Fewer ADTs keeps the design easier to reason about IMHO. Having said that I'll be happy to change it and add those if it is what we usually follow in cats for other types. Let me know. thanks!

johnynek · 2016-08-09T04:16:26Z

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.

raulraja · 2016-08-09T04:47:51Z

@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.

johnynek · 2016-08-09T04:52:08Z

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.

raulraja · 2016-08-11T04:59:33Z

@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.

johnynek · 2016-08-11T05:36:06Z

free/src/main/scala/cats/free/FreeT.scala

+  @tailrec
+  private def step: FreeT[S, M, A] =
+    this match {
+      case g @ FlatMapped(_, _) => g.a match {


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 }

These are for the same reasons as #1266 (comment) Inference does not work when referring to an abstract type member.

scala!!!!!!!!!!

Okay.

raulraja · 2016-08-11T05:38:33Z

@johnynek @puffnfresh Added attribution based on the ones @ceedubs added in #1276

johnynek · 2016-08-11T05:45:35Z

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.

johnynek · 2016-08-11T17:49:28Z

👍

non · 2016-08-12T20:43:38Z

👍

johnynek · 2016-08-12T23:08:43Z

I think you need to merge master and make a couple of changes (MonadRec no longer exists). See Free.scala for the change.

ceedubs · 2016-08-13T13:15:40Z

free/src/main/scala/cats/free/FreeT.scala

+  * 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.
+  */


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.

ceedubs · 2016-08-13T13:17:11Z

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?

raulraja · 2016-08-18T17:13:16Z

free/src/test/scala/cats/free/FreeTTests.scala

+
+  implicit val listWrapperMonad: MonadRec[ListWrapper] with Alternative[ListWrapper] = ListWrapper.monadCombine
+
+  implicit val intStateMonad: MonadRec[IntState] = throw("can't summon this instance!")


@adelbertc @johnynek This is what I was mentioning in gitter. I can't find a way to summon the StateT Monad with RecursiveTailRecM instance

Nevermind, I think i may have found the issue

johnynek · 2016-08-18T18:36:59Z

free/src/main/scala/cats/free/FreeT.scala

+      def M = M0
+    }
+
+  implicit def catsFreeCombineForFreeT[S[_], M[_]: Applicative: RecursiveTailRecM: SemigroupK]: SemigroupK[FreeT[S, M, ?]] =


I don't think you need RecursiveTailRecM unless you also have FlatMap or Monad and I don't see that here.

johnynek · 2016-08-18T18:55:48Z

👍 looks good to me now. Thanks!

raulraja · 2016-08-19T23:03:10Z

@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!

johnynek · 2016-08-19T23:38:45Z

👍

adelbertc · 2016-08-19T23:59:41Z

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.

adelbertc · 2016-08-20T03:01:33Z

Hey @raulraja I just merged #1289 - doesn't cause any merge conflicts but moving forward Cats will be using Either so I made a PR to the branch which merges the latest stuff + changes usage to Either: https://github.com/47deg/cats/pull/1

EDIT 👍 on Travis green

Freet either

johnynek · 2016-08-20T05:35:24Z

👍

non · 2016-08-20T14:38:45Z

Looks good to me. 👍

raulraja added 5 commits July 20, 2016 01:12

A mens to derive folds from foldMap and FreeT + instances

dd3419a

FreeT Instances + Law Tests

43d08cb

Fixed typo on Kleisli Tests

80ac0d5

Basic docs for FreeT

5a616e8

validation style and doc changes

ee890fb

travisbrown reviewed Aug 4, 2016
View reviewed changes

raulraja added 2 commits August 4, 2016 22:23

Replace FlarMapRec+Applicative for MonadRec ev.

a4bfec8

Fixed TransLift instance and added missing tests

a9a86d3

johnynek reviewed Aug 4, 2016
View reviewed changes

Renamed Gosub to FlatMapped

9654b3d

travisbrown mentioned this pull request Aug 5, 2016

Draft of 0.7.0 release notes #1259

Merged

Added proper attribution

014ff40

johnynek reviewed Aug 11, 2016
View reviewed changes

Removed whitespace

11a11ae

ceedubs reviewed Aug 13, 2016
View reviewed changes

raulraja reviewed Aug 18, 2016
View reviewed changes

raulraja added 2 commits August 18, 2016 20:08

Fixed implicits for RecursiveTailRecM

a7e690f

Minor cleanup

1ebc15c

johnynek reviewed Aug 18, 2016
View reviewed changes

Addresses PR comments regarding use of RecursiveTailRecM

f4d8693

raulraja added 6 commits August 18, 2016 22:49

Merge remote-tracking branch 'upstream/master'

2c515dc

Rearranged instance to workaround implicit search compiler bug in 2.10.6

ce62750

More compiler workarounds non-sense for hangs on 2.11

16733ea

Simplified test instances

ff46b5b

Reverted ListWrapper to its original state

a15b267

Added explicit instances and extra tests

ba3c1d6

adelbertc mentioned this pull request Aug 19, 2016

Replace Xor with Either, fixes #1192 #1289

Merged

adelbertc added 2 commits August 19, 2016 19:49

Merge remote-tracking branch 'upstream/master' into 47deg/master

c7d8089

s/Xor/Either in FreeT

d75686d

Merge pull request #1 from adelbertc/freet-either

c416127

Freet either

travisbrown mentioned this pull request Aug 20, 2016

Revert Xor / Either changes for 0.7.0 release #1302

Closed

adelbertc merged commit 8301453 into typelevel:master Aug 20, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FreeT stack-safe transformer #1266

FreeT stack-safe transformer #1266

raulraja commented Aug 4, 2016

codecov-io commented Aug 4, 2016 •

edited

Loading

travisbrown Aug 4, 2016

raulraja Aug 4, 2016

johnynek Aug 4, 2016

raulraja Aug 4, 2016

travisbrown commented Aug 5, 2016

ceedubs commented Aug 7, 2016

raulraja commented Aug 8, 2016 •

edited

Loading

johnynek commented Aug 9, 2016

raulraja commented Aug 9, 2016

johnynek commented Aug 9, 2016

raulraja commented Aug 11, 2016

johnynek Aug 11, 2016

raulraja Aug 11, 2016

johnynek Aug 11, 2016

raulraja commented Aug 11, 2016

johnynek commented Aug 11, 2016

johnynek commented Aug 11, 2016

non commented Aug 12, 2016

johnynek commented Aug 12, 2016

ceedubs Aug 13, 2016

ceedubs commented Aug 13, 2016

raulraja Aug 18, 2016

raulraja Aug 18, 2016

johnynek Aug 18, 2016

johnynek commented Aug 18, 2016

raulraja commented Aug 19, 2016

johnynek commented Aug 19, 2016

adelbertc commented Aug 19, 2016

adelbertc commented Aug 20, 2016 •

edited

Loading

johnynek commented Aug 20, 2016

non commented Aug 20, 2016


		implicit val listWrapperMonad: MonadRec[ListWrapper] with Alternative[ListWrapper] = ListWrapper.monadCombine

		implicit val intStateMonad: MonadRec[IntState] = throw("can't summon this instance!")

FreeT stack-safe transformer #1266

FreeT stack-safe transformer #1266

Conversation

raulraja commented Aug 4, 2016

codecov-io commented Aug 4, 2016 • edited Loading

Current coverage is 90.65% (diff: 93.42%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travisbrown commented Aug 5, 2016

ceedubs commented Aug 7, 2016

raulraja commented Aug 8, 2016 • edited Loading

johnynek commented Aug 9, 2016

raulraja commented Aug 9, 2016

johnynek commented Aug 9, 2016

raulraja commented Aug 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raulraja commented Aug 11, 2016

johnynek commented Aug 11, 2016

johnynek commented Aug 11, 2016

non commented Aug 12, 2016

johnynek commented Aug 12, 2016

Choose a reason for hiding this comment

ceedubs commented Aug 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnynek commented Aug 18, 2016

raulraja commented Aug 19, 2016

johnynek commented Aug 19, 2016

adelbertc commented Aug 19, 2016

adelbertc commented Aug 20, 2016 • edited Loading

johnynek commented Aug 20, 2016

non commented Aug 20, 2016

codecov-io commented Aug 4, 2016 •

edited

Loading

raulraja commented Aug 8, 2016 •

edited

Loading

adelbertc commented Aug 20, 2016 •

edited

Loading