added a new foldRight lazy law, move forallLazy and existLazy laws #2817

kailuowang · 2019-04-28T02:06:57Z

I noticed that foldRight's laziness was tested through forallLazy and existLazy laws. Since foldRight is a type class method I think we should have the laziness law on it. In kittens we were caught by the lack of such a law.

Also I noticed that although forallLazy and existLazy are defined in UnorderedFoldableLaws, they are actually only used in FoldableTests. Moreover, implementations in UnorderedFoldable breaks these two laws. So I think they should probably be FoldableLaws.

Update: This actually caught a bug in NonEmptyChain whose Foldable.foldRight implementation is simply delegating to the native (scala std lib) foldRight.

kailuowang · 2019-04-28T02:09:10Z

BTW, strictly speaking this is breaking change for UnorderedFoldableLaws but it's unlikely to affect anyone and we are breaking BC in cats-laws in our next release.

joroKr21 · 2019-04-28T08:27:52Z

I guess it makes sense because someone might override forall and exists before testing the laws and then not realize that the foldRight implementation is not lazy.

joroKr21 · 2019-04-28T08:54:16Z

This mistake is actually incredibly easy to make. Here is a silly example:

    implicit val foldableSeq: Foldable[Seq] = new Foldable[Seq] {
      def foldLeft[A, B](fa: Seq[A], b: B)(f: (B, A) => B) = fa.foldLeft(b)(f)
      def foldRight[A, B](fa: Seq[A], lb: Eval[B])(f: (A, Eval[B]) => Eval[B]) = fa.foldRight(lb)(f)
      override def forall[A](fa: Seq[A])(p: A => Boolean) = fa.forall(p)
      override def exists[A](fa: Seq[A])(p: A => Boolean) = fa.exists(p)
    }

kailuowang · 2019-04-28T11:43:56Z

@joroKr21 funny that's exactly what I just discovered on NonEmptyChain

codecov-io · 2019-04-28T12:19:03Z

Codecov Report

Merging #2817 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2817      +/-   ##
==========================================
+ Coverage   94.15%   94.21%   +0.05%     
==========================================
  Files         368      368              
  Lines        6914     6944      +30     
  Branches      296      301       +5     
==========================================
+ Hits         6510     6542      +32     
+ Misses        404      402       -2

Impacted Files	Coverage Δ
core/src/main/scala/cats/data/NonEmptyChain.scala	`97.32% <ø> (-0.83%)`	⬇️
...c/main/scala/cats/laws/UnorderedFoldableLaws.scala	`100% <100%> (+5.26%)`	⬆️
.../cats/laws/discipline/UnorderedFoldableTests.scala	`100% <100%> (+12.5%)`	⬆️
core/src/main/scala/cats/instances/set.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/instances/map.scala	`95.34% <100%> (+0.22%)`	⬆️
...ain/scala/cats/laws/discipline/FoldableTests.scala	`100% <100%> (ø)`	⬆️
laws/src/main/scala/cats/laws/FoldableLaws.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/UnorderedFoldable.scala	`100% <100%> (+7.14%)`	⬆️
core/src/main/scala/cats/Apply.scala	`75% <0%> (ø)`	⬆️
... and 4 more

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 7514aa6...fbc7cf8. Read the comment docs.

kailuowang · 2019-04-28T13:16:27Z

cc @non, @ceedubs and @johnynek who may have opinion over this.

johnynek · 2019-04-28T15:47:36Z

laws/src/main/scala/cats/laws/FoldableLaws.scala

