Merge pull request #5556 from dragos/ticket/10071

SI-10071 Separate compilation for varargs methods

11 files changed, 147 insertions, 65 deletions

diff --git a/src/compiler/scala/tools/nsc/transform/Erasure.scala b/src/compiler/scala/tools/nsc/transform/Erasure.scala
index 25475515aa..92accaf9dd 100644
--- a/src/compiler/scala/tools/nsc/transform/Erasure.scala
+++ b/src/compiler/scala/tools/nsc/transform/Erasure.scala
@@ -1278,5 +1278,4 @@ abstract class Erasure extends InfoTransform
   }
 
   private class TypeRefAttachment(val tpe: TypeRef)
-  class TypeParamVarargsAttachment(val typeParamRef: Type)
 }
diff --git a/src/compiler/scala/tools/nsc/transform/UnCurry.scala b/src/compiler/scala/tools/nsc/transform/UnCurry.scala
index d8fa7b58e8..ea3c7da014 100644
--- a/src/compiler/scala/tools/nsc/transform/UnCurry.scala
+++ b/src/compiler/scala/tools/nsc/transform/UnCurry.scala
@@ -27,6 +27,8 @@ import scala.reflect.internal.util.ListOfNil
  *  - for every repeated Scala parameter `x: T*' --> x: Seq[T].
  *  - for every repeated Java parameter `x: T...' --> x: Array[T], except:
  *    if T is an unbounded abstract type, replace --> x: Array[Object]
+ *  - for every method defining repeated parameters annotated with @varargs, generate
+ *    a synthetic Java-style vararg method
  *  - for every argument list that corresponds to a repeated Scala parameter
  *       (a_1, ..., a_n) => (Seq(a_1, ..., a_n))
  *  - for every argument list that corresponds to a repeated Java parameter
@@ -44,6 +46,8 @@ import scala.reflect.internal.util.ListOfNil
  *          def liftedTry$1 = try { x_i } catch { .. }
  *          meth(x_1, .., liftedTry$1(), .. )
  *        }
+ *  - remove calls to elidable methods and replace their bodies with NOPs when elide-below
+ *    requires it
  */
 /*</export> */
 abstract class UnCurry extends InfoTransform
@@ -577,7 +581,13 @@ abstract class UnCurry extends InfoTransform
               case None    => literalRhsIfConst
             }
           )
-          addJavaVarargsForwarders(dd, flatdd)
+          // Only class members can reasonably be called from Java due to name mangling.
+          // Additionally, the Uncurry info transformer only adds a forwarder symbol to class members,
+          // since the other symbols are not part of the ClassInfoType (see reflect.internal.transform.UnCurry)
+          if (dd.symbol.owner.isClass)
+            addJavaVarargsForwarders(dd, flatdd)
+          else
+            flatdd
 
         case tree: Try =>
           if (tree.catches exists (cd => !treeInfo.isCatchCase(cd)))
@@ -739,68 +749,32 @@ abstract class UnCurry extends InfoTransform
         if (!hasRepeated) reporter.error(dd.symbol.pos, "A method without repeated parameters cannot be annotated with the `varargs` annotation.")
       }
 
-    /* Called during post transform, after the method argument lists have been flattened.
-     * It looks for the method in the `repeatedParams` map, and generates a Java-style
+    /**
+     * Called during post transform, after the method argument lists have been flattened.
+     * It looks for the forwarder symbol in the symbol attachments and generates a Java-style
      * varargs forwarder.
+     *
+     * @note The Java-style varargs method symbol is generated in the Uncurry info transformer. If the
+     *       symbol can't be found this method reports a warning and carries on.
+     * @see  [[scala.reflect.internal.transform.UnCurry]]
      */
     private def addJavaVarargsForwarders(dd: DefDef, flatdd: DefDef): DefDef = {
       if (!dd.symbol.hasAnnotation(VarargsClass) || !enteringUncurry(mexists(dd.symbol.paramss)(sym => definitions.isRepeatedParamType(sym.tpe))))
         return flatdd
 
-      val forwSym = currentClass.newMethod(dd.name.toTermName, dd.pos, VARARGS | SYNTHETIC | flatdd.symbol.flags & ~DEFERRED)
-
-      val isRepeated = enteringUncurry(dd.symbol.info.paramss.flatten.map(sym => definitions.isRepeatedParamType(sym.tpe)))
-
-      val oldPs = flatdd.symbol.paramss.head
-
-      // see comment in method toArrayType below
-      val arrayTypesMappedToObject = mutable.Map.empty[Symbol, Type]
-
-      val forwTpe = {
-        val (oldTps, tps) = dd.symbol.tpe match {
-          case PolyType(oldTps, _) =>
-            val newTps = oldTps.map(_.cloneSymbol(forwSym))
-            (oldTps, newTps)
-
-          case _ => (Nil, Nil)
-        }
-
-        def toArrayType(tp: Type, newParam: Symbol): Type = {
-          val arg = elementType(SeqClass, tp)
-          val elem = if (arg.typeSymbol.isTypeParameterOrSkolem && !(arg <:< AnyRefTpe)) {
-            // To prevent generation of an `Object` parameter from `Array[T]` parameter later
-            // as this would crash the Java compiler which expects an `Object[]` array for varargs
-            //   e.g.        def foo[T](a: Int, b: T*)
-            //   becomes     def foo[T](a: Int, b: Array[Object])
-            //   instead of  def foo[T](a: Int, b: Array[T]) ===> def foo[T](a: Int, b: Object)
-            //
-            // In order for the forwarder method to type check we need to insert a cast:
-            //   def foo'[T'](a: Int, b: Array[Object]) = foo[T'](a, wrapRefArray(b).asInstanceOf[Seq[T']])
-            // The target element type for that cast (T') is stored in the `arrayTypesMappedToObject` map.
-            val originalArg = arg.substSym(oldTps, tps)
-            arrayTypesMappedToObject(newParam) = originalArg
-            // Store the type parameter that was replaced by Object to emit the correct generic signature
-            newParam.updateAttachment(new erasure.TypeParamVarargsAttachment(originalArg))
-            ObjectTpe
-          } else
-            arg
-          arrayType(elem)
+      val forwSym: Symbol = {
+        currentClass.info // make sure the info is up to date, so the varargs forwarder symbol has been generated
+        flatdd.symbol.attachments.get[VarargsSymbolAttachment] match {
+          case Some(VarargsSymbolAttachment(sym)) => sym
+          case None =>
+            reporter.warning(dd.pos, s"Could not generate Java varargs forwarder for ${flatdd.symbol}. Please file a bug.")
+            return flatdd
         }
-
-        val ps = map2(oldPs, isRepeated)((oldParam, isRep) => {
-          val newParam = oldParam.cloneSymbol(forwSym)
-          val tp = if (isRep) toArrayType(oldParam.tpe, newParam) else oldParam.tpe
-          newParam.setInfo(tp)
-        })
-
-        val resTp = dd.symbol.tpe_*.finalResultType.substSym(oldPs, ps)
-        val mt = MethodType(ps, resTp)
-        val r = if (tps.isEmpty) mt else PolyType(tps, mt)
-        r.substSym(oldTps, tps)
       }
 
-      forwSym.setInfo(forwTpe)
-      val newPs = forwTpe.params
+      val newPs = forwSym.tpe.params
+      val isRepeated = enteringUncurry(dd.symbol.info.paramss.flatten.map(sym => definitions.isRepeatedParamType(sym.tpe)))
+      val oldPs = flatdd.symbol.paramss.head
 
       val theTyper = typer.atOwner(dd, currentClass)
       val forwTree = theTyper.typedPos(dd.pos) {
@@ -809,8 +783,8 @@ abstract class UnCurry extends InfoTransform
           else {
             val parTp = elementType(ArrayClass, param.tpe)
             val wrap = gen.mkWrapArray(Ident(param), parTp)
-            arrayTypesMappedToObject.get(param) match {
-              case Some(tp) => gen.mkCast(wrap, seqType(tp))
+            param.attachments.get[TypeParamVarargsAttachment] match {
+              case Some(TypeParamVarargsAttachment(tp)) => gen.mkCast(wrap, seqType(tp))
               case _ => wrap
             }
           }
@@ -821,13 +795,12 @@ abstract class UnCurry extends InfoTransform
       }
 
       // check if the method with that name and those arguments already exists in the template
-      currentClass.info.member(forwSym.name).alternatives.find(s => s != forwSym && s.tpe.matches(forwSym.tpe)) match {
-        case Some(s) => reporter.error(dd.symbol.pos,
-                                   "A method with a varargs annotation produces a forwarder method with the same signature "
-                                   + s.tpe + " as an existing method.")
+      enteringUncurry(currentClass.info.member(forwSym.name).alternatives.find(s => s != forwSym && s.tpe.matches(forwSym.tpe))) match {
+        case Some(s) =>
+          reporter.error(dd.symbol.pos,
+            s"A method with a varargs annotation produces a forwarder method with the same signature ${s.tpe} as an existing method.")
         case None =>
           // enter symbol into scope
-          currentClass.info.decls enter forwSym
           addNewMember(forwTree)
       }
 
diff --git a/src/reflect/scala/reflect/internal/StdAttachments.scala b/src/reflect/scala/reflect/internal/StdAttachments.scala
index 78f360409d..fd8f51cfb1 100644
--- a/src/reflect/scala/reflect/internal/StdAttachments.scala
+++ b/src/reflect/scala/reflect/internal/StdAttachments.scala
@@ -78,4 +78,7 @@ trait StdAttachments {
   case object OuterArgCanBeElided extends PlainAttachment
 
   case object UseInvokeSpecial extends PlainAttachment
+
+  /** An attachment carrying information between uncurry and erasure */
+  case class TypeParamVarargsAttachment(val typeParamRef: Type)
 }
diff --git a/src/reflect/scala/reflect/internal/transform/UnCurry.scala b/src/reflect/scala/reflect/internal/transform/UnCurry.scala
index a50084f40d..3918723b5c 100644
--- a/src/reflect/scala/reflect/internal/transform/UnCurry.scala
+++ b/src/reflect/scala/reflect/internal/transform/UnCurry.scala
@@ -4,6 +4,7 @@ package internal
 package transform
 
 import Flags._
+import scala.collection.mutable
 
 trait UnCurry {
 
@@ -11,6 +12,12 @@ trait UnCurry {
   import global._
   import definitions._
 
+  /**
+   * The synthetic Java vararg method symbol corresponding to a Scala vararg method
+   * annotated with @varargs.
+   */
+  case class VarargsSymbolAttachment(varargMethod: Symbol)
+
   /** Note: changing tp.normalize to tp.dealias in this method leads to a single
    *  test failure: run/t5688.scala, where instead of the expected output
    *    Vector(ta, tb, tab)
@@ -65,18 +72,74 @@ trait UnCurry {
     def apply(tp0: Type): Type = {
       val tp = expandAlias(tp0)
       tp match {
-        case ClassInfoType(parents, decls, clazz) =>
+        case ClassInfoType(parents, decls, clazz) if !clazz.isJavaDefined =>
           val parents1 = parents mapConserve uncurry
-          if (parents1 eq parents) tp
-          else ClassInfoType(parents1, decls, clazz) // @MAT normalize in decls??
+          val varargOverloads = mutable.ListBuffer.empty[Symbol]
+
+          // Not using `hasAnnotation` here because of dreaded cyclic reference errors:
+          // it may happen that VarargsClass has not been initialized yet and we get here
+          // while processing one of its superclasses (such as java.lang.Object). Since we
+          // don't need the more precise `matches` semantics, we only check the symbol, which
+          // is anyway faster and safer
+          for (decl <- decls if decl.annotations.exists(_.symbol == VarargsClass)) {
+            if (mexists(decl.paramss)(sym => definitions.isRepeatedParamType(sym.tpe))) {
+              varargOverloads += varargForwarderSym(clazz, decl, exitingPhase(phase)(decl.info))
+            }
+          }
+          if ((parents1 eq parents) && varargOverloads.isEmpty) tp
+          else {
+            val newDecls = decls.cloneScope
+            varargOverloads.foreach(newDecls.enter)
+            ClassInfoType(parents1, newDecls, clazz)
+          } // @MAT normalize in decls??
+
         case PolyType(_, _) =>
           mapOver(tp)
+
         case _ =>
           tp
       }
     }
   }
 
+  private def varargForwarderSym(currentClass: Symbol, origSym: Symbol, newInfo: Type): Symbol = {
+    val forwSym = origSym.cloneSymbol(currentClass, VARARGS | SYNTHETIC | origSym.flags & ~DEFERRED, origSym.name.toTermName).withoutAnnotations
+
+    // we are using `origSym.info`, which contains the type *before* the transformation
+    // so we still see repeated parameter types (uncurry replaces them with Seq)
+    val isRepeated = origSym.info.paramss.flatten.map(sym => definitions.isRepeatedParamType(sym.tpe))
+    val oldPs = newInfo.paramss.head
+    def toArrayType(tp: Type, newParam: Symbol): Type = {
+      val arg = elementType(SeqClass, tp)
+      val elem = if (arg.typeSymbol.isTypeParameterOrSkolem && !(arg <:< AnyRefTpe)) {
+        // To prevent generation of an `Object` parameter from `Array[T]` parameter later
+        // as this would crash the Java compiler which expects an `Object[]` array for varargs
+        //   e.g.        def foo[T](a: Int, b: T*)
+        //   becomes     def foo[T](a: Int, b: Array[Object])
+        //   instead of  def foo[T](a: Int, b: Array[T]) ===> def foo[T](a: Int, b: Object)
+        //
+        // In order for the forwarder method to type check we need to insert a cast:
+        //   def foo'[T'](a: Int, b: Array[Object]) = foo[T'](a, wrapRefArray(b).asInstanceOf[Seq[T']])
+        // The target element type for that cast (T') is stored in the  TypeParamVarargsAttachment
+//        val originalArg = arg.substSym(oldTps, tps)
+        // Store the type parameter that was replaced by Object to emit the correct generic signature
+        newParam.updateAttachment(new TypeParamVarargsAttachment(arg))
+        ObjectTpe
+      } else
+        arg
+      arrayType(elem)
+    }
+
+    foreach2(forwSym.paramss.flatten, isRepeated)((p, isRep) =>
+      if (isRep) {
+        p.setInfo(toArrayType(p.info, p))
+      }
+    )
+
+    origSym.updateAttachment(VarargsSymbolAttachment(forwSym))
+    forwSym
+  }
+
   /** - return symbol's transformed type,
    *  - if symbol is a def parameter with transformed type T, return () => T
    *
diff --git a/src/reflect/scala/reflect/runtime/JavaUniverseForce.scala b/src/reflect/scala/reflect/runtime/JavaUniverseForce.scala
index b74ccb9177..dbafbfc6ba 100644
--- a/src/reflect/scala/reflect/runtime/JavaUniverseForce.scala
+++ b/src/reflect/scala/reflect/runtime/JavaUniverseForce.scala
@@ -47,6 +47,7 @@ trait JavaUniverseForce { self: runtime.JavaUniverse  =>
     this.InlineCallsiteAttachment
     this.OuterArgCanBeElided
     this.UseInvokeSpecial
+    this.TypeParamVarargsAttachment
     this.noPrint
     this.typeDebug
     this.Range
@@ -458,6 +459,7 @@ trait JavaUniverseForce { self: runtime.JavaUniverse  =>
     definitions.ScalaValueClassesNoUnit
     definitions.ScalaValueClasses
 
+    uncurry.VarargsSymbolAttachment
     uncurry.DesugaredParameterType
     erasure.GenericArray
     erasure.scalaErasure
diff --git a/test/files/jvm/varargs-separate-bytecode.check b/test/files/jvm/varargs-separate-bytecode.check
new file mode 100644
index 0000000000..1507cd48c5
--- /dev/null
+++ b/test/files/jvm/varargs-separate-bytecode.check
@@ -0,0 +1 @@
+Found vararg overload for method create
\ No newline at end of file
diff --git a/test/files/jvm/varargs-separate-bytecode/AbstractProps_1.scala b/test/files/jvm/varargs-separate-bytecode/AbstractProps_1.scala
new file mode 100644
index 0000000000..5dfb8d1a9e
--- /dev/null
+++ b/test/files/jvm/varargs-separate-bytecode/AbstractProps_1.scala
@@ -0,0 +1,8 @@
+package foo
+
+import scala.annotation.varargs
+
+trait AbstractProps {
+  @varargs
+  def create(x: String, y: Int*): AbstractProps = null
+}
diff --git a/test/files/jvm/varargs-separate-bytecode/Props_2.scala b/test/files/jvm/varargs-separate-bytecode/Props_2.scala
new file mode 100644
index 0000000000..3fc09586fc
--- /dev/null
+++ b/test/files/jvm/varargs-separate-bytecode/Props_2.scala
@@ -0,0 +1,3 @@
+import foo.AbstractProps
+
+class Props extends AbstractProps
\ No newline at end of file
diff --git a/test/files/jvm/varargs-separate-bytecode/Test.scala b/test/files/jvm/varargs-separate-bytecode/Test.scala
new file mode 100644
index 0000000000..a666de7f39
--- /dev/null
+++ b/test/files/jvm/varargs-separate-bytecode/Test.scala
@@ -0,0 +1,15 @@
+import scala.collection.JavaConverters._
+import scala.tools.asm
+import scala.tools.asm.Opcodes
+import scala.tools.partest.BytecodeTest
+
+object Test extends BytecodeTest {
+  def show: Unit = {
+    val classNode = loadClassNode("Props")
+    val methods = classNode.methods.iterator().asScala.filter( m => m.name == "create")
+
+    for (m <- methods if (m.access & Opcodes.ACC_VARARGS) > 0) {
+      println(s"Found vararg overload for method ${m.name}")
+    }
+  }
+}
diff --git a/test/files/run/t5125b.check b/test/files/run/t5125b.check
index ddbf908f04..29b438a2d6 100644
--- a/test/files/run/t5125b.check
+++ b/test/files/run/t5125b.check
@@ -5,3 +5,6 @@ public void C2.f(scala.collection.Seq)
 public void C2$C3.f(java.lang.String[])
 public void C2$C3.f(scala.collection.Seq)
 public void C4.f(scala.collection.Seq)
+private void C5.f(int,int[])
+private void C5.f(int,scala.collection.Seq)
+public void C5.f(scala.collection.Seq)
diff --git a/test/files/run/t5125b.scala b/test/files/run/t5125b.scala
index 149c49e213..60ab1d9792 100644
--- a/test/files/run/t5125b.scala
+++ b/test/files/run/t5125b.scala
@@ -23,6 +23,17 @@ class C4 {
   }
 }
 
+class C5 {
+  def f(values: String*) = println("Calling C5.f(): " + values)
+  @scala.annotation.varargs
+  private def f(v: Int, values: Int*) = println("Calling C5.f(): " + values)
+
+  def method(): Unit = {
+    @scala.annotation.varargs
+    def f(values: String*) = println("Calling C5.<locally>.f(): " + values)
+  }
+}
+
 object Test extends App {
   def check(c: Class[_]) {
     val methodName = "f"
@@ -34,4 +45,5 @@ object Test extends App {
   check(classOf[C2])
   check(classOf[C2#C3])
   check(classOf[C4])
+  check(classOf[C5])
 }