Add Foldable and Traversable instances for Free #1840

aaronlevin · 2017-08-21T22:01:54Z

Addresses #1833 .

aaronlevin · 2017-08-22T06:55:52Z

Hmm, the JS build failed. I'll look into this.

peterneyens

It seems the build is failing because scalastyle is requiring some additional spaces.

peterneyens · 2017-08-22T09:33:16Z

free/src/main/scala/cats/free/Free.scala

+    fa.fold(f(_, lb), F.foldRight(_,lb)( (freeA,eb) => foldRight(freeA,eb)(f)))
+}
+
+private trait FreeTraverse[F[_]] extends Traverse[Free[F,?]] with FreeFoldable[F] {


You can override map here to use Free#map which is more efficient than the default map for Traverse (traverse with Id).

Good point, I'll do that!

peterneyens · 2017-08-22T09:35:46Z

free/src/main/scala/cats/free/Free.scala

+      def FunctorF = implicitly
+    }
+
+  implicit def catsTraverseForFree[F[_] : Functor : Traverse]: Traverse[Free[F, ?]] =


Traverse is a Functor as well, so you can drop the Functor requirement.

I think we generally prefer "explicit implicits" (is that a thing?), compared to context bounds.

Hah, I was copying the style from Cofree, but I'm happy to add explicit implicits.

peterneyens · 2017-08-22T09:37:07Z

free/src/main/scala/cats/free/Free.scala

