Skip to content

Commit

Permalink
[#11600] Vector.from(ArraySeq) copies elems rather than reusing
Browse files Browse the repository at this point in the history
unsafeArray

There were issues with reusing the ArraySeq unsafeArray which arose in
subsequent calls to Vector#updated which would then clone underlying
arrays, and insert into them the new updated element. However the
underlying array.clone() would return a new array with the same
componentType as the passed varargs, rather than an Array[AnyRef] which
is required in the general case.
  • Loading branch information
joshlemer committed Jul 2, 2019
1 parent e89bdde commit 6a73793
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 11 deletions.
28 changes: 17 additions & 11 deletions src/library/scala/collection/immutable/Vector.scala
Expand Up @@ -33,8 +33,20 @@ object Vector extends StrictOptimizedSeqFactory[Vector] {
case as: ArraySeq[E] if as.length <= 32 && as.unsafeArray.isInstanceOf[Array[AnyRef]] =>
if (as.isEmpty) NIL
else {
val v = new Vector(0, as.length, 0)
v.display0 = as.unsafeArray.asInstanceOf[Array[AnyRef]]
val unsafeArray = as.unsafeArray
val len = unsafeArray.length
val v = new Vector(0, len, 0)
val display0 = new Array[Any](len)
if (unsafeArray.isInstanceOf[Array[AnyRef]]) {
System.arraycopy(unsafeArray, 0, display0, 0, len)
} else {
var i = 0
while (i < len) {
display0(i) = unsafeArray(i)
i += 1
}
}
v.display0 = display0.asInstanceOf[Array[AnyRef]]
v.depth = 1
releaseFence()
v
Expand All @@ -45,17 +57,11 @@ object Vector extends StrictOptimizedSeqFactory[Vector] {

if (knownSize == 0) empty[E]
else if (knownSize > 0 && knownSize <= 32) {
val display0 = new Array[AnyRef](knownSize)

var i = 0
val iterator = it.iterator
while (iterator.hasNext) {
display0(i) = iterator.next().asInstanceOf[AnyRef]
i += 1
}
val display0 = new Array[Any](knownSize)
it.iterator.copyToArray(display0)
val v = new Vector[E](0, knownSize, 0)
v.depth = 1
v.display0 = display0
v.display0 = display0.asInstanceOf[Array[AnyRef]]
releaseFence()
v
} else {
Expand Down
44 changes: 44 additions & 0 deletions test/junit/scala/collection/immutable/VectorTest.scala
Expand Up @@ -219,4 +219,48 @@ class VectorTest {
assertArrayEquals(s"<${v2.length}>.slice($j, $k)", v2.toArray.slice(j, k), v2.iterator.slice(j, k).toArray)
}
}

@Test
def i11600(): Unit = {
locally {
abstract class Base
class Derived1 extends Base
class Derived2 extends Base
val d1 = new Derived1
val d2 = new Derived2

locally {
val arraySeq = ArraySeq(d1)
val vector = Vector(arraySeq: _*)
assertEquals(arraySeq, ArraySeq(d1)) // ensure arraySeq is not mutated
assertEquals(vector.updated(0, d2), Vector(d2))
}

locally {
val list = List(d1)
val vector = Vector.from(list)
assertEquals(list, vector)
assertEquals(List(d2), vector.updated(0, d2))
}
}

locally {
// ensure boxing logic works:
val arraySeq = ArraySeq(1,2,3,4,5)
val vector = Vector(arraySeq: _*)

assertEquals(1 to 5, vector)
assertEquals(vector.updated(0, 20), Vector(20,2,3,4,5))
assertEquals(vector.updated(0, ""), Vector("",2,3,4,5))
assertEquals(1 to 5, arraySeq) // ensure arraySeq is not mutated
}
locally {
// ensure boxing logic works:
val arr = Array(1)
val vector = Vector.from(arr)
assertEquals(arr.toList, vector)
assertEquals(List(20), vector.updated(0, 20))
assertEquals(List(""), vector.updated(0, ""))
}
}
}

0 comments on commit 6a73793

Please sign in to comment.