Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deal with special case for Array[Unit] in Array.{copyOf,copyAs} #6829

Merged
merged 2 commits into from
Aug 3, 2018

Conversation

retronym
Copy link
Member

No description provided.

@scala-jenkins scala-jenkins added this to the 2.13.0-M5 milestone Jun 20, 2018
@retronym retronym requested a review from szeiger June 20, 2018 09:12
@retronym
Copy link
Member Author

Thanks to @mkeskells for spotting that we were filling Arrry[Unit] with null-s.

@sjrd
Copy link
Member

sjrd commented Jun 20, 2018

That's not false, but fixing this seems like a drop in the ocean. There are many other cases where constructing an array of Units fills it with null. new Array[Unit](n), for starters:

$ ~/opt/scala-2.13.0-M4/bin/scala
Welcome to Scala 2.13.0-M4 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_40).
Type in expressions for evaluation. Or try :help.

scala> val a = new Array[Unit](5)
a: Array[Unit] = Array((), (), (), (), ()) // This is a lie

scala> a.asInstanceOf[Array[scala.runtime.BoxedUnit]](0)
res0: scala.runtime.BoxedUnit = null

The lie comes from ArraySeq.ofUnit.apply (or WrappedArray.ofUnit.apply in earlier versions), which returns a hard Unit, and therefore the result of array(index) is actually dropped, and a new BoxedUnit.UNIT is created in the bridge apply(int)Object generated to implement the inherited def apply(index: Int): T.

@@ -122,7 +122,9 @@ object Array {
* @see `java.util.Arrays#copyOf`
*/
def copyOf[A](original: Array[A], newLength: Int): Array[A] = (original match {
case x: Array[AnyRef] => java.util.Arrays.copyOf(x, newLength)
case x: Array[AnyRef] =>
if (x.getClass.getComponentType == classOf[BoxedUnit]) newUnitArray(newLength).asInstanceOf[Array[A]]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason not to use the usual match case x: Array[BoxedUnit]?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed to use that. The only tricky part is that it needs to precede the match on _: Array[AnyRef] due to array covariance in the VM.

@xuwei-k
Copy link
Contributor

xuwei-k commented Jul 19, 2018

BTW xuwei-k@a9761ba 🤔

@szeiger
Copy link
Contributor

szeiger commented Aug 3, 2018

@xuwei-k The JVM doesn't have void arrays, therefore Array[Unit] is really Array[BoxedUnit]. The pattern matcher would have to explicitly check for that case when matching on Array[AnyRef]. It's not exactly a common case, favoring correctness over performance seems reasonable. I suggest filing a bug.

@szeiger szeiger merged commit 29f6907 into scala:2.13.x Aug 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants