bug icon indicating copy to clipboard operation
bug copied to clipboard

Array.apply optimization in Cleanup can currently CCE and could be extended to generic (not `<: AnyRef`) arrays

Open retronym opened this issue 5 years ago • 3 comments

The current optimization is unsound:

scala> def foo[T : reflect.ClassTag](t: T) = Array.apply[T](t); foo[String]("")
def foo[T](t: T)(implicit evidence$1: scala.reflect.ClassTag[T]): Array[T]
val res13: Array[String] = Array("")

scala> def foo[T <: AnyRef : reflect.ClassTag](t: T) = Array.apply[T](t); foo[String]("")
java.lang.ClassCastException: [Ljava.lang.Object; cannot be cast to [Ljava.lang.String;
  ... 32 elided

This should be fixed by limiting it to Array.apply applications that had a concrete type. I think this can be done by detecting ApplyImplicitArgs(Array.apply(....)(<implicitly materialed class tag>).

In cases when this isn't the case, we could still avoid temporary array by code generating {val arr = classTag.newArray(N); ScalaRunTime.arrayUpdate(arr, N, elemN); ...; arr}. Then, both variations below could avoid a temporary array and collection wrapper.

import scala.reflect.ClassTag


class OptimusSeq[T]

object OptimusSeq {
  private def unsafeFromAnyArray1[T <: AnyRef](ts: Array[T]): OptimusSeq[T] = null;
  def apply1[T <: AnyRef : ClassTag](p1: T, p2: T, p3: T, p4: T): OptimusSeq[T] = {
    unsafeFromAnyArray2(Array(p1, p2, p3, p4))
  }


private def unsafeFromAnyArray2[T](ts: Array[T]): OptimusSeq[T] = null;
  def apply2[T : ClassTag](p1: T, p2: T, p3: T, p4: T): OptimusSeq[T] = {
    unsafeFromAnyArray2(Array(p1, p2, p3, p4))
  }
}
[[syntax trees at end of                   erasure]] // test.scala
package <empty> {
  class OptimusSeq extends Object {
    def <init>(): OptimusSeq = {
      OptimusSeq.super.<init>();
      ()
    }
  };
  object OptimusSeq extends Object {
    def <init>(): OptimusSeq.type = {
      OptimusSeq.super.<init>();
      ()
    };
    private def unsafeFromAnyArray1(ts: Array[Object]): OptimusSeq = null;
    def apply1(p1: Object, p2: Object, p3: Object, p4: Object, evidence$1: scala.reflect.ClassTag): OptimusSeq = OptimusSeq.this.unsafeFromAnyArray2(scala.Array.apply(scala.Predef.wrapRefArray(Array[Object]{p1, p2, p3, p4}), evidence$1));
    private def unsafeFromAnyArray2(ts: Object): OptimusSeq = null;
    def apply2(p1: Object, p2: Object, p3: Object, p4: Object, evidence$2: scala.reflect.ClassTag): OptimusSeq = OptimusSeq.this.unsafeFromAnyArray2(scala.Array.apply(scala.Predef.genericWrapArray(Array[Object]{p1, p2, p3, p4}), evidence$2))
  }
}

[[syntax trees at end of                   cleanup]] // test.scala
package <empty> {
  class OptimusSeq extends Object {
    def <init>(): OptimusSeq = {
      OptimusSeq.super.<init>();
      ()
    }
  };
  object OptimusSeq extends Object {
    private def unsafeFromAnyArray1(ts: Array[Object]): OptimusSeq = null;
    def apply1(p1: Object, p2: Object, p3: Object, p4: Object, evidence$1: scala.reflect.ClassTag): OptimusSeq = OptimusSeq.this.unsafeFromAnyArray2(Array[Object]{p1, p2, p3, p4});
    private def unsafeFromAnyArray2(ts: Object): OptimusSeq = null;
    def apply2(p1: Object, p2: Object, p3: Object, p4: Object, evidence$2: scala.reflect.ClassTag): OptimusSeq = OptimusSeq.this.unsafeFromAnyArray2(scala.Array.apply(scala.Predef.genericWrapArray(Array[Object]{p1, p2, p3, p4}), evidence$2));
    def <init>(): OptimusSeq.type = {
      OptimusSeq.super.<init>();
      ()
    }
  }
}

retronym avatar Feb 11 '20 00:02 retronym

We've got the same limitation in Scala.js. we have a progression test for it: https://github.com/scala-js/scala-js/blob/cb6d935fa15b92b5a653f9eb716606e7ba7ee8e4/compiler/src/test/scala/org/scalajs/nscplugin/test/OptimizationTest.scala#L48-L67

sjrd avatar Feb 11 '20 04:02 sjrd

@retronym did anything further ever happen here?

dwijnand avatar Nov 23 '20 17:11 dwijnand

@dwijnand No, I haven't fixed this one yet. It's pretty standalone if you'd like to get your hands dirty in the Cleanup phase.

retronym avatar Nov 24 '20 03:11 retronym

https://github.com/scala/scala/pull/9356

som-snytt avatar Nov 15 '23 03:11 som-snytt