Scala.js: Optimize varargs in a platform-dependent friendly way.

We now recognize the shapes of varargs, deconstruct them, and emit
instead calls to Scala.js-specific runtime methods. Those methods
choose at link-time the best implementation for the target platform.

The changes in `genActualArgs` revealed that we cannot trust
`Symbol.isPrivate`. It uses `lastDenot.flagsUNSAFE` under the hood,
which returns absurd results if `lastDenot` happened to be accessed
within an `atPhase(...)` call. Therefore, we change those calls to
`sym.is(Private)` instead. `Symbol.is` always uses an up-to-date
denotation.

Another consequence of the optimization is that the string
representation of varargs seqs is altered in Scala.js. This is
consistent with the Scala 2 behavior. We generalize the check file
lookup of Vulpix to allow platform-dependent check files. We use
that mechanism to introduce Scala.js-specific versions of the
affected tests.

This commit is essentially a forward port of the Scala.js commit
scala-js/scala-js@24750cf
except that, in dotc, previously we did not do the optimization at
all.

Author: sjrd

compiler/src/dotty/tools/backend/sjs/JSCodeGen.scala

@@ -1609,10 +1609,10 @@ class JSCodeGen()(using genCtx: Context) {
                 optimizerHints, Unversioned)
           } else {
             val namespace = if (isMethodStaticInIR(sym)) {
-              if (sym.isPrivate) js.MemberNamespace.PrivateStatic
+              if (sym.is(Private)) js.MemberNamespace.PrivateStatic
               else js.MemberNamespace.PublicStatic
             } else {
-              if (sym.isPrivate) js.MemberNamespace.Private
+              if (sym.is(Private)) js.MemberNamespace.Private
               else js.MemberNamespace.Public
             }
             val resultIRType = toIRType(patchedResultType(sym))
@@ -3770,7 +3770,7 @@ class JSCodeGen()(using genCtx: Context) {
   /** Gen a statically linked call to an instance method. */
   def genApplyMethodMaybeStatically(receiver: js.Tree, method: Symbol,
       arguments: List[js.Tree])(implicit pos: Position): js.Tree = {
-    if (method.isPrivate || method.isClassConstructor)
+    if (method.is(Private) || method.isClassConstructor)
       genApplyMethodStatically(receiver, method, arguments)
     else
       genApplyMethod(receiver, method, arguments)
@@ -3779,7 +3779,7 @@ class JSCodeGen()(using genCtx: Context) {
   /** Gen a dynamically linked call to a Scala method. */
   def genApplyMethod(receiver: js.Tree, method: Symbol, arguments: List[js.Tree])(
       implicit pos: Position): js.Tree = {
-    assert(!method.isPrivate,
+    assert(!method.is(Private),
         s"Cannot generate a dynamic call to private method $method at $pos")
     js.Apply(js.ApplyFlags.empty, receiver, encodeMethodSym(method), arguments)(
         toIRType(patchedResultType(method)))
@@ -3789,7 +3789,7 @@ class JSCodeGen()(using genCtx: Context) {
   def genApplyMethodStatically(receiver: js.Tree, method: Symbol, arguments: List[js.Tree])(
       implicit pos: Position): js.Tree = {
     val flags = js.ApplyFlags.empty
-      .withPrivate(method.isPrivate && !method.isClassConstructor)
+      .withPrivate(method.is(Private) && !method.isClassConstructor)
       .withConstructor(method.isClassConstructor)
     js.ApplyStatically(flags, receiver, encodeClassName(method.owner),
         encodeMethodSym(method), arguments)(
@@ -3799,7 +3799,7 @@ class JSCodeGen()(using genCtx: Context) {
   /** Gen a call to a static method. */
   private def genApplyStatic(method: Symbol, arguments: List[js.Tree])(
       implicit pos: Position): js.Tree = {
-    js.ApplyStatic(js.ApplyFlags.empty.withPrivate(method.isPrivate),
+    js.ApplyStatic(js.ApplyFlags.empty.withPrivate(method.is(Private)),
         encodeClassName(method.owner), encodeMethodSym(method), arguments)(
         toIRType(patchedResultType(method)))
   }
@@ -4400,34 +4400,53 @@ class JSCodeGen()(using genCtx: Context) {
    *  This tries to optimize repeated arguments (varargs) by turning them
    *  into js.WrappedArray instead of Scala wrapped arrays.
    */
-  private def genActualArgs(sym: Symbol, args: List[Tree])(
-      implicit pos: Position): List[js.Tree] = {
-    args.map(genExpr)
-    /*val wereRepeated = exitingPhase(currentRun.typerPhase) {
-      sym.tpe.params.map(p => isScalaRepeatedParamType(p.tpe))
+  private def genActualArgs(sym: Symbol, args: List[Tree]): List[js.Tree] = {
+    // Fast path for methods that do not have any repeated parameter
+    val isAnyParamRepeated: Boolean = atPhase(elimRepeatedPhase) {
+      sym.info.paramInfoss.flatten.exists(_.isRepeatedParam)
     }
 
-    if (wereRepeated.size > args.size) {
-      // Should not happen, but let's not crash
+    if (!isAnyParamRepeated) {
       args.map(genExpr)
     } else {
-      /* Arguments that are in excess compared to the type signature after
-       * erasure are lambda-lifted arguments. They cannot be repeated, hence
-       * the extension to `false`.
+      /* Erasure and lambdalift both insert capture params in dotc, at
+       * unpredictable positions. Therefore, we use the *names* of parameters
+       * as the only reliable link to whether they were initially repeated.
+       * We also do this in JSSymUtils.jsParamInfos for JS methods.
        */
-      for ((arg, wasRepeated) <- args.zipAll(wereRepeated, EmptyTree, false)) yield {
-        if (wasRepeated) {
-          tryGenRepeatedParamAsJSArray(arg, handleNil = false).fold {
-            genExpr(arg)
-          } { genArgs =>
-            genNew(WrappedArrayClass, WrappedArray_ctor,
-                List(js.JSArrayConstr(genArgs)))
+      val namesOfRepeatedParams: Set[TermName] = atPhase(elimRepeatedPhase) {
+        sym.info.paramNamess.flatten.zip(sym.info.paramInfoss.flatten).collect {
+          case (paramName, paramInfo) if paramInfo.isRepeatedParam => paramName
+        }.toSet
+      }
+
+      for ((arg, paramName) <- args.zip(sym.info.paramNamess.flatten)) yield {
+        if (namesOfRepeatedParams.contains(paramName)) {
+          /* If the argument is a call to the compiler's chosen `wrapArray`
+           * method with an array literal as argument, we know it actually
+           * came from expanded varargs. In that case, rewrite to calling our
+           * custom `scala.scalajs.runtime.to*VarArgs` method. These methods
+           * choose the best implementation of varargs depending on the
+           * target platform.
+           */
+          arg match {
+            case MaybeAsInstanceOf(wrapArray @ WrapArray(MaybeAsInstanceOf(arrayValue: JavaSeqLiteral))) =>
+              implicit val pos: SourcePosition = wrapArray.sourcePos
+              js.Apply(
+                js.ApplyFlags.empty,
+                js.LoadModule(ScalaJSRuntimeModClassName),
+                js.MethodIdent(WrapArray.wrapArraySymToToVarArgsName(wrapArray.symbol)),
+                List(genExpr(arrayValue))
+              )(jstpe.ClassType(encodeClassName(defn.SeqClass), nullable = true))
+
+            case _ =>
+              genExpr(arg)
           }
         } else {
           genExpr(arg)
         }
       }
-    }*/
+    }
   }
 
   /** Gen actual actual arguments to a JS method call.
@@ -4496,47 +4515,28 @@ class JSCodeGen()(using genCtx: Context) {
    *  `js.Array`.
    */
   private def genJSRepeatedParam(arg: Tree): List[js.TreeOrJSSpread] = {
-    tryGenRepeatedParamAsJSArray(arg, handleNil = true).getOrElse {
-      /* Fall back to calling runtime.genTraversableOnce2jsArray
-       * to perform the conversion to js.Array, then wrap in a Spread
-       * operator.
-       */
-      implicit val pos: SourcePosition = arg.sourcePos
-      val jsArrayArg = genModuleApplyMethod(
-          jsdefn.Runtime_toJSVarArgs,
-          List(genExpr(arg)))
-      List(js.JSSpread(jsArrayArg))
-    }
-  }
-
-  /** Try and expand an actual argument to a repeated param `(xs: T*)`.
-   *
-   *  This method recognizes the shapes of tree generated by the desugaring
-   *  of repeated params in Scala, and expands them.
-   *  If `arg` does not have the shape of a generated repeated param, this
-   *  method returns `None`.
-   */
-  private def tryGenRepeatedParamAsJSArray(arg: Tree,
-      handleNil: Boolean): Option[List[js.Tree]] = {
-    implicit val pos = arg.span
+    implicit val pos: SourcePosition = arg.sourcePos
 
     // Given a method `def foo(args: T*)`
     arg match {
       // foo(arg1, arg2, ..., argN) where N > 0
       case MaybeAsInstanceOf(WrapArray(MaybeAsInstanceOf(array: JavaSeqLiteral))) =>
         /* Value classes in arrays are already boxed, so no need to use
          * the type before erasure.
-         * TODO Is this true in dotty?
          */
-        Some(array.elems.map(e => box(genExpr(e), e.tpe)))
+        array.elems.map(e => box(genExpr(e), e.tpe))
 
       // foo()
-      case Ident(_) if handleNil && arg.symbol == defn.NilModule =>
-        Some(Nil)
+      case Ident(_) if arg.symbol == defn.NilModule =>
+        Nil
 
       // foo(argSeq: _*) - cannot be optimized
       case _ =>
-        None
+        /* Fall back to calling runtime.toJSVarArgs to perform the conversion
+         * to js.Array, then wrap in a Spread operator.
+         */
+        val jsArrayArg = genModuleApplyMethod(jsdefn.Runtime_toJSVarArgs, List(genExpr(arg)))
+        List(js.JSSpread(jsArrayArg))
     }
   }
 
@@ -4551,16 +4551,32 @@ class JSCodeGen()(using genCtx: Context) {
   }
 
   private object WrapArray {
-    lazy val isWrapArray: Set[Symbol] = {
-      val names0 = defn.ScalaValueClasses().map(sym => nme.wrapXArray(sym.name))
-      val names1 = names0 ++ Set(nme.wrapRefArray, nme.genericWrapArray)
-      val symsInPredef = names1.map(defn.ScalaPredefModule.requiredMethod(_))
-      val symsInScalaRunTime = names1.map(defn.ScalaRuntimeModule.requiredMethod(_))
-      (symsInPredef ++ symsInScalaRunTime).toSet
+    lazy val wrapArraySymToToVarArgsName: Map[Symbol, MethodName] = {
+      val SeqClassRef = jstpe.ClassRef(encodeClassName(defn.SeqClass))
+
+      val items: Seq[(Name, String, jstpe.TypeRef)] = Seq(
+        (nme.genericWrapArray, "toGenericVarArgs", jswkn.ObjectRef),
+        (nme.wrapRefArray, "toRefVarArgs", jstpe.ArrayTypeRef(jswkn.ObjectRef, 1)),
+        (tpd.wrapArrayMethodName(defn.UnitType), "toUnitVarArgs", jstpe.ArrayTypeRef(jstpe.ClassRef(jswkn.BoxedUnitClass), 1)),
+        (tpd.wrapArrayMethodName(defn.BooleanType), "toBooleanVarArgs", jstpe.ArrayTypeRef(jstpe.BooleanRef, 1)),
+        (tpd.wrapArrayMethodName(defn.CharType), "toCharVarArgs", jstpe.ArrayTypeRef(jstpe.CharRef, 1)),
+        (tpd.wrapArrayMethodName(defn.ByteType), "toByteVarArgs", jstpe.ArrayTypeRef(jstpe.ByteRef, 1)),
+        (tpd.wrapArrayMethodName(defn.ShortType), "toShortVarArgs", jstpe.ArrayTypeRef(jstpe.ShortRef, 1)),
+        (tpd.wrapArrayMethodName(defn.IntType), "toIntVarArgs", jstpe.ArrayTypeRef(jstpe.IntRef, 1)),
+        (tpd.wrapArrayMethodName(defn.LongType), "toLongVarArgs", jstpe.ArrayTypeRef(jstpe.LongRef, 1)),
+        (tpd.wrapArrayMethodName(defn.FloatType), "toFloatVarArgs", jstpe.ArrayTypeRef(jstpe.FloatRef, 1)),
+        (tpd.wrapArrayMethodName(defn.DoubleType), "toDoubleVarArgs", jstpe.ArrayTypeRef(jstpe.DoubleRef, 1))
+      )
+
+      items.map { case (wrapArrayName, simpleName, argTypeRef) =>
+        val wrapArraySym = defn.getWrapVarargsArrayModule.requiredMethod(wrapArrayName)
+        val toVarArgsName = MethodName(simpleName, argTypeRef :: Nil, SeqClassRef)
+        wrapArraySym -> toVarArgsName
+      }.toMap
     }
 
     def unapply(tree: Apply): Option[Tree] = tree match {
-      case Apply(wrapArray_?, List(wrapped)) if isWrapArray(wrapArray_?.symbol) =>
+      case Apply(wrapArray_?, List(wrapped)) if wrapArraySymToToVarArgsName.contains(wrapArray_?.symbol) =>
         Some(wrapped)
       case _ =>
         None
@@ -5002,6 +5018,7 @@ object JSCodeGen {
   private val JLRArrayClassName = ClassName("java.lang.reflect.Array")
   private val JSObjectClassName = ClassName("scala.scalajs.js.Object")
   private val JavaScriptExceptionClassName = ClassName("scala.scalajs.js.JavaScriptException")
+  private val ScalaJSRuntimeModClassName = ClassName("scala.scalajs.runtime.package$")
 
   private val ObjectArrayTypeRef = jstpe.ArrayTypeRef(jswkn.ObjectRef, 1)
 

compiler/src/dotty/tools/dotc/transform/sjs/JSSymUtils.scala

@@ -188,6 +188,7 @@ object JSSymUtils {
         sym.info.paramNamess.flatten.zip(sym.info.paramInfoss.flatten)
 
       val paramInfosAtElimRepeated = atPhase(elimRepeatedPhase) {
+        // See also JSCodeGen.genActualArgs
         val list =
           for ((name, info) <- paramNamesAndTypes) yield {
             val v =

compiler/test/dotty/tools/vulpix/ParallelTesting.scala

@@ -71,7 +71,13 @@ trait ParallelTesting extends RunnerOrchestration:
     def outDir: JFile
     def flags: TestFlags
     def sourceFiles: Array[JFile]
-    def checkFile: Option[JFile]
+    def checkFileBasePathCandidates: Array[String]
+
+    final def checkFile: Option[JFile] =
+      checkFileBasePathCandidates
+        .iterator
+        .flatMap(base => Iterator(new JFile(s"$base.$testPlatform.check"), new JFile(s"$base.check")))
+        .find(_.exists())
 
     def runClassPath: String = outDir.getPath + JFile.pathSeparator + flags.runClassPath
 
@@ -186,9 +192,8 @@ trait ParallelTesting extends RunnerOrchestration:
   ) extends TestSource {
     def sourceFiles: Array[JFile] = files.filter(isSourceFile)
 
-    def checkFile: Option[JFile] =
-      sourceFiles.map(f => new JFile(f.getPath.replaceFirst("\\.(scala|java)$", ".check")))
-        .find(_.exists())
+    def checkFileBasePathCandidates: Array[String] =
+      sourceFiles.map(f => f.getPath.replaceFirst("\\.(scala|java)$", ""))
   }
 
   /** A test source whose files will be compiled separately according to their
@@ -221,11 +226,8 @@ trait ParallelTesting extends RunnerOrchestration:
 
     def sourceFiles = compilationGroups.map(_._2).flatten.toArray
 
-    def checkFile: Option[JFile] =
-      val platform =
-        if allToolArgs.getOrElse(ToolName.Target, Nil).nonEmpty then s".$testPlatform"
-        else ""
-      Some(new JFile(dir.getPath + platform + ".check")).filter(_.exists)
+    def checkFileBasePathCandidates: Array[String] =
+      Array(dir.getPath)
   }
 
   protected def shouldSkipTestSource(testSource: TestSource): Boolean = false

tests/run/i17761.scala-js.check

@@ -0,0 +1,8 @@
+Normal
+test
+WrappedVarArgs(class java.lang.String, int)
+WrappedVarArgs(test, 42)
+Transparent
+test
+ArraySeq(class java.lang.String, int)
+ArraySeq(test, 42)

tests/run/i3207.scala-js.check

@@ -0,0 +1,2 @@
+WrappedVarArgs()
+WrappedVarArgs(A, B)

tests/run/i768.check

@@ -0,0 +1 @@
+ArraySeq(a, bc)

tests/run/i768.scala

@@ -4,8 +4,6 @@ case class A(a: String*){
 
 object Test {
   def main(args: Array[String]) = {
-    assert(A("a", "bc").s == "ArraySeq(a, bc)")
+    println(A("a", "bc").s)
   }
 }
-
-

tests/run/i768.scala-js.check

@@ -0,0 +1 @@
+WrappedVarArgs(a, bc)

tests/run/sammy_repeated.scala-js.check

@@ -0,0 +1 @@
+WrappedVarArgs(1)

tests/run/tagless.scala-js.check

@@ -0,0 +1,20 @@
+5
+(8 + (-(1 + 2)))
+5
+(7 + (-1 * 2))
+35
+7 * (8 + (-(1 + 2)))
+tf1Tree = Node(Add,WrappedVarArgs(Node(Lit,WrappedVarArgs(Leaf(8))), Node(Neg,WrappedVarArgs(Node(Add,WrappedVarArgs(Node(Lit,WrappedVarArgs(Leaf(1))), Node(Lit,WrappedVarArgs(Leaf(2)))))))))
+tfm1Tree = Node(Add,WrappedVarArgs(Node(Lit,WrappedVarArgs(Leaf(7))), Node(Neg,WrappedVarArgs(Node(Mult,WrappedVarArgs(Node(Lit,WrappedVarArgs(Leaf(1))), Node(Lit,WrappedVarArgs(Leaf(2)))))))))
+2
+Not a number: "X"
+5
+(8 + (-(1 + 2)))
+Node(Add,WrappedVarArgs(Node(Lit,WrappedVarArgs(Leaf(8))), Node(Neg,WrappedVarArgs(Node(Add,WrappedVarArgs(Node(Lit,WrappedVarArgs(Leaf(1))), Node(Lit,WrappedVarArgs(Leaf(2)))))))))
+5
+(8 + (-(1 + 2)))
+(8 + ((-1) + (-2)))
+(8 + ((-1) + (-2)))
+(8 + ((-1) + (-2)))
+(7 + 1 * (-2))
+7 * (8 + ((-1) + (-2)))