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.

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

Merged
merged 1 commit into from
Aug 12, 2020
Merged

Fix ReducibleLaws causing stack overflow by calling Eval.now early #3565

merged 1 commit into from
Aug 12, 2020

Conversation

bastewart
Copy link
Contributor

@bastewart bastewart commented Aug 12, 2020

The test reduceRightConsistentWithReduceRightOption currently calls
value on an Eval before the end of the computation, i.e. it calls value
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.

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`.
@bastewart
Copy link
Contributor Author

bastewart commented Aug 12, 2020

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 Reducible instance for. I have run sbt validate locally and do get a failure, but it is in the documentation generation (tut) so am pretty sure it's unrelated!

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 value being called before the final Eval has been resolved but couldn't see any others to fix.

* The type I see the failure for is a zipper for navigating a rose tree (multi-way tree), the TreeLoc below. The test I have changed in this PR fails with an SO when testing Reducible[TreeLoc], with my change it doesn't though.

// 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-commenter
Copy link

Codecov Report

Merging #3565 into master will not change coverage.
The diff coverage is 100.00%.

@@           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           

Copy link
Member

@joroKr21 joroKr21 left a 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 👍

@travisbrown
Copy link
Contributor

This looks reasonable to me but it'd probably be good for @johnynek to take a quick look since he added the test in #1475.

@LukaJCB LukaJCB requested a review from johnynek August 12, 2020 15:05
Copy link
Member

@LukaJCB LukaJCB left a 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!

Copy link
Contributor

@johnynek johnynek left a comment

Choose a reason for hiding this comment

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

Looks good

@johnynek johnynek merged commit fc959f4 into typelevel:master Aug 12, 2020
@travisbrown travisbrown added this to the 2.2.0-RC3 milestone Aug 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants