Fix slicing of views of IndexedSeqs (including fixing 2.13.7-only reverseIterator regression)

[som-snytt]

Fixes scala/bug#12479

[SethTisue]

review by @scala/collections ?

[som-snytt]

If collections drops by, so to speak, maybe they could indicate whether it was intentional that drop was made eager. That is, it calls next instead of using sliceIterator like take does.

[som-snytt]

I see Stefan reverted the change in laziness for take, so maybe I can just propose the same reversion for drop.

"An iterator for slicing that does not drop eagerly." at #4086

The paulp tweets there are dead links.

[NthPortal]

title should be "fixed slice of indexed seq view [...]"

[SethTisue]

A plain fix is of course mergeable, but it would be awesome if someone could plug the apparent hole in https://github.com/scala/scala-collection-laws too

[som-snytt]

Unfortunately, I broke another test-at-a-distance. I'll also look into The Laws. I didn't feel great tweaking the innards without test coverage.

[NthPortal]

while certainly simpler and a lot easier to follow, I'm not certain that the new implementation of IndexedSeqViewReverseIterator#sliceIterator is correct. also, I think we probably need more tests for sliceIterator, since I didn't see anything directly related to it fail

[johnynek]

some laws that might be useful to scalacheck:

forAll { (v: Vector[Int], n0: Int) =>
  val n = n0 & Int.MaxValue // make it non-neg
  assert(v.iterator.drop(n).toList == v.toList.drop(n))
}
forAll { (v: Vector[Int], n: Int) =>
  val n = n0 & Int.MaxValue // make it non-neg
  assert(v.iterator.take(n).toList == v.toList.take(n))
}
forAll { (v: Vector[Int], n: Int) =>
  val n = n0 & Int.MaxValue // make it non-neg
  assert(v.reverseIterator.drop(n).toList == v.toList.reverse.drop(n))
}

forAll { (v: Vector[Int], n: Int) =>
  val n = n0 & Int.MaxValue // make it non-neg
  assert(v.reverseIterator.take(n).toList == v.toList.reverse.take(n))
}

forAll { (v: Vector[Int], a0: Int, b0: Int) =>
  val a1 = a0 & Int.MaxValue // make it non-neg
  val b1 = b0 & Int.MaxValue // make it non-neg
  val (a, b) = if (a <= b) (a1, b1) else (b1, a1)
  assert(v.view.slice(a, b).toList == v.slice(a, b).toList)
}

[som-snytt]

@NthPortal there are optimized small Map iterators from last year (to avoid calling Iterator.apply). They override drop to just bump a count; that is probably where it came up to return this.type. But that only makes sense for iterating over these pre-computed values. These iterators do not override take because that is too hard? Or is every iterator a slice at heart?

The failing test was to check that empty and single Maps don't allocate on iterating after drop; the fix was to make Iterator.empty and single not allocate.

[Ichoran]

@johnynek - The property tests in the Scala Collections Laws repository do not use scalacheck because they instead do code generation to also check type inference and such. It would also be possible to write more property tests in scalacheck.

@som-snytt - Collections Laws are rather idiosyncratic. Let me know if I should try to carve out time to tackle this part. I don't remember the code much, but I guess "not much" is better than none, and I have the advantage that I still probably think like 2014-me. (Or whenever that was.)

[johnynek]

@SethTisue I'm sorry for making noise here (I don't know an appropriate channel) but can you remove me from the collections group? I don't have time to do these reviews at the moment.

[SethTisue]

@johnynek done! thanks for all the help you've provided these past months, it's much appreciated 🙇

[NthPortal]

I still think a few more tests for sliceIterator (directly or indirectly) would be a good idea; otherwise, LGTM

[lrytz]

Thank you all 👍

[lrytz]

Another follow-up could be to remove the drop override in IndexedSeqViewIterator (the non-reverse variant).

scala/src/library/scala/collection/IndexedSeqView.scala

Lines 66 to 72 in 2131f5b

	override def drop(n: Int): Iterator[A] = {
	if (n > 0) {
	current += n
	remainder = Math.max(0, remainder - n)
	}
	this
	}

[som-snytt]

Roger that. Collections are more eager (so to speak) to override drop than take.

➜  grep "override def drop(" $(findsrc) | wc -l
26
➜  grep "override def take(" $(findsrc) | wc -l
17

In this context, I would have expected more symmetry, but I'll report again in the follow-up PR.