+  implicit def catsTraverseForFree[F[_] : Functor : Traverse]: Traverse[Free[F, ?]] =
+    new FreeTraverse[F] {
+      def F = implicitly
+      def FunctorF = implicitly


These defs can be vals.

aaronlevin · 2017-08-22T10:27:42Z

One question I had regarded which Foldable[F] instance to use in Traverse[Free[F,?]]. I'm assuming this must be the Foldable[F] which corresponds to the Traverse[F]. I tried to check the Traverse laws but didn't find anything online stating how Traverse relates to Foldable.

If it's not the case that the Foldable[F] used in Traverse[Free[F,?]] does not need to correspond to the Traverse[F] instance, then I think the selection should be pushed down to the catsTraverseForFree (effectively adding a third implicit for Foldable[F]).

But my gut tells me how its implemented now is correct (fwiw both styles pass the tests).

peterneyens · 2017-08-22T10:37:42Z

free/src/main/scala/cats/free/Free.scala

+
+sealed private[free] abstract class FreeInstances {
+
+  implicit def catsFoldableForFree[F[_]](


It is not really documented outside issue #1061 itself (it should probably be added to these guidelines): the implicit values in Cats are prefixed with the package.

So these implicits would be catsFreeFoldableForFree and catsFreeTraverseForFree.

cool. Thanks, I've updated.

I also created an issue for this: #1841

peterneyens · 2017-08-22T10:37:49Z

free/src/main/scala/cats/free/Free.scala

+  ): Traverse[Free[F, ?]] =
+    new FreeTraverse[F] {
+      val TraversableF = traversableF
+      val FunctorF = functorF


You might have missed my previous comment, but Traverse[F] is also a Functor[F].
So you could write :

implicit def catsTraverseForFree[F[_]](implicit F: Traverse[F]): Traverse[Free[F, ?]] = new FreeTraverse[F] { val TraversableF = F val FunctorF = F }

Ah, yes, I did miss that. Thanks!

peterneyens · 2017-08-22T10:45:44Z

Generally if a datatype implements multiple typeclasses in a hierarchy, the the implicits are overridden by a more powerful instance.

For example in OptionT :

Foldable and Traverse.
Eq, PartialOrder and Order.

So the Foldable[F] instance used in Traverse[Free[F,?]] is indeed the Traverse[F] instance.

aaronlevin · 2017-08-22T11:33:54Z

FYI, I'll squash and rebase when the tests pass and I've incorporated all your feedback. Thanks again, @peterneyens !!

codecov-io · 2017-08-22T12:20:45Z

Codecov Report

Merging #1840 into master will increase coverage by <.01%.
The diff coverage is 95%.

@@            Coverage Diff             @@
##           master    #1840      +/-   ##
==========================================
+ Coverage   94.96%   94.96%   +<.01%     
==========================================
  Files         241      241              
  Lines        4173     4193      +20     
  Branches      106      109       +3     
==========================================
+ Hits         3963     3982      +19     
- Misses        210      211       +1

Impacted Files	Coverage Δ
free/src/main/scala/cats/free/Free.scala	`91.04% <95%> (+1.68%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8458b24...3cdb62d. Read the comment docs.

aaronlevin · 2017-08-22T12:22:43Z

The tests passed, so I've squashed. Should be ready to merge.

peterneyens · 2017-08-22T14:16:36Z

Given that these methods are recursive, we should probably use resume and pattern match instead of fold to reduce some stack space.

I think we can make foldRight stack safe as well :

fa.resume match {
  case Left(fFreeA) =>
    F.foldRight(fFreeA, lb)( (freeA, eb) => 
      eb.flatMap(b => foldRight(freeA, Eval.now(b))(f))
    )
  case Right(a) => f(a, lb)
}

peterneyens · 2017-08-22T14:24:05Z

Hmm, it seems Foldable[Free[F, ?]] as it stands also requires Functor[F] as it uses fold (or resume), which means that there is only a Foldable instance for Free given a Traverse[F] (Foldable + Functor).

It seems that Scalaz uses a foldStep method which uses the underlying representation of Free (which doesn't need a Functor[F] in Free.liftF) to support Foldable[Free[F, ?]] for just Foldable[F].

So either we have to add something similar to foldStep or we only provide the Traverse instance.

aaronlevin · 2017-08-22T15:16:18Z

@peterneyens ok, I'll add foldStep and see about implementing Foldable without Functor (I double-checked the Haskell implementation and indeed the Functor[F] constraint is not present).

aaronlevin · 2017-08-22T15:16:35Z

(I'll also see about making foldRight stack-safe)

aaronlevin · 2017-08-22T16:03:29Z

@peterneyens OK, so I've implemented foldStep and removed the Functor constraint on Foldable[Free[F,?]]. My one issue is I don't really know how / what to test for foldStep. It's kind of a weird function.

peterneyens · 2017-08-22T17:02:50Z

I think we probably want to hide foldStep as it would expose the actual implementation of Free compared to the one provided by resume / fold / ...

If we hide foldStep I don't think it needs tests, step also doesn't have tests itself.

As the name suggest foldStep is a combination of step and a fold and I think it could have been implemented using step as well :

this.step match {
  case Pure(a) => onPure(a)
  case Suspend(a) => onSuspend(a)
  case FlatMapped(Suspend(fa), f) => onFlatMapped((fa, f))
  case _ => sys.error("FlatMapped should be right associative after step") // :(
}

aaronlevin · 2017-08-22T18:21:05Z

@peterneyens ok, I've implemented foldStep in terms of step as you suggested. I've also rebased and (I hope) fixed all the build issues.

peterneyens · 2017-08-22T18:27:00Z

Not sure if you know, but you can run scalastyle locally as well in sbt using scalastyle.

aaronlevin · 2017-08-22T18:57:42Z

ah, hah. I was doing that before, but I wasn't prefixing it with the project (e.g. scalastyle vs freeJVM/scalastyle) and I was confused because the errors in travis were happening on the JS project. anyway, got it now and that will be helpful for sure!

everything should be good now.

peterneyens

Thanks already @aaronlevin, this looks good.

I left two more comments.

peterneyens · 2017-08-22T21:45:20Z

free/src/main/scala/cats/free/Free.scala

@@ -55,6 +55,20 @@ sealed abstract class Free[S[_], A] extends Product with Serializable {
  }

  /**
+   * A combination of step and fold.
+   */
+  final def foldStep[B](


I think we need to make this private[free] to not expose the actual representation of Free compared to Either[F[Free[F, A]]], A] which is exposed by resume and fold.

peterneyens · 2017-08-22T22:01:27Z

free/src/main/scala/cats/free/Free.scala

+    fa.foldStep(
+      a => f(a, lb),
+      fa => F.foldRight(fa, lb)(f),
+      { case (fx, g) => F.foldRight(fx, lb)( (a, lbb) => foldRight(g(a), lbb)(f)) }


I think that lbb.flatMap(bb => foldRight(g(a), Eval.now(bb))(f)) instead of foldRight(g(a), lbb)(f) should make this stack safe.

We should probably add a test to check this. Something like (haven't compiled this):

val n = 50000 val freeOption: Int => Free[Option, Int] = Free.pure(x) val free = (0 to n).foldLeft(freeOption(0))((r, _) => r.flatMap(n => freeOption(n + 1))) free.foldRight(0)((a, lb) => lb.map(_ + a)) should (n)

i've added the test!

aaronlevin · 2017-08-23T10:21:32Z

@peterneyens I'm not savvy about understanding Free's stack usage, but two comments about that test:

as the test is written, both my definition and your definition pass (i.e. I did not observe the stack overflow)
I also used the same test as above to test foldLeft and it was also stack safe.

Is this expected.

peterneyens · 2017-08-23T11:16:56Z

I am sorry, it seems my comments about foldRight don't apply any more after you introduced foldStep.

Feel free to revert back to your original foldRight implementation. Maybe you can adapt the foldRight stack safety test to check foldLeft as well?

aaronlevin · 2017-08-23T12:50:14Z

@peterneyens ok, I've updated and added the foldLeft tests. I'm not quite sure if the tests actually test the stack safety as I've not yet seen a failing case, but I can try to find the old, pre-foldStep implementation and check that it breaks the stack.

aaronlevin · 2017-08-23T14:50:20Z

@peterneyens I just tried with the old implementations of foldLeft and foldRight using Free.fold (and adding the Functor[F] constraint), and the stacks did not explode.

So I'm not convinced that example is actually testing the stack. I'm also unsure if there is a problem with the stacks. Perhaps the implementation of foldLeft and foldRight are ok?

peterneyens · 2017-08-23T16:40:33Z

It seems you are right, not sure what I tried yesterday which let to a stack overflow. Sorry for the confusion.

peterneyens · 2017-08-23T16:51:39Z

free/src/main/scala/cats/free/Free.scala

+  ): Traverse[Free[F, ?]] =
+    new FreeTraverse[F] {
+      val TraversableF = traversableF
+      val FunctorF = traversableF


This val FunctorF isn't used any more right?

aaronlevin · 2017-08-25T20:02:02Z

@peterneyens ok, I've just squashed everything. so I think this is good. Who can merge this?

peterneyens

Thanks @aaronlevin!

johnynek

👍

Thanks for pushing this through!

peterneyens reviewed Aug 22, 2017

View reviewed changes

aaronlevin force-pushed the traverseable-free branch from 3c1d91c to 98879bd Compare August 22, 2017 12:22

aaronlevin force-pushed the traverseable-free branch from f167d9a to 429da1f Compare August 22, 2017 18:20

aaronlevin force-pushed the traverseable-free branch from 429da1f to c913765 Compare August 22, 2017 18:56

peterneyens reviewed Aug 22, 2017

View reviewed changes

aaronlevin force-pushed the traverseable-free branch from c913765 to 360025f Compare August 23, 2017 10:18

aaronlevin force-pushed the traverseable-free branch from 360025f to 2eb3099 Compare August 23, 2017 12:49

peterneyens reviewed Aug 23, 2017

View reviewed changes

Add Foldable and Traversable instances for Free

3cdb62d

aaronlevin force-pushed the traverseable-free branch from e92ce5f to 3cdb62d Compare August 25, 2017 20:01

peterneyens approved these changes Aug 26, 2017

View reviewed changes

johnynek reviewed Aug 26, 2017

View reviewed changes

peterneyens merged commit 2db490a into typelevel:master Aug 26, 2017

kailuowang modified the milestone: 1.0.0-RC1 Aug 28, 2017

LukaJCB pushed a commit to LukaJCB/cats that referenced this pull request Sep 1, 2017

Add Foldable and Traversable instances for Free (typelevel#1840)

bf26750

kailuowang added the enhancement label Oct 15, 2017


		sealed private[free] abstract class FreeInstances {

		implicit def catsFoldableForFree[F[_]](

Add Foldable and Traversable instances for Free #1840

Add Foldable and Traversable instances for Free #1840

Conversation

aaronlevin commented Aug 21, 2017

aaronlevin commented Aug 22, 2017

peterneyens left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronlevin commented Aug 22, 2017

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

Choose a reason for hiding this comment

peterneyens commented Aug 22, 2017 • edited Loading

aaronlevin commented Aug 22, 2017

codecov-io commented Aug 22, 2017 • edited Loading

Codecov Report

aaronlevin commented Aug 22, 2017

peterneyens commented Aug 22, 2017

peterneyens commented Aug 22, 2017

aaronlevin commented Aug 22, 2017

aaronlevin commented Aug 22, 2017

aaronlevin commented Aug 22, 2017

peterneyens commented Aug 22, 2017

aaronlevin commented Aug 22, 2017

peterneyens commented Aug 22, 2017

aaronlevin commented Aug 22, 2017

peterneyens left a comment

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

Choose a reason for hiding this comment

aaronlevin commented Aug 23, 2017

peterneyens commented Aug 23, 2017

aaronlevin commented Aug 23, 2017

aaronlevin commented Aug 23, 2017

peterneyens commented Aug 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronlevin commented Aug 25, 2017

peterneyens left a comment

Choose a reason for hiding this comment

johnynek left a comment

Choose a reason for hiding this comment

peterneyens commented Aug 22, 2017 •

edited

Loading

codecov-io commented Aug 22, 2017 •

edited

Loading