Handle Java varargs T... where T is a class parameter

Previously, ElimRepeated correctly handled Java varargs of the
form `Object...` and `T...` where `T` is a method parameter, in both
cases we erased them to `Array[? <: Object]` in the denotation
transformer and handled any adaptation in the tree transformer of
ElimRepeated.

Unfortunately, the denotation transformer logic failed to account for
class type parameters, and fixing it introduced issues in override
checking (RefChecks happens after ElimRepeated). So this commit gives up
and delay the adaptation of primitive arrays into reference arrays until
Erasure. This is less efficient in some situations but is closer to what
Scala 2 does so hopefully means we won't run into more pitfalls.

Fixes #13645.

Author: smarter

compiler/src/dotty/tools/dotc/core/Definitions.scala

@@ -376,18 +376,18 @@ class Definitions {
    *       <T> void meth8(T... args) {}
    *
    *       // B.scala
-   *       meth7(1) // OK
-   *       meth8(1) // OK
+   *       meth7(1) // OK (creates a reference array)
+   *       meth8(1) // OK (creates a primitive array and copies it into a reference array at Erasure)
    *       val ai = Array[Int](1)
-   *       meth7(ai: _*) // OK (will copy the array)
-   *       meth8(ai: _*) // OK (will copy the array)
+   *       meth7(ai: _*) // OK (will copy the array at Erasure)
+   *       meth8(ai: _*) // OK (will copy the array at Erasure)
    *
    *     Java repeated arguments are erased to arrays, so it would be safe to treat
    *     them in the same way: add an `& Object` to the parameter type to disallow
    *     passing primitives, but that would be very inconvenient as it is common to
    *     want to pass a primitive to an Object repeated argument (e.g.
    *     `String.format("foo: %d", 1)`). So instead we type them _without_ adding the
-   *     `& Object` and let `ElimRepeated` take care of doing any necessary adaptation
+   *     `& Object` and let `ElimRepeated` and `Erasure` take care of doing any necessary adaptation
    *     (note that adapting a primitive array to a reference array requires
    *     copying the whole array, so this transformation only preserves semantics
    *     if the callee does not try to mutate the varargs array which is a reasonable

compiler/src/dotty/tools/dotc/core/Types.scala

@@ -299,7 +299,8 @@ object Types {
       loop(this)
     }
 
-    def isFromJavaObject(using Context): Boolean = typeSymbol eq defn.FromJavaObjectSymbol
+    def isFromJavaObject(using Context): Boolean =
+      isRef(defn.ObjectClass) && (typeSymbol eq defn.FromJavaObjectSymbol)
 
     def containsFromJavaObject(using Context): Boolean = this match
       case tp: OrType => tp.tp1.containsFromJavaObject || tp.tp2.containsFromJavaObject

compiler/src/dotty/tools/dotc/transform/ElimRepeated.scala

@@ -114,22 +114,7 @@ class ElimRepeated extends MiniPhase with InfoTransformer { thisPhase =>
         if lastIdx >= 0 then
           val last = paramTypes(lastIdx)
           if last.isRepeatedParam then
-            // We need to be careful when handling Java repeated parameters
-            // of the form `Object...` or `T...` where `T` is unbounded:
-            // in both cases, `Object` will have been translated to `FromJavaObject`
-            // to allow passing primitives as repeated arguments, but we can't
-            // pass a primitive array as argument to such a method since the
-            // parameter will be erased to `Object[]`. To handle this correctly we
-            // drop usage of `FromJavaObject` as an element type here, the
-            // tree transformer of this phase is then responsible for handling
-            // mismatches by emitting the correct adaptation (cf `adaptToArray`).
-            // See also the documentation of `FromJavaObjectSymbol`.
-            val last1 =
-              if isJava && last.elemType.isFromJavaObject then
-                defn.ArrayOf(TypeBounds.upper(defn.ObjectType))
-              else
-                last.translateFromRepeated(toArray = isJava)
-            paramTypes.updated(lastIdx, last1)
+            paramTypes.updated(lastIdx, last.translateFromRepeated(toArray = isJava))
           else paramTypes
         else paramTypes
       tp.derivedLambdaType(paramNames, paramTypes1, resultType1)
@@ -144,8 +129,7 @@ class ElimRepeated extends MiniPhase with InfoTransformer { thisPhase =>
         val isJavaDefined = tree.fun.symbol.is(JavaDefined)
         val tpe = arg.expr.tpe
         if isJavaDefined then
-          val pt = tree.fun.tpe.widen.firstParamTypes.last
-          adaptToArray(arg.expr, pt.elemType.bounds.hi)
+          adaptToArray(arg.expr)
         else if tpe.derivesFrom(defn.ArrayClass) then
           arrayToSeq(arg.expr)
         else
@@ -154,58 +138,25 @@ class ElimRepeated extends MiniPhase with InfoTransformer { thisPhase =>
     }
     cpy.Apply(tree)(tree.fun, args)
 
-  /** Convert sequence argument to Java array */
-  private def seqToArray(tree: Tree)(using Context): Tree = tree match
+  private def adaptToArray(tree: Tree)(implicit ctx: Context): Tree = tree match
     case SeqLiteral(elems, elemtpt) =>
-      JavaSeqLiteral(elems, elemtpt)
+      JavaSeqLiteral(elems, elemtpt).withSpan(tree.span)
     case _ =>
-      val elemType = tree.tpe.elemType
-      var elemClass = erasure(elemType).classSymbol
-      if defn.NotRuntimeClasses.contains(elemClass) then
-        elemClass = defn.ObjectClass
-      end if
-      ref(defn.DottyArraysModule)
-        .select(nme.seqToArray)
-        .appliedToType(elemType)
-        .appliedTo(tree, clsOf(elemClass.typeRef))
-
-  /** Adapt a Seq or Array tree to be a subtype of `Array[_ <: $elemPt]`.
-   *
-   *  @pre `elemPt` must either be a super type of the argument element type or `Object`.
-   *        The special handling of `Object` is required to deal with the translation
-   *        of generic Java varargs in `elimRepeated`.
-   */
-  private def adaptToArray(tree: Tree, elemPt: Type)(implicit ctx: Context): Tree =
-    val elemTp = tree.tpe.elemType
-    val elemTpMatches = elemTp <:< elemPt
-    val treeIsArray = tree.tpe.derivesFrom(defn.ArrayClass)
-    if elemTpMatches && treeIsArray then
-      tree // No adaptation necessary
-    else tree match
-      case SeqLiteral(elems, elemtpt) =>
-        // By the precondition, we only have mismatches if elemPt is Object, in
-        // that case we use `FromJavaObject` as the element type to allow the
-        // sequence literal to typecheck no matter the types of the elements,
-        // Erasure will take care of any necessary boxing (see documentation
-        // of `FromJavaObjectSymbol` for more information).
-        val adaptedElemTpt = if elemTpMatches then elemtpt else TypeTree(defn.FromJavaObjectType)
-        JavaSeqLiteral(elems, adaptedElemTpt).withSpan(tree.span)
-      case _ =>
-        if treeIsArray then
-          // Convert an Array[T] to an Array[Object]
-          ref(defn.ScalaRuntime_toObjectArray)
-            .appliedTo(tree)
-        else if elemTpMatches then
-          // Convert a Seq[T] to an Array[$elemPt]
-          ref(defn.DottyArraysModule)
-            .select(nme.seqToArray)
-            .appliedToType(elemPt)
-            .appliedTo(tree, clsOf(elemPt))
+      val elemTp = tree.tpe.elemType
+      val adapted =
+        if tree.tpe.derivesFrom(defn.ArrayClass) then
+          tree
         else
-          // Convert a Seq[T] to an Array[Object]
-          ref(defn.ScalaRuntime_toArray)
-            .appliedToType(elemTp)
-            .appliedTo(tree)
+          ref(defn.DottyArraysModule)
+          .select(nme.seqToArray)
+          .appliedToType(elemTp)
+          .appliedTo(tree, clsOf(elemTp))
+      // This seemingly redundant type ascription is needed because the result
+      // type of `adapted` might be erased to `Object`, but we need to keep
+      // the precise result type at erasure for `Erasure.Boxing.cast` to adapt
+      // a primitive array into a reference array if needed.
+      // Test case in tests/run/t1360.scala.
+      Typed(adapted, TypeTree(defn.ArrayOf(elemTp)))
 
   /** Convert an Array into a scala.Seq */
   private def arrayToSeq(tree: Tree)(using Context): Tree =

compiler/src/dotty/tools/dotc/transform/Erasure.scala

@@ -325,6 +325,13 @@ object Erasure {
       assert(!pt.isInstanceOf[SingletonType], pt)
       if (pt isRef defn.UnitClass) unbox(tree, pt)
       else (tree.tpe.widen, pt) match {
+        // Convert primitive arrays into reference arrays, this path is only
+        // needed to handle repeated arguments, see
+        // `Definitions#FromJavaObjectSymbol` and `ElimRepeated#adaptToArray`.
+        case (JavaArrayType(treeElem), JavaArrayType(ptElem))
+        if treeElem.widen.isPrimitiveValueType && !ptElem.isPrimitiveValueType =>
+          cast(ref(defn.ScalaRuntime_toObjectArray).appliedTo(tree), pt)
+
         // When casting between two EVTs, we need to check which one underlies the other to determine
         // whether u2evt or evt2u should be used.
         case (tp1 @ ErasedValueType(tycon1, underlying1), tp2 @ ErasedValueType(tycon2, underlying2)) =>

tests/run/java-varargs-3/A.java

@@ -0,0 +1,14 @@
+class A<S> {
+  public void gen(S... args) {
+  }
+
+  public <T extends S> void gen2(S... args) {
+  }
+}
+class B<S extends java.io.Serializable> {
+  public void gen(S... args) {
+  }
+
+  public <T extends S> void gen2(S... args) {
+  }
+}

tests/run/java-varargs-3/Test.scala

@@ -0,0 +1,19 @@
+object Test {
+  def main(args: Array[String]): Unit = {
+    val ai = new A[Int]
+    ai.gen(1)
+    ai.gen2(1)
+    ai.gen(Array(1)*)
+    ai.gen2(Array(1)*)
+    ai.gen(Seq(1)*)
+    ai.gen2(Seq(1)*)
+
+    val b = new B[String]
+    b.gen("")
+    b.gen2("")
+    b.gen(Array("")*)
+    b.gen2(Array("")*)
+    b.gen(Seq("")*)
+    b.gen2(Seq("")*)
+  }
+}