Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 8 commits into from
May 5, 2019
Merged

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b9b8144
added a new foldRight lazy law, move forAll and exist laws
kailuowang Apr 28, 2019
70d513f
reformat
kailuowang Apr 28, 2019
d6b1584
fix foldRight on NonEmptyChain
kailuowang Apr 28, 2019
5811126
reformat
kailuowang Apr 28, 2019
1e9fcd7
reformat
kailuowang Apr 28, 2019
f75c18a
Update FoldableLaws.scala
kailuowang Apr 28, 2019
cc8f633
incorporated Ross and Oscar suggestion
kailuowang May 5, 2019
fbc7cf8
override default implementation in map and set
kailuowang May 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/src/main/scala/cats/data/NonEmptyChain.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,7 +452,7 @@ sealed abstract private[data] class NonEmptyChainInstances extends NonEmptyChain
fa.foldLeft(b)(f)

override def foldRight[A, B](fa: NonEmptyChain[A], lb: Eval[B])(f: (A, Eval[B]) => Eval[B]): Eval[B] =
fa.foldRight(lb)(f)
Foldable[Chain].foldRight(fa.toChain, lb)(f)

override def foldMap[A, B](fa: NonEmptyChain[A])(f: A => B)(implicit B: Monoid[B]): B =
B.combineAll(fa.toChain.iterator.map(f))
Expand Down
29 changes: 29 additions & 0 deletions laws/src/main/scala/cats/laws/FoldableLaws.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,16 @@ import scala.collection.mutable
trait FoldableLaws[F[_]] extends UnorderedFoldableLaws[F] {
implicit def F: Foldable[F]

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Eval.now("not empty")
}
.value
i == (if (F.isEmpty(fa)) 0 else 1)
}

def leftFoldConsistentWithFoldMap[A, B](
fa: F[A],
f: A => B
Expand Down Expand Up @@ -111,6 +121,25 @@ trait FoldableLaws[F[_]] extends UnorderedFoldableLaws[F] {
def orderedConsistency[A: Eq](x: F[A], y: F[A])(implicit ev: Eq[F[A]]): IsEq[List[A]] =
if (x === y) (F.toList(x) <-> F.toList(y))
else List.empty[A] <-> List.empty[A]

def existsLazy[A](fa: F[A]): Boolean = {
var i = 0
F.exists(fa) { _ =>
i += 1
true
}
i == (if (F.isEmpty(fa)) 0 else 1)
}

def forallLazy[A](fa: F[A]): Boolean = {
var i = 0
F.forall(fa) { _ =>
i += 1
false
}
i == (if (F.isEmpty(fa)) 0 else 1)
}

}

object FoldableLaws {
Expand Down
18 changes: 0 additions & 18 deletions laws/src/main/scala/cats/laws/UnorderedFoldableLaws.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

var i = 0
F.exists(fa) { _ =>
i = i + 1
true
}
i == (if (F.isEmpty(fa)) 0 else 1)
}

def forallLazy[A](fa: F[A]): Boolean = {
var i = 0
F.forall(fa) { _ =>
i = i + 1
false
}
i == (if (F.isEmpty(fa)) 0 else 1)
}

/**
* If `F[A]` is empty, forall must return true.
*/
Expand Down
1 change: 1 addition & 0 deletions laws/src/main/scala/cats/laws/discipline/FoldableTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ trait FoldableTests[F[_]] extends UnorderedFoldableTests[F] {
parent = Some(unorderedFoldable[A, B]),
"foldLeft consistent with foldMap" -> forAll(laws.leftFoldConsistentWithFoldMap[A, B] _),
"foldRight consistent with foldMap" -> forAll(laws.rightFoldConsistentWithFoldMap[A, B] _),
"foldRight is lazy" -> forAll(laws.foldRightLazy[A] _),
"ordered constistency" -> forAll(laws.orderedConsistency[A] _),
"exists consistent with find" -> forAll(laws.existsConsistentWithFind[A] _),
"exists is lazy" -> forAll(laws.existsLazy[A] _),
Expand Down