Inconsistent `copyToArray` semantics between `IterableOnceOps` and `Vector` (and some other collections).

[noresttherein]

Reproduction steps

Scala version: 2.13.12 I made another attempt to bring semantics of my collections exactly in line with the standard library and failed miserably. I hinted at it #12795, but it gets worse.

	println(Vector(1).copyToArray(new Array[Int](1), Int.MinValue, 1))
	println(List(1).copyToArray(new Array[Int](1), Int.MinValue, 1))

Problem

0
Exception in thread "main" java.lang.ArrayIndexOutOfBoundsException: Index -2147483648 out of bounds for length 20268
	at scala.runtime.ScalaRunTime$.array_update(ScalaRunTime.scala:75)
	at scala.collection.IterableOnceOps.copyToArray(IterableOnce.scala:926)
	at scala.collection.IterableOnceOps.copyToArray$(IterableOnce.scala:921)
	at scala.collection.AbstractIterable.copyToArray(Iterable.scala:933)
	at Playground$.delayedEndpoint$Playground$1(Playground.scala:20)
	at Playground$delayedInit$body.apply(Playground.scala:8)

Coincidentally,

	println(List(1).copyToArray(new Array[Int](1), -1, -1))
	println(List(1).copyToArray(new Array[Int](1), Int.MinValue, -1))

yields the same result:

0
Exception in thread "main" java.lang.ArrayIndexOutOfBoundsException: Index -2147483648 out of bounds for length 1
	at scala.runtime.ScalaRunTime$.array_update(ScalaRunTime.scala:75)
	at scala.collection.IterableOnceOps.copyToArray(IterableOnce.scala:926)
	at scala.collection.IterableOnceOps.copyToArray$(IterableOnce.scala:921)
	at scala.collection.AbstractIterable.copyToArray(Iterable.scala:933)
	at Playground$.delayedEndpoint$Playground$1(Playground.scala:22)
	at Playground$delayedInit$body.apply(Playground.scala:8)

I know one probably has to be autistic to be bothered by it enough to fix it, given limited resources, but is there an official policy of what should happen in both of these scenarios that I can adopt? `` <soapbox>These are exactly consequences of the philosophy of deriving API semantics from the simplest possible implementation, rather than the simplest possible specification, as defended in one of my past bugs</soapbox>. I don't like that negative start index is accepted if the amount to copy is zero, but at least collections using IterableOnce.elemsToCopyToArray are somewhat consistent in it. I champion 'always reject negative index' cause in large part because it guards the rest of the code from an underflow.

[SethTisue]

@scala/collections

[som-snytt]

If "my collections" are a public repo, could you link? I'd be interested in at least following along to appreciate the use cases.

[Ichoran]

I think the most consistent way to handle this, given the behavior of other slice-of-array type of operations, is that mis-indexing into arrays throws an exception, if there's any mis-indexing to do.

So if you index outside of the array, and you copy anything at all, you get an exception. If you copy nothing, you're fine.

So xs.copyToArray(a, i, n) should exactly match the behavior of xs.take(n).zipWithIndex.foreach{ case (x, j) => a(i+j) = x }. This seems the simplest to me, anyway.

[som-snytt]

I can never tell if Ichoran gives the simplest possible implementation or specification or both, but I agree.

So this is just an implementation bug. The Scaladoc is clear that len is "maximal" count and copying stops when it is reached, which is trivially true for negative len. (edit: that was the second case; the first case is negative start not throwing)

I sympathize with OP about "always check inputs and fail early", but I also agree with Ichoran that if we're paying for index checks for array access anyway, we should just rely on it. The other argument, about forgiveness in API, is more design philosophy.

[noresttherein]

I think the most consistent way to handle this, given the behavior of other slice-of-array type of operations, is that mis-indexing into arrays throws an exception, if there's any mis-indexing to do.

So if you index outside of the array, and you copy anything at all, you get an exception. If you copy nothing, you're fine.

So xs.copyToArray(a, i, n) should exactly match the behavior of xs.take(n).zipWithIndex.foreach{ case (x, j) => a(i+j) = x }. This seems the simplest to me, anyway.

Currently, IndexOutOfBoundsException is thrown also if the array itself is of zero length. This is kind of inconsistent, and arguably against the documentation, but at least it seems to work that way in all collections.

[som-snytt]

https://github.com/scala/bug/issues/11048 on the policy of not throwing on no work.

[som-snytt]

Thanks for the report and the solution. The PR includes the zero-length destination case. Let me know if there's more.