Add unit test to verify Chain.zipWithIndex stack safety

[dantb]

Noticed while poking around to add zipAll that zipWithIndex on Chain (also called from NonEmptyChain) isn’t tail recursive. It’ll blow the stack if the chain is built from many concats meaning we have a highly nested Append structure. E.g.

scala> List.fill(10000)(1).foldLeft(Chain(1)) { case (acc, next) => acc.concat(Chain(next)) }.zipWithIndex
java.lang.StackOverflowError
  at scala.collection.AbstractIterable.<init>(Iterable.scala:56)
  at scala.collection.AbstractSeq.<init>(Seq.scala:45)

This wasn’t caught by tests as the arbitrary instances generate only small collections - I guess this was a conscious decision?

Either way worth flagging 😄

Edit: has since been fixed in #3533 so this change only adds a unit test. Changed title to reflect that.

[travisbrown]

👍, this might be my fault. Good catch, and thanks for implementing it. Do you mind adding a test?

[rossabaker]

Any reason not to use a .toVector or something else that ++'s better than List here?

[dantb]

Sorry appalling response time on this. The arbitrary instance here is used all over the Chain tests so I thought I would add a targeted non property-based unit test, is that alright @travisbrown ?

Also I only force pushed, github making me sound sassy 💁‍♂️

@dantb dismissed travisbrown’s stale review

[barambani]

The failure here is just styling, you probably just need to re-run +prePR and should be good to go.

[johnynek]

why not:

case a@Append(_, _) =>
  val it = a.iterator
  Wrap(it.zip(Iterator.iterate(0)(_ + 1)).toList)

this way, we only materialize one new list and leverage the existing iterator code. The code above materializes to vector (.toVector), one of them is copies again: (.map(... leftSize + i) }), then copies them (.zipWithIndex) then concats them (++).

So, there are like 6 materializations of the values. the iterator approach I suggest above does just one.

[barambani]

I think the current master's version resolve this issue, but the test in here is still a valuable addition IMO. @dantb, I was wondering if you have some steam left to resolve the conflicts. Either way thanks a lot for your contribution.

[dantb]

Rebased and repurposed - thanks @barambani

[johnynek]

thanks!

[barambani]

Thanks again.