+  def foldRightLazy[A](fa: F[A]): Boolean = {
+    var i = 0
+    F.foldRight(fa, Eval.now("empty")) { (_, _) =>
+        i += 1


We use += here but not below. Any reason for the difference?

no real reason except this one is written by me and the other ones are from before. Just changed the other ones to be more consistent.

johnynek · 2019-04-28T15:51:05Z

laws/src/main/scala/cats/laws/UnorderedFoldableLaws.scala

@@ -21,24 +21,6 @@ trait UnorderedFoldableLaws[F[_]] {
      (F.isEmpty(fa) || F.exists(fa)(p))
    } else true // can't test much in this case

-  def existsLazy[A](fa: F[A]): Boolean = {


Foldable extends UnorderedFoldable. To me, the bug here is that we weren’t calling these in UnorderedFoldable tests and that FoldableLaws were not calling through to them as we should do any time we extend a typeclass.

That seems like the better fix to me than moving the law (since we should still want these laws on UnorderedFoldable).

It does look that way but actually the default implementations of exist and forall in UnorderedFoldable do not obey these laws. These laws can only be supported in Foldable through foldRight, hence they should be in Foldable

We can keep the laws in UnorderedFoldableTests if we implement the CommutativeMonoid[Eval[Boolean]]s directly in UnorderedFoldable. See rossabaker@4af246d.

rossabaker

👍 to foldRightLazy.

We don't need to move the laws. See below.

rossabaker · 2019-05-04T03:46:23Z

laws/src/main/scala/cats/laws/UnorderedFoldableLaws.scala

@@ -21,24 +21,6 @@ trait UnorderedFoldableLaws[F[_]] {
      (F.isEmpty(fa) || F.exists(fa)(p))
    } else true // can't test much in this case

-  def existsLazy[A](fa: F[A]): Boolean = {


We can keep the laws in UnorderedFoldableTests if we implement the CommutativeMonoid[Eval[Boolean]]s directly in UnorderedFoldable. See rossabaker@4af246d.

johnynek

I’d rather keep the laws the forall and exists are lazy on UnorderedFoldable.

That said, the worst case is already linear complexity so maybe that law is too restrictive anyway. Maybe the real mistake is forcing laziness on Foldable.

johnynek · 2019-05-04T03:59:55Z

If we are going to keep a laziness law, my vote is to keep it on UnorderedFoldable, if we aren’t going to do that, I’d next prefer to remove the laziness requirement entirely.

joroKr21 · 2019-05-04T12:39:36Z

That said, the worst case is already linear complexity so maybe that law is too restrictive anyway. Maybe the real mistake is forcing laziness on Foldable.

I think one might expect the laziness from the type signature.

rossabaker · 2019-05-04T18:26:35Z

The commit I linked above keeps the lazy laws on UnorderedFoldable, passes the tests for Map[K, ?] and Set with the new default implementation, and preserves bincompat in core. Are there any desiderata that it misses?

johnynek · 2019-05-04T18:32:38Z

@RossBaker I think the approach is good but I have a few nits:

We should override exists and forall for our concrete instances.
In the Eval monoids I’d rather not reevaluate the Eval which could be expensive in principle.

kailuowang · 2019-05-04T18:50:17Z

Thanks for all the feedbacks. I will incorporate @RossBaker's code with @johnynek's suggestions

rossabaker · 2019-05-04T19:44:29Z

I agree with @johnynek's suggestions, but if we override for Map and Set, that will leave the default implementations untested. I'm unsure whether we've developed an idiom for testing unused defaults.

kailuowang · 2019-05-04T19:58:56Z

@rossabaker https://github.com/typelevel/cats/blob/master/tests/src/test/scala/cats/tests/UnorderedFoldableSuite.scala is already setup to test some other default implemetations. This should be easy to add.

added a new foldRight lazy law, move forAll and exist laws

b9b8144

kailuowang requested a review from LukaJCB April 28, 2019 02:07

kailuowang added the testing label Apr 28, 2019

kailuowang added this to the 2.0.0-RC1 milestone Apr 28, 2019

kailuowang mentioned this pull request Apr 28, 2019

Rework Foldable derivation typelevel/kittens#139

Merged

reformat

70d513f

fix foldRight on NonEmptyChain

d6b1584

reformat

5811126

kailuowang added bug and removed testing labels Apr 28, 2019

reformat

1e9fcd7

johnynek reviewed Apr 28, 2019

View reviewed changes

Update FoldableLaws.scala

f75c18a

kailuowang requested review from rossabaker and tpolecat May 3, 2019 16:52

rossabaker requested changes May 4, 2019

View reviewed changes

johnynek reviewed May 4, 2019

View reviewed changes

incorporated Ross and Oscar suggestion

cc8f633

kailuowang requested a review from rossabaker May 5, 2019 01:05

override default implementation in map and set

fbc7cf8

johnynek approved these changes May 5, 2019

View reviewed changes

rossabaker approved these changes May 5, 2019

View reviewed changes

kailuowang merged commit 287c27e into typelevel:master May 5, 2019

kailuowang modified the milestones: 2.0.0-RC1, 2.0.0-M2 Jun 4, 2019

travisbrown mentioned this pull request Mar 6, 2020

Backport #2772 add foldRightDefer to Foldable #3338

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added a new foldRight lazy law, move forallLazy and existLazy laws #2817

added a new foldRight lazy law, move forallLazy and existLazy laws #2817

kailuowang commented Apr 28, 2019 •

edited

Loading

kailuowang commented Apr 28, 2019

joroKr21 commented Apr 28, 2019

joroKr21 commented Apr 28, 2019

kailuowang commented Apr 28, 2019

codecov-io commented Apr 28, 2019 •

edited

Loading

kailuowang commented Apr 28, 2019

johnynek Apr 28, 2019

kailuowang Apr 28, 2019

johnynek Apr 28, 2019

kailuowang Apr 28, 2019

rossabaker May 4, 2019

rossabaker left a comment

rossabaker May 4, 2019

johnynek left a comment

johnynek commented May 4, 2019

joroKr21 commented May 4, 2019

rossabaker commented May 4, 2019

johnynek commented May 4, 2019

kailuowang commented May 4, 2019

rossabaker commented May 4, 2019

kailuowang commented May 4, 2019

added a new foldRight lazy law, move forallLazy and existLazy laws #2817

added a new foldRight lazy law, move forallLazy and existLazy laws #2817

Conversation

kailuowang commented Apr 28, 2019 • edited Loading

kailuowang commented Apr 28, 2019

joroKr21 commented Apr 28, 2019

joroKr21 commented Apr 28, 2019

kailuowang commented Apr 28, 2019

codecov-io commented Apr 28, 2019 • edited Loading

Codecov Report

kailuowang commented Apr 28, 2019

johnynek Apr 28, 2019

Choose a reason for hiding this comment

kailuowang Apr 28, 2019

Choose a reason for hiding this comment

johnynek Apr 28, 2019

Choose a reason for hiding this comment

kailuowang Apr 28, 2019

Choose a reason for hiding this comment

rossabaker May 4, 2019

Choose a reason for hiding this comment

rossabaker left a comment

Choose a reason for hiding this comment

rossabaker May 4, 2019

Choose a reason for hiding this comment

johnynek left a comment

Choose a reason for hiding this comment

johnynek commented May 4, 2019

joroKr21 commented May 4, 2019

rossabaker commented May 4, 2019

johnynek commented May 4, 2019

kailuowang commented May 4, 2019

rossabaker commented May 4, 2019

kailuowang commented May 4, 2019

kailuowang commented Apr 28, 2019 •

edited

Loading

codecov-io commented Apr 28, 2019 •

edited

Loading