-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
Fix ReducibleLaws causing stack overflow by calling Eval.now
early
#3565
Fix ReducibleLaws causing stack overflow by calling Eval.now
early
#3565
Conversation
The test `reduceRightConsistentWithReduceRightOption` currently calls `value` on an `Eval` before the end of the computation, i.e. inside the functions passed to `reduceRight` and `reduceRightOption`. This can cause a stack overflow in otherwise lawful `Reducible` instances. The test has been changed to instead use `map` on the `Eval` which is the intended way of using `Eval` in methods like `reduceRight`.
It would be hard for me to write a test for this behaviour, it only happens in a complex and recursive data-type* which I have defined a I did check if this test was intentionally written like this on Gitter before opening this PR but got no response, I'm pretty sure it is a mistake in the way the laws test is written though. (https://gitter.im/typelevel/cats?at=5f3165af733c00338f036dbd). I also checked the other laws suites for cases of * The type I see the failure for is a zipper for navigating a rose tree (multi-way tree), the // Scala 2.13 LazyList
final case class Tree[A](rootLabel: A, subForest: LazyList[Tree[A]])
type TreeForest[A] = LazyList[Tree[A]]
type Parent[A] = (TreeForest[A], A, TreeForest[A])
type Parents[A] = LazyList[Parent[A]]
final case class TreeLoc[A](tree: Tree[A], lefts: TreeForest[A], rights: TreeForest[A], parents: Parents[A]) |
Codecov Report
@@ Coverage Diff @@
## master #3565 +/- ##
=======================================
Coverage 91.26% 91.26%
=======================================
Files 386 386
Lines 8587 8587
Branches 235 237 +2
=======================================
Hits 7837 7837
Misses 750 750 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's a no-brainer 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
The test
reduceRightConsistentWithReduceRightOption
currently callsvalue
on anEval
before the end of the computation, i.e. it callsvalue
inside the functions passed to
reduceRight
andreduceRightOption
.This can cause a stack overflow in otherwise lawful
Reducible
instances.The test has been changed to instead use
map
on theEval
which isthe intended way of using
Eval
in methods likereduceRight
.