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

Foldable instance for List breaks substitution #1716

Closed
noelwelsh opened this issue Jun 2, 2017 · 5 comments
Closed

Foldable instance for List breaks substitution #1716

noelwelsh opened this issue Jun 2, 2017 · 5 comments
Assignees
Labels
Milestone

Comments

@noelwelsh
Copy link

noelwelsh commented Jun 2, 2017

The Foldable instance for List breaks substitution, as the following program demonstrates. The value of result.value differs between the first and subsequent calls.

import cats.instances.list._
import cats.Eval
import cats.syntax.foldable._

val result  = List.range(0,10).foldM(List.empty[Int])((accum, elt) => Eval.always(elt :: accum))

result.value
// res: List[Int] = List(9, 8, 7, 6, 5, 4, 3, 2, 1, 0)

result.value
// res: List[Int] = List(0)

I haven't had time to debug but I suspect it relates to the use of a mutable ListBuffer in the implementation of tailRecM.

@peterneyens peterneyens added the bug label Jun 2, 2017
@peterneyens
Copy link
Collaborator

It is the use of an iterator in foldM using Foldable.iteratorFoldM.

With the default implementation of foldM it evaluates both times :

import cats.instances.list._
import cats.Eval

val numbers = List.range(0, 10)
val result = Foldable[List].foldLeft(numbers, Eval.now(List.empty[Int])) { (eb, a) => 
  eb.flatMap(acc => Eval.always(a :: acc))
}

result.value
// List[Int] = List(9, 8, 7, 6, 5, 4, 3, 2, 1, 0)

result.value
// List[Int] = List(9, 8, 7, 6, 5, 4, 3, 2, 1, 0)

@kailuowang kailuowang added this to the 1.0.0-MF milestone Jun 2, 2017
@peterneyens
Copy link
Collaborator

If we change the foldM implementations which use Foldable.iteratorFoldM to use the default implementation, we probably need to reopen #1030, because the default implementation always folds across the entire structure while the implementation using Iterator and tailRecM can short circuit (added in #1414).

A potential solution may be @TomasMikula 's #1117.

@kailuowang
Copy link
Contributor

@peterneyens shall we schedule #1117 to 1.0.0-MF then? I feel bad that we let it fell through the cracks.

@ceedubs
Copy link
Contributor

ceedubs commented Jun 21, 2017

I've approved #1117. It would be great to get some other reviewers on that. If/when it's merged we probably want to remove Foldable.iteratorFoldM, since it leads to these sorts of issues.

@johnynek
Copy link
Contributor

Yeah, it seems like Foldable.iteratorFoldM is just not safe for "rerunning" monads. Since you can't tell if you have a monad like that or not when it is generic, it's only safe if you concretely know the monad you are using.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants