fix foldMap stack safety #702
Conversation
| case Free.Gosub(c, g) => c match { | ||
| case Free.Pure(a) => g(M.pure(a)).foldMap(f) | ||
| case Free.Suspend(s) => g(f(s)).foldMap(f) | ||
| case Free.Gosub(c1, g1) => g(M.flatMap(c1.foldMapRecursiveStep(f))(cc => g1(cc).foldMapRecursiveStep(f))).foldMap(f) |
There was a problem hiding this comment.
tbh this line should never happen as its resolved by step - should we simple throw an exception and remove foldMapRecursiveStep function?
There was a problem hiding this comment.
will remove Free. prefixes
also, looks like the only thing thats not handled by step is Suspend..
Current coverage is
|
|
how to run @codecov-io again? |
| case Gosub(c, g) => M.flatMap(c.foldMap(f))(cc => g(cc).foldMap(f)) | ||
| case Gosub(c, g) => c match { | ||
| case Suspend(s) => g(f(s)).foldMap(f) | ||
| case _ => throw new Error("Unexpected operation. The case should have been eliminated by `step`.") |
There was a problem hiding this comment.
Need to check where else is step used, could it be moved to this function to avoid the exception throwing or does it add value on its own?
|
👍 |
|
Thanks, @svalaskevicius! This looks good to me. Does anyone else have any concerns? @mandubian? |
|
👍 good catch... I wonder why we have missed this before ;) |
This reverts most of the changes from typelevel#702, because it appears to have caused a regression in the case of Gosub. The stack safety test is now failing. I've commented it out for now, because an incorrectness bug seems to be worse than a stack safety issue.
This reverts most of the changes from typelevel#702, because it appears to have caused a regression in the case of Gosub. This commit fixes the "mapSuspension id" unit test and the issue reported in typelevel#712. The stack safety test is now failing. I've commented it out for now, because an incorrectness bug seems to be worse than a stack safety issue.
No description provided.