From bcbc66eb3cca661e939eaf26fc97ebf2e7dbc60b Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Mon, 27 Jul 2020 18:33:34 +0200 Subject: [PATCH 1/2] Parse varargs in a more direct manner In bytecode, vararg parameters are encoded as arrays with the ACC_VARARGS flag set on the method. Previously we first parsed them as arrays then translated the array into a repeated param. With this commit we directly parse them as repeated params. This isn't really any simpler since we need to keep track of the fact that we're in a varargs method, but it will become useful in the next commit where we start treating arrays differently from varargs (`T[]` will still be translated as `Array[T & Object]` whereas `T...` will be translated as `T*` and handled specially in ElimRepeated to preserve soundness). Similarly for joint compilation, we now directly desugar varargs as `RepeatedParam[T]` instead of `Array[T] @Repeated`, this doesn't change anything currently because: - When giving a type to the method, we call `annotatedToRepeated` on each parameter type - `typedAppliedTypeTree` takes care of adding `& Object` for both `Array[T]` and `RepeatedParam[T]` coming From Java (this is what the next commit will change). --- .../src/dotty/tools/dotc/ast/Desugar.scala | 10 ++-- .../dotc/core/classfile/ClassfileParser.scala | 54 ++++++++++++++----- .../core/unpickleScala2/Scala2Unpickler.scala | 15 ------ 3 files changed, 46 insertions(+), 33 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/ast/Desugar.scala b/compiler/src/dotty/tools/dotc/ast/Desugar.scala index 9c27443f54cf..730dea84919e 100644 --- a/compiler/src/dotty/tools/dotc/ast/Desugar.scala +++ b/compiler/src/dotty/tools/dotc/ast/Desugar.scala @@ -1684,10 +1684,12 @@ object desugar { Apply(Select(Apply(scalaDot(nme.StringContext), strs), id).withSpan(tree.span), elems) case PostfixOp(t, op) => if ((ctx.mode is Mode.Type) && !isBackquoted(op) && op.name == tpnme.raw.STAR) { - val seqType = if (ctx.compilationUnit.isJava) defn.ArrayType else defn.SeqType - Annotated( - AppliedTypeTree(ref(seqType), t), - New(ref(defn.RepeatedAnnot.typeRef), Nil :: Nil)) + if ctx.compilationUnit.isJava then + AppliedTypeTree(ref(defn.RepeatedParamType), t) + else + Annotated( + AppliedTypeTree(ref(defn.SeqType), t), + New(ref(defn.RepeatedAnnot.typeRef), Nil :: Nil)) } else { assert(ctx.mode.isExpr || ctx.reporter.errorsReported || ctx.mode.is(Mode.Interactive), ctx.mode) diff --git a/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala b/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala index e22e3e44a203..6aa1835b357a 100644 --- a/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala +++ b/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala @@ -280,15 +280,13 @@ class ClassfileParser( addConstructorTypeParams(denot) } - denot.info = pool.getType(in.nextChar) + val isVarargs = denot.is(Flags.Method) && (jflags & JAVA_ACC_VARARGS) != 0 + denot.info = pool.getType(in.nextChar, isVarargs) if (isEnum) denot.info = ConstantType(Constant(sym)) if (isConstructor) normalizeConstructorParams() - denot.info = translateTempPoly(parseAttributes(sym, denot.info)) + denot.info = translateTempPoly(parseAttributes(sym, denot.info, isVarargs)) if (isConstructor) normalizeConstructorInfo() - if (denot.is(Flags.Method) && (jflags & JAVA_ACC_VARARGS) != 0) - denot.info = arrayToRepeated(denot.info) - if (ctx.explicitNulls) denot.info = JavaNullInterop.nullifyMember(denot.symbol, denot.info, isEnum) // seal java enums @@ -324,7 +322,7 @@ class ClassfileParser( case BOOL_TAG => defn.BooleanType } - private def sigToType(sig: SimpleName, owner: Symbol = null)(using Context): Type = { + private def sigToType(sig: SimpleName, owner: Symbol = null, isVarargs: Boolean = false)(using Context): Type = { var index = 0 val end = sig.length def accept(ch: Char): Unit = { @@ -395,13 +393,41 @@ class ClassfileParser( val elemtp = sig2type(tparams, skiptvs) defn.ArrayOf(elemtp.translateJavaArrayElementType) case '(' => - // we need a method symbol. given in line 486 by calling getType(methodSym, ..) + def isMethodEnd(i: Int) = sig(i) == ')' + def isArray(i: Int) = sig(i) == '[' + + /** Is this a repeated parameter type? + * This is true if we're in a vararg method and this is the last parameter. + */ + def isRepeatedParam(i: Int): Boolean = + if !isVarargs then return false + var cur = i + // Repeated parameters are represented as arrays + if !isArray(cur) then return false + // Handle nested arrays: int[]... + while isArray(cur) do + cur += 1 + // Simple check to see if we're the last parameter: there should be no + // array in the signature until the method end. + while !isMethodEnd(cur) do + if isArray(cur) then return false + cur += 1 + true + end isRepeatedParam + val paramtypes = new ListBuffer[Type]() var paramnames = new ListBuffer[TermName]() - while (sig(index) != ')') { + while !isMethodEnd(index) do paramnames += nme.syntheticParamName(paramtypes.length) - paramtypes += objToAny(sig2type(tparams, skiptvs)) - } + paramtypes += { + if isRepeatedParam(index) then + index += 1 + val elemType = sig2type(tparams, skiptvs) + defn.RepeatedParamType.appliedTo(elemType.translateJavaArrayElementType) + else + objToAny(sig2type(tparams, skiptvs)) + } + index += 1 val restype = sig2type(tparams, skiptvs) JavaMethodType(paramnames.toList, paramtypes.toList, restype) @@ -574,7 +600,7 @@ class ClassfileParser( None // ignore malformed annotations } - def parseAttributes(sym: Symbol, symtype: Type)(using Context): Type = { + def parseAttributes(sym: Symbol, symtype: Type, isVarargs: Boolean = false)(using Context): Type = { var newType = symtype def parseAttribute(): Unit = { @@ -584,7 +610,7 @@ class ClassfileParser( attrName match { case tpnme.SignatureATTR => val sig = pool.getExternalName(in.nextChar) - newType = sigToType(sig, sym) + newType = sigToType(sig, sym, isVarargs) if (ctx.debug && ctx.verbose) println("" + sym + "; signature = " + sig + " type = " + newType) case tpnme.SyntheticATTR => @@ -1103,8 +1129,8 @@ class ClassfileParser( c } - def getType(index: Int)(using Context): Type = - sigToType(getExternalName(index)) + def getType(index: Int, isVarargs: Boolean = false)(using Context): Type = + sigToType(getExternalName(index), isVarargs = isVarargs) def getSuperClass(index: Int)(using Context): Symbol = { assert(index != 0, "attempt to parse java.lang.Object from classfile") diff --git a/compiler/src/dotty/tools/dotc/core/unpickleScala2/Scala2Unpickler.scala b/compiler/src/dotty/tools/dotc/core/unpickleScala2/Scala2Unpickler.scala index 17f6731ffe49..956cd0380712 100644 --- a/compiler/src/dotty/tools/dotc/core/unpickleScala2/Scala2Unpickler.scala +++ b/compiler/src/dotty/tools/dotc/core/unpickleScala2/Scala2Unpickler.scala @@ -59,21 +59,6 @@ object Scala2Unpickler { denot.info = PolyType.fromParams(denot.owner.typeParams, denot.info) } - /** Convert array parameters denoting a repeated parameter of a Java method - * to `RepeatedParamClass` types. - */ - def arrayToRepeated(tp: Type)(using Context): Type = tp match { - case tp: MethodType => - val lastArg = tp.paramInfos.last - assert(lastArg isRef defn.ArrayClass) - tp.derivedLambdaType( - tp.paramNames, - tp.paramInfos.init :+ lastArg.translateParameterized(defn.ArrayClass, defn.RepeatedParamClass), - tp.resultType) - case tp: PolyType => - tp.derivedLambdaType(tp.paramNames, tp.paramInfos, arrayToRepeated(tp.resultType)) - } - def ensureConstructor(cls: ClassSymbol, scope: Scope)(using Context): Unit = { if (scope.lookup(nme.CONSTRUCTOR) == NoSymbol) { val constr = newDefaultConstructor(cls) From 7e689625abffefb520a53da9b8339d9d1b4eba7c Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Mon, 27 Jul 2020 20:09:26 +0200 Subject: [PATCH 2/2] Translate Java varargs `...T` into `T*` instead of `(T & Object)*` This is much more convenient for users but is more complicated for the compiler since we might still need to translate the varargs into an `Object[]` in bytecode. Since ElimRepeated was already responsible for doing some adaptation of repeated arguments, it now also takes care of this (unlike Scala 2 where this is handled in Erasure). Fixes #9439. --- .../dotty/tools/dotc/core/Definitions.scala | 2 + .../dotc/core/classfile/ClassfileParser.scala | 3 +- .../tools/dotc/transform/ElimRepeated.scala | 71 ++++++++++++++++--- .../src/dotty/tools/dotc/typer/Typer.scala | 2 +- tests/neg/i533/Test.scala | 2 +- tests/pos/arrays2.scala | 1 + tests/run/i9439.scala | 17 +++++ tests/run/java-varargs-2/A.java | 10 +++ tests/run/java-varargs-2/Test.scala | 13 ++++ tests/run/java-varargs/A_1.java | 3 + tests/run/java-varargs/Test_2.scala | 5 ++ 11 files changed, 117 insertions(+), 12 deletions(-) create mode 100644 tests/run/i9439.scala create mode 100644 tests/run/java-varargs-2/A.java create mode 100644 tests/run/java-varargs-2/Test.scala diff --git a/compiler/src/dotty/tools/dotc/core/Definitions.scala b/compiler/src/dotty/tools/dotc/core/Definitions.scala index 612a5ce86e21..f3c226c5ce74 100644 --- a/compiler/src/dotty/tools/dotc/core/Definitions.scala +++ b/compiler/src/dotty/tools/dotc/core/Definitions.scala @@ -399,6 +399,8 @@ class Definitions { def runtimeMethodRef(name: PreName): TermRef = ScalaRuntimeModule.requiredMethodRef(name) def ScalaRuntime_drop: Symbol = runtimeMethodRef(nme.drop).symbol @tu lazy val ScalaRuntime__hashCode: Symbol = ScalaRuntimeModule.requiredMethod(nme._hashCode_) + @tu lazy val ScalaRuntime_toArray: Symbol = ScalaRuntimeModule.requiredMethod(nme.toArray) + @tu lazy val ScalaRuntime_toObjectArray: Symbol = ScalaRuntimeModule.requiredMethod(nme.toObjectArray) @tu lazy val BoxesRunTimeModule: Symbol = requiredModule("scala.runtime.BoxesRunTime") @tu lazy val BoxesRunTimeModule_externalEquals: Symbol = BoxesRunTimeModule.info.decl(nme.equals_).suchThat(toDenot(_).info.firstParamTypes.size == 2).symbol diff --git a/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala b/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala index 6aa1835b357a..94b40ab84e80 100644 --- a/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala +++ b/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala @@ -423,7 +423,8 @@ class ClassfileParser( if isRepeatedParam(index) then index += 1 val elemType = sig2type(tparams, skiptvs) - defn.RepeatedParamType.appliedTo(elemType.translateJavaArrayElementType) + // `ElimRepeated` is responsible for correctly erasing this. + defn.RepeatedParamType.appliedTo(elemType) else objToAny(sig2type(tparams, skiptvs)) } diff --git a/compiler/src/dotty/tools/dotc/transform/ElimRepeated.scala b/compiler/src/dotty/tools/dotc/transform/ElimRepeated.scala index 4b48b649f278..9911544bef65 100644 --- a/compiler/src/dotty/tools/dotc/transform/ElimRepeated.scala +++ b/compiler/src/dotty/tools/dotc/transform/ElimRepeated.scala @@ -20,8 +20,9 @@ object ElimRepeated { val name: String = "elimRepeated" } -/** A transformer that removes repeated parameters (T*) from all types, replacing - * them with Seq types. +/** A transformer that eliminates repeated parameters (T*) from all types, replacing + * them with Seq or Array types and adapting repeated arguments to conform to + * the transformed type if needed. */ class ElimRepeated extends MiniPhase with InfoTransformer { thisPhase => import ast.tpd._ @@ -55,9 +56,28 @@ class ElimRepeated extends MiniPhase with InfoTransformer { thisPhase => case tp @ MethodTpe(paramNames, paramTypes, resultType) => val resultType1 = elimRepeated(resultType) val paramTypes1 = - if paramTypes.nonEmpty && paramTypes.last.isRepeatedParam then - val last = paramTypes.last.translateFromRepeated(toArray = tp.isJavaMethod) - paramTypes.init :+ last + val lastIdx = paramTypes.length - 1 + if lastIdx >= 0 then + val last = paramTypes(lastIdx) + if last.isRepeatedParam then + val isJava = tp.isJavaMethod + // A generic Java varargs `T...` where `T` is unbounded is erased to + // `Object[]` in bytecode, we directly translate such a type to + // `Array[_ <: Object]` instead of `Array[_ <: T]` here. This allows + // the tree transformer of this phase to emit the correct adaptation + // for repeated arguments if needed (for example, an `Array[Int]` will + // be copied into an `Array[Object]`, see `adaptToArray`). + val last1 = + if isJava && { + val elemTp = last.elemType + elemTp.isInstanceOf[TypeParamRef] && elemTp.typeSymbol == defn.AnyClass + } + then + defn.ArrayOf(TypeBounds.upper(defn.ObjectType)) + else + last.translateFromRepeated(toArray = isJava) + paramTypes.updated(lastIdx, last1) + else paramTypes else paramTypes tp.derivedLambdaType(paramNames, paramTypes1, resultType1) case tp: PolyType => @@ -82,9 +102,10 @@ class ElimRepeated extends MiniPhase with InfoTransformer { thisPhase => case arg: Typed if isWildcardStarArg(arg) => val isJavaDefined = tree.fun.symbol.is(JavaDefined) val tpe = arg.expr.tpe - if isJavaDefined && tpe.derivesFrom(defn.SeqClass) then - seqToArray(arg.expr) - else if !isJavaDefined && tpe.derivesFrom(defn.ArrayClass) + if isJavaDefined then + val pt = tree.fun.tpe.widen.firstParamTypes.last + adaptToArray(arg.expr, pt.elemType.bounds.hi) + else if tpe.derivesFrom(defn.ArrayClass) then arrayToSeq(arg.expr) else arg.expr @@ -107,7 +128,39 @@ class ElimRepeated extends MiniPhase with InfoTransformer { thisPhase => .appliedToType(elemType) .appliedTo(tree, clsOf(elemClass.typeRef)) - /** Convert Java array argument to Scala Seq */ + /** 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 treeIsArray = tree.tpe.derivesFrom(defn.ArrayClass) + if elemTp <:< elemPt then + if treeIsArray then + tree // no adaptation needed + else + tree match + case SeqLiteral(elems, elemtpt) => + JavaSeqLiteral(elems, elemtpt).withSpan(tree.span) + case _ => + // Convert a Seq[T] to an Array[$elemPt] + ref(defn.DottyArraysModule) + .select(nme.seqToArray) + .appliedToType(elemPt) + .appliedTo(tree, clsOf(elemPt)) + else if treeIsArray then + // Convert an Array[T] to an Array[Object] + ref(defn.ScalaRuntime_toObjectArray) + .appliedTo(tree) + else + // Convert a Seq[T] to an Array[Object] + ref(defn.ScalaRuntime_toArray) + .appliedToType(elemTp) + .appliedTo(tree) + + /** Convert an Array into a scala.Seq */ private def arrayToSeq(tree: Tree)(using Context): Tree = tpd.wrapArray(tree, tree.tpe.elemType) diff --git a/compiler/src/dotty/tools/dotc/typer/Typer.scala b/compiler/src/dotty/tools/dotc/typer/Typer.scala index 26124dca054a..ded36b9fdaac 100644 --- a/compiler/src/dotty/tools/dotc/typer/Typer.scala +++ b/compiler/src/dotty/tools/dotc/typer/Typer.scala @@ -1723,7 +1723,7 @@ class Typer extends Namer checkedArgs = checkedArgs.mapconserve(arg => checkSimpleKinded(checkNoWildcard(arg))) else if (ctx.compilationUnit.isJava) - if (tpt1.symbol eq defn.ArrayClass) || (tpt1.symbol eq defn.RepeatedParamClass) then + if (tpt1.symbol eq defn.ArrayClass) then checkedArgs match { case List(arg) => val elemtp = arg.tpe.translateJavaArrayElementType diff --git a/tests/neg/i533/Test.scala b/tests/neg/i533/Test.scala index 784715736574..80326f9c3af5 100644 --- a/tests/neg/i533/Test.scala +++ b/tests/neg/i533/Test.scala @@ -3,6 +3,6 @@ object Test { val x = new Array[Int](1) x(0) = 10 println(JA.get(x)) // error - println(JA.getVarargs(x: _*)) // error + println(JA.getVarargs(x: _*)) // now OK. } } diff --git a/tests/pos/arrays2.scala b/tests/pos/arrays2.scala index b2fe6ff1d6f0..c3812b798299 100644 --- a/tests/pos/arrays2.scala +++ b/tests/pos/arrays2.scala @@ -24,4 +24,5 @@ one warning found // #2461 object arrays3 { def apply[X <: AnyRef](xs : X*) : java.util.List[X] = java.util.Arrays.asList(xs: _*) + def apply2[X](xs : X*) : java.util.List[X] = java.util.Arrays.asList(xs: _*) } diff --git a/tests/run/i9439.scala b/tests/run/i9439.scala new file mode 100644 index 000000000000..90b2f8a52f90 --- /dev/null +++ b/tests/run/i9439.scala @@ -0,0 +1,17 @@ +object Test { + // First example with a concrete type <: AnyVal + def main(args: Array[String]): Unit = { + val coll = new java.util.ArrayList[Int]() + java.util.Collections.addAll(coll, 5, 6) + println(coll.size()) + + foo(5, 6) + } + + // Second example with an abstract type not known to be <: AnyRef + def foo[A](a1: A, a2: A): Unit = { + val coll = new java.util.ArrayList[A]() + java.util.Collections.addAll(coll, a1, a2) + println(coll.size()) + } +} diff --git a/tests/run/java-varargs-2/A.java b/tests/run/java-varargs-2/A.java new file mode 100644 index 000000000000..3b5169f84fa6 --- /dev/null +++ b/tests/run/java-varargs-2/A.java @@ -0,0 +1,10 @@ +class A { + public static void foo(int... args) { + } + + public static void gen(T... args) { + } + + public static void gen2(T... args) { + } +} diff --git a/tests/run/java-varargs-2/Test.scala b/tests/run/java-varargs-2/Test.scala new file mode 100644 index 000000000000..1a3d165188bd --- /dev/null +++ b/tests/run/java-varargs-2/Test.scala @@ -0,0 +1,13 @@ +object Test { + def main(args: Array[String]): Unit = { + A.foo(1) + A.foo(Array(1): _*) + A.foo(Seq(1): _*) + A.gen(1) + A.gen(Array(1): _*) + A.gen(Seq(1): _*) + A.gen2("") + A.gen2(Array(""): _*) + A.gen2(Seq(""): _*) + } +} diff --git a/tests/run/java-varargs/A_1.java b/tests/run/java-varargs/A_1.java index 9e98992e4097..df0cb5994ccd 100644 --- a/tests/run/java-varargs/A_1.java +++ b/tests/run/java-varargs/A_1.java @@ -4,4 +4,7 @@ public static void foo(int... args) { public static void gen(T... args) { } + + public static void gen2(T... args) { + } } diff --git a/tests/run/java-varargs/Test_2.scala b/tests/run/java-varargs/Test_2.scala index eb77d6e248f7..4b5c5749fa7a 100644 --- a/tests/run/java-varargs/Test_2.scala +++ b/tests/run/java-varargs/Test_2.scala @@ -1,8 +1,13 @@ object Test { def main(args: Array[String]): Unit = { + A_1.foo(1) A_1.foo(Array(1): _*) A_1.foo(Seq(1): _*) + A_1.gen(1) A_1.gen(Array(1): _*) A_1.gen(Seq(1): _*) + A_1.gen2("") + A_1.gen2(Array(""): _*) + A_1.gen2(Seq(""): _*) } }