Backport #3103 traverseFilter Queue instance to scala_2.11

[gagandeepkalra]

addresses #3143

[gagandeepkalra]

We can have an implementation without using Eval, might even perform a little better (for a bigger queue); without all those extra Eval wrappers.

fa.foldRight(G.pure(Queue.empty[B]))( (x, xse) => G.map2(f(x), xse)((i, o) => i.fold(o)(_ +: o)))

[travisbrown]

That won't short-circuit, though, right?

[gagandeepkalra]

@travisbrown yes agreed it won't, but for the same reason the present implementation wouldn't.

If by short circuit you mean f wouldn't have to run on the remaining Queue elements (given we are folding from the right).

This is my understanding and derived reasoning-
map2Eval short-circuits: given the underlying G structure allows it, then the Eval argument skips running.

In this case however, the Eval description is computed by running f on each element of the Queue already. The first argument to map2Eval is not Lazy. In every situation f will run on each element.

Please correct me If I understood this wrongly.

[gagandeepkalra]

I see it now, we are using the wrong foldRight here, queue.foldRight ends up with a while loop (standard library). This should have been-

traverse.foldRight[A, G[Queue[B]]](fa, Always(G.pure(Queue.empty))) { (a, lglb) => G.map2Eval(f(a), lglb)((i, o) => i.fold(o)(_ +: o)) } .value

[gagandeepkalra]

I see the same problem with other traverseFilter implementations, should I open a new issue?

[travisbrown]

That won't short-circuit, though, right?

[travisbrown]

Update

The comment below is wrong. 😄 Or at least the last sentence—it's definitely possible to do that, and @gagandeepkalra's implementation above works:

import cats.Applicative

def traverseFilter[G[_], A, B](fa: Queue[A])(f: (A) => G[Option[B]])(implicit G: Applicative[G]): G[Queue[B]] =
  fa.foldRight(G.pure(Queue.empty[B]))( (x, xse) => G.map2(f(x), xse)((i, o) => i.fold(o)(_ +: o)))

And then:

scala> traverseFilter(Queue(2, 1, 0, -1, -2))(g).value.written
res14: List[Int] = List(2, 1, 0)

I'll look at this more closely tomorrow.

Original comment

@gagandeepkalra Answering your question above here so the discussion doesn't get lost. Part of the problem is that (as far as I know) we don't have very good settled terminology here. If you look at it like this…

import cats.implicits._
import scala.collection.immutable.Queue

val f: Int => Either[String, Option[Int]] = i => {
  print(s"$i ")
  if (i > 0) Right(Some(i)) else Left("not positive")
}

Then right, the implementations don't match—the List one is lazier than the others:

scala> List(2, 1, 0, -1).traverseFilter(f)
2 1 0 res0: Either[String,List[Int]] = Left(not positive)

scala> Vector(2, 1, 0, -1).traverseFilter(f)
-1 0 1 2 res1: Either[String,scala.collection.immutable.Vector[Int]] = Left(not positive)

scala> Queue(2, 1, 0, -1).traverseFilter(f)
-1 0 1 2 res2: Either[String,scala.collection.immutable.Queue[Int]] = Left(not positive)

But that's not what we mean by short-circuiting (or at least that's not how I've been using it). The important thing is that once we've "failed", we stop accumulating the effects of f in G (and may or may not stop applying the function). For example:

import cats.data.{EitherT, Writer}

val g: Int => EitherT[Writer[List[Int], *], String, Option[Int]] = i => if (i > 0) {
  EitherT.right[String](Writer(List(i), Some(i)))
} else {
  EitherT.left(Writer(List(i), "not positive"))
}

And in that sense they all short-circuit as expected:

scala> List(2, 1, 0, -1).traverseFilter(g).value.written
res11: List[Int] = List(2, 1, 0)

scala> Vector(2, 1, 0, -1).traverseFilter(g).value.written
res12: List[Int] = List(2, 1, 0)

scala> Queue(2, 1, 0, -1).traverseFilter(g).value.written
res13: List[Int] = List(2, 1, 0)

I don't think it would be possible to implement traverseFilter without map2Eval (or some equivalent) and maintain this short-circuiting.

[travisbrown]

@gagandeepkalra I updated my previous comment because it was completely wrong. 😄

I think you're right and the TraverseFilter implementations for both Queue and Vector (and probably other stuff) are broken. 😦

[gagandeepkalra]

@travisbrown thank you for following up. I guess we leave this as is here and fix it on the main branch. Would love to take this on though 🙂

[gagandeepkalra]

accidentally closed the PR.

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (scala_2.11@0980189). Click here to learn what that means.
The diff coverage is 100%.

@@              Coverage Diff              @@
##             scala_2.11    #3292   +/-   ##
=============================================
  Coverage              ?   93.47%           
=============================================
  Files                 ?      385           
  Lines                 ?     7174           
  Branches              ?      195           
=============================================
  Hits                  ?     6706           
  Misses                ?      468           
  Partials              ?        0

Impacted Files	Coverage Δ
core/src/main/scala/cats/instances/queue.scala	100% <100%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0980189...8335f18. Read the comment docs.