Rework Foldable derivation

[joroKr21]

• Make it more consistent with other derivations
• Add more tests
• Make sure it's stack safe
• Make sure forall and exists are lazy

Based on #138

[kailuowang]

interesting. Did you find out because it broke the forallLazy law here?

[kailuowang]

Update, yes I confirmed that without these it broke the law. However the default forall implementation did not break this law on List instance (yes, I commented out the override forall in the list instance). I am trying understand is the default implementation of forall incorrect? or is it the foldRight derived here different?

[kailuowang]

I figured it out. Our foldRight seems to be problematic (i.e. we shouldn't override the forall and exist, we should fix our foldRight)
I added this test to our TraverseSuite on your branch

  test(s"$context foldRightLazy Not Empty") {
      var i = 0
      iList.foldRight(large, Eval.now(0)) { (_, _) =>
        i += 1
        Eval.now(1)
      }.value

      assert(i == 1)
    }

This test failed , i turns out to be 9999 the size of large.
It is a surprise that it's caught through foralllazy and existlazy laws, which I think is an issue in cats-laws. So I PRed one there (typelevel/cats#2817)

I haven't got the time to figure which part in our derivation that makes foldRight not lazy. Need to go afk for the night.

[joroKr21]

Hmm, interesting. I don't understand entirely how it's supposed to work. How do you know that the first value is the final one so you stop calling foldRight?

[joroKr21]

I figured it out. foldRight was being called in the wrong order for HCons. I noticed this before but I missed the part where I have to add Eval.defer to make it lazy / stack safe:

https://github.com/typelevel/kittens/compare/8223a1c8a9ad16b64d0cf6b3ff1b3ef9527db579..1f85bcff82661e3476b0ac361bb19f88bedb9ef6

[joroKr21]

@kailuowang now foldRight for Snoc is not stack safe. I'm out of ideas...

[kailuowang]

I have some ideas I want to try. will report back later.

[kailuowang]

@joroKr21 , hmmm this pass locally on my macbook pro, which made it harder for me to debug the issue. how about on your machine?

[joroKr21]

Ah sorry, I should have pushed a reliably failing test. We just need to modify val large to use Snoc instead of IList.

Like this:

   test(s"$context.Foldable[Snoc].foldRight is stack safe") {
      val large = List.fill(1)(Snoc.fromSeq(1 until n))
      val actual = listSnoc.foldRight(large, Eval.Zero)((i, sum) => sum.map(_ + i))
      val expected = 100 * n * (n - 1) / 2
      assert(actual.value == expected)
    }

[joroKr21]

@kailuowang I am baffled. This definition (which is closer to the original) works:

      def foldRight[A, B](fa: F[A], lb: Eval[B])(f: (A, Eval[B]) => Eval[B]) = for {
        unpacked <- Eval.now(F.unpack(fa))
        b <- F.fh.unify.foldRight(unpacked._1, F.ft.foldRight(unpacked._2, lb)(f))(f)
      } yield b

It looks like Eval.Unit.flatMap(_ => x) is stack safe and Eval.defer(x) isn't.
Is that expected behaviour?

[kailuowang]

It looks like Eval.Unit.flatMap(_ => x) is stack safe and Eval.defer(x) isn't.

hmm, not sure about that so I created a test here
https://scastie.scala-lang.org/xS2EW7NnQ4WzGreUn6tIOw

Seems both are stack safe in the simple use case.
I am looking into what could be the problem in our derivation.

[kailuowang]

We are using Eval to stack safe our recursive call of foldRight. In this particular case, Eval.flatmap works fine but Eval.defer SOs.

In fact if you put a Eval.defer inside Eval.flatMap it also SO. The following example SO

       def foldRight[A, B](fa: F[A], lb: Eval[B])(f: (A, Eval[B]) => Eval[B]) =
        Eval.now(F.unpack(fa)).flatMap { case (fha, fhb) =>
          F.fh.unify.foldRight(fha, Eval.defer(F.ft.foldRight(fhb, lb)(f)))(f) //Eval.defer added here just to test it. 
        }

I couldn't get to the bottom of this one. I was trying to minimize it but didn't succeed.
anyway, I think for this PR, it's ready to go.

[kailuowang]

Thanks so much!