to(collection.IndexedSeq) should not create a new collection if the source collection is a collection.IndexedSeq

[Atry]

Welcome to Scala 2.13.1 (OpenJDK 64-Bit Server VM, Java 11.0.1).
Type in expressions for evaluation. Or try :help.

scala> val s = Array(1): collection.IndexedSeq[Int]
s: scala.collection.IndexedSeq[Int] = ArraySeq(1)

scala> s.to(collection.IndexedSeq) eq s // should be true
res0: Boolean = false

[julienrf]

I am not sure. My opinion is that it is probably safer to create a new collection if the source collection is a mutable collection. However, if the source collection is immutable we can do the optimization you are looking for: https://scastie.scala-lang.org/QcTwjN63RkyTHLgZSblZpw

val s = List(1): collection.Seq[Int]
s.to(collection.Seq) eq s // true

That being said, the current behavior should be clearly documented.

[Jasper-M]

I think that mutable collections always copy. Otherwise you get unpredictable behavior.

[Atry]

Suppose you need a function to create a sorted array from whatever collection, you might write down something like this:

def createSortedArrayFrom(iterable: Iterable[Int]): Array[Int] = {
  iterable.to(collection.Seq).view.sorted.toArray
}

Unfortunately, it performs the unnecessary element copy when the parameter is a collection.Seq, like createSortedArrayFrom(Array(3, 2, 4)). How can we provide an API to avoid the copy?

[Atry]

I wonder if we can make SeqView a Factory to support this use case:

def createSortedArrayFrom(iterable: Iterable[Int]): Array[Int] = {
  iterable.to(collection.SeqView).sorted.toArray
}

[dwijnand]

Otherwise you get unpredictable behavior.

Such as? I've wanted the behaviour described here too.

[julienrf]

@dwijnand Both behaviours make sense, but whatever behaviour is implemented it has to be clearly documented. In Scala 2.12 the to operation was also copying. If we change this now to aliasing instead of copying, we might break existing code relying on the old behaviour.

[sjrd]

In Scala 2.12, toSeq and toArray are definitely not copying, at least for Arrays:

$ scala
Welcome to Scala 2.12.8 (OpenJDK 64-Bit Server VM, Java 1.8.0_232).
Type in expressions for evaluation. Or try :help.

scala> val arr = Array(1, 2, 3)
arr: Array[Int] = Array(1, 2, 3)

scala> val s: Seq[Int] = arr
s: Seq[Int] = WrappedArray(1, 2, 3)

scala> val x = s.toArray
x: Array[Int] = Array(1, 2, 3)

scala> x eq arr
res0: Boolean = true

scala> arr.toSeq.toArray eq arr
res1: Boolean = true

[Jasper-M]

Such as? I've wanted the behaviour described here too.

import scala.collection.mutable

val x: collection.IndexedSeq[Int] = mutable.ArrayBuffer(1, 2, 3)
val a = x.to(mutable.ArrayBuffer)
val b = x.to(mutable.ArraySeq)

a(0) = 9
b(1) = 8
println(x) // ArrayBuffer(9, 1, 2) ???

Unpredictable in the sense that if I don't know the runtime type of x I can't predict whether or not mutating a or b will mutate x. If to would alias instead of copy for mutable collections.

[sjrd]

Same thing with ListBuffers:

scala> val b = scala.collection.mutable.ListBuffer.empty[Int]
b: scala.collection.mutable.ListBuffer[Int] = ListBuffer()

scala> b += 5
res3: b.type = ListBuffer(5)

scala> val s = b.toSeq
s: Seq[Int] = List(5)

scala> b += 4
res4: b.type = ListBuffer(5, 4)

scala> s
res5: Seq[Int] = List(5, 4)

Ouch ... did I just create a scala.collection.immutable.List that is actually not immutable!? That's bad.

[julienrf]

I wonder if we can make SeqView a Factory to support this use case:

def createSortedArrayFrom(iterable: Iterable[Int]): Array[Int] = {
  iterable.to(collection.SeqView).sorted.toArray
}

Maybe, yes, although I’m not sure how we should implement the SeqView for collections that are not Seqs (e.g. Set).

@sjrd

In Scala 2.12, toSeq and toArray are definitely not copying, at least for Arrays:

to[collection.Seq] did copy, but toSeq did not. There was an inconsistency.

[julienrf]

Ouch ... did I just create a scala.collection.immutable.List that is actually not immutable!? That's bad.

Good catch, that’s a bug!

[dwijnand]

Ouch ... did I just create a scala.collection.immutable.List that is actually not immutable!? That's bad.

Yeah, the fact that b.toSeq doesn't copy is really risky.

I'm not convinced that it's inconsistent to downcast without copying (as in Jasper's sc.Seq to ArrayBuffer example), but I'm vastly more interested in the immutable collection side of things.

[Jasper-M]

Ouch ... did I just create a scala.collection.immutable.List that is actually not immutable!? That's bad.

Unfortunately GitHub doesn't provide a "shocked to my very core" type of emoji reaction.