From 6e35c6ffdadacdac9fd026b9da72ad2028c264a3 Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Tue, 5 Oct 2021 16:07:52 +0200 Subject: [PATCH] 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. --- .../dotty/tools/dotc/core/Definitions.scala | 10 +-- .../src/dotty/tools/dotc/core/Types.scala | 3 +- .../tools/dotc/transform/ElimRepeated.scala | 85 ++++--------------- .../dotty/tools/dotc/transform/Erasure.scala | 7 ++ tests/run/java-varargs-3/A.java | 14 +++ tests/run/java-varargs-3/Test.scala | 19 +++++ 6 files changed, 65 insertions(+), 73 deletions(-) create mode 100644 tests/run/java-varargs-3/A.java create mode 100644 tests/run/java-varargs-3/Test.scala diff --git a/compiler/src/dotty/tools/dotc/core/Definitions.scala b/compiler/src/dotty/tools/dotc/core/Definitions.scala index 4a48eccffec9..60f375c116fc 100644 --- a/compiler/src/dotty/tools/dotc/core/Definitions.scala +++ b/compiler/src/dotty/tools/dotc/core/Definitions.scala @@ -376,18 +376,18 @@ class Definitions { * 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 diff --git a/compiler/src/dotty/tools/dotc/core/Types.scala b/compiler/src/dotty/tools/dotc/core/Types.scala index cd5df4f78614..1d36efcb2a01 100644 --- a/compiler/src/dotty/tools/dotc/core/Types.scala +++ b/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 diff --git a/compiler/src/dotty/tools/dotc/transform/ElimRepeated.scala b/compiler/src/dotty/tools/dotc/transform/ElimRepeated.scala index 71d814bb0a62..84a83a1b9b06 100644 --- a/compiler/src/dotty/tools/dotc/transform/ElimRepeated.scala +++ b/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 = diff --git a/compiler/src/dotty/tools/dotc/transform/Erasure.scala b/compiler/src/dotty/tools/dotc/transform/Erasure.scala index 3d9997642be0..2875e2e2d865 100644 --- a/compiler/src/dotty/tools/dotc/transform/Erasure.scala +++ b/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)) => diff --git a/tests/run/java-varargs-3/A.java b/tests/run/java-varargs-3/A.java new file mode 100644 index 000000000000..046592bb020e --- /dev/null +++ b/tests/run/java-varargs-3/A.java @@ -0,0 +1,14 @@ +class A { + public void gen(S... args) { + } + + public void gen2(S... args) { + } +} +class B { + public void gen(S... args) { + } + + public void gen2(S... args) { + } +} diff --git a/tests/run/java-varargs-3/Test.scala b/tests/run/java-varargs-3/Test.scala new file mode 100644 index 000000000000..4c00e491a6ad --- /dev/null +++ b/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("")*) + } +}