Move the insertion of fake New nodes after erasure.

We enable Ycheck for our phases in the test suite to make sure that
we do not regress in the future.

Author: sjrd

compiler/src/dotty/tools/dotc/Compiler.scala

@@ -99,6 +99,7 @@ class Compiler {
          new PureStats,              // Remove pure stats from blocks
          new VCElideAllocations,     // Peep-hole optimization to eliminate unnecessary value class allocations
          new ArrayApply,             // Optimize `scala.Array.apply([....])` and `scala.Array.apply(..., [....])` into `[...]`
+         new sjs.AddLocalJSFakeNews, // Adds fake new invocations to local JS classes in calls to `createLocalJSClass`
          new ElimPolyFunction,       // Rewrite PolyFunction subclasses to FunctionN subclasses
          new TailRec,                // Rewrite tail recursion to loops
          new CompleteJavaEnums,      // Fill in constructors for Java enums

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

@@ -384,6 +384,13 @@ object ExplicitOuter {
       }
       else Nil
 
+    /** If the constructors of the given `cls` need to be passed an outer
+     *  argument, the singleton list with the argument, otherwise Nil.
+     */
+    def argsForNew(cls: ClassSymbol, tpe: Type): List[Tree] =
+      if (hasOuterParam(cls)) singleton(fixThis(outerPrefix(tpe))) :: Nil
+      else Nil
+
     /** A path of outer accessors starting from node `start`. `start` defaults to the
      *  context owner's this node. There are two alternative conditions that determine
      *  where the path ends:

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

@@ -0,0 +1,99 @@
+package dotty.tools
+package dotc
+package transform
+package sjs
+
+import MegaPhase._
+import core.Constants
+import core.Contexts._
+import core.Decorators._
+import core.StdNames.nme
+import core.Phases._
+import core.Symbols._
+import core.Types._
+import ast.Trees._
+import dotty.tools.dotc.ast.tpd
+
+import dotty.tools.backend.sjs.JSDefinitions.jsdefn
+
+/** Adds fake calls to the constructors of local JS classes in calls to
+ *  `createLocalJSClass`.
+ *
+ *  Given a call of the form
+ *  {{{
+ *  scala.scalajs.runtime.createLocalJSClass(classOf[C], jsClassValue, ???)
+ *  }}}
+ *  this phase fills in the `???` with an array of calls to the constructors
+ *  of `C`, like
+ *  {{{
+ *  [ new C(), new C(???, ???) : Object ]
+ *  }}}
+ *
+ *  If the class `C` has an outer pointer, as determined by the `ExplicitOuter`
+ *  phase, the calls to the constructor insert a reference to the outer
+ *  instance:
+ *  {{{
+ *  [ new C(Enclosing.this), new C(Enclosing.this, ???, ???) : Object ]
+ *  }}}
+ *
+ *  The `LambdaLift` phase will further expand those constructor calls with
+ *  values for captures. The back-end will extract the values of the outer
+ *  pointer and/or the captures to introduce them as JS class captures.
+ *
+ *  Since we need to insert fake `new C()` calls, this scheme does not work for
+ *  abstract local classes. We therefore reject them as implementation
+ *  restriction in `PrepJSInterop`.
+ *
+ *  This phase complements `ExplicitJSClasses`. The latter cannot create the
+ *  fake new invocations because that would require inventing sound type
+ *  arguments for the class' type parameters in order not to break Ycheck.
+ */
+class AddLocalJSFakeNews extends MiniPhase { thisPhase =>
+  import ExplicitOuter.outer
+  import ast.tpd._
+
+  override def phaseName: String = AddLocalJSFakeNews.name
+
+  override def isEnabled(using Context): Boolean =
+    ctx.settings.scalajs.value
+
+  override def runsAfter: Set[String] = Set(Erasure.name)
+
+  override def transformApply(tree: Apply)(using Context): Tree = {
+    if (tree.symbol == jsdefn.Runtime_createLocalJSClass) {
+      val classValueArg :: superClassValueArg :: _ :: Nil = tree.args
+      val cls = classValueArg match {
+        case Literal(constant) if constant.tag == Constants.ClazzTag =>
+          constant.typeValue.typeSymbol.asClass
+        case _ =>
+          // this shouldn't happen
+          report.error(i"unexpected $classValueArg for the first argument to `createLocalJSClass`", classValueArg)
+          jsdefn.JSObjectClass
+      }
+
+      val fakeNews = {
+        val ctors = cls.info.decls.lookupAll(nme.CONSTRUCTOR).toList.reverse
+        val elems = ctors.map(ctor => fakeNew(cls, ctor.asTerm))
+        JavaSeqLiteral(elems, TypeTree(defn.ObjectType))
+      }
+
+      cpy.Apply(tree)(tree.fun, classValueArg :: superClassValueArg :: fakeNews :: Nil)
+    } else {
+      tree
+    }
+  }
+
+  /** Creates a fake invocation of the given class with the given constructor. */
+  private def fakeNew(cls: ClassSymbol, ctor: TermSymbol)(using Context): Tree = {
+    val tycon = cls.typeRef
+    val outerArgs = outer.argsForNew(cls, tycon)
+    val nonOuterArgCount = ctor.info.firstParamTypes.size - outerArgs.size
+    val nonOuterArgs = List.fill(nonOuterArgCount)(ref(defn.Predef_undefined).appliedToNone)
+
+    New(tycon, ctor, outerArgs ::: nonOuterArgs)
+  }
+}
+
+object AddLocalJSFakeNews {
+  val name: String = "addLocalJSFakeNews"
+}

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

@@ -163,13 +163,15 @@ import JSSymUtils._
  *    val Local\$jsclass: AnyRef = createLocalJSClass(
  *        classOf[Local],
  *        js.constructorOf[ParentJSClass],
- *        Array[AnyRef](new Local(), ...))
+ *        ???)
  *  }
  *  }}}
  *
- *  Since we need to insert fake `new Inner()`s, this scheme does not work for
- *  abstract local classes. We therefore reject them as implementation
- *  restriction in `PrepJSInterop`.
+ *  The third argument `???` is a placeholder, which will be filled in by
+ *  `AddLocalJSFakeNews` with fake new invocations for the all the constructors
+ *  of `Local`. We cannot do it at this phase because that would require
+ *  inventing sound type arguments for the type parameters of `Local` out of
+ *  thin air.
  *
  *  If the body of `Local` references itself, then the `val Local\$jsclass` is
  *  instead declared as a `var` to work around the cyclic dependency:
@@ -526,15 +528,7 @@ class ExplicitJSClasses extends MiniPhase with InfoTransformer { thisPhase =>
         val typeRef = tree.tpe
         val clazzValue = clsOf(typeRef)
         val superClassCtor = genJSConstructorOf(tree, extractSuperTypeConstructor(tree.rhs))
-        val fakeNewInstances = {
-          /* We need to use `reverse` because the Scope returns elements in reverse order compared to tree definitions.
-           * The back-end needs the fake News to be in the same order as the corresponding tree definitions.
-           */
-          val ctors = cls.info.decls.lookupAll(nme.CONSTRUCTOR).toList.reverse
-          val elems = ctors.map(ctor => fakeNew(cls, ctor.asTerm))
-          JavaSeqLiteral(elems, TypeTree(defn.AnyRefType))
-        }
-        ref(jsdefn.Runtime_createLocalJSClass).appliedTo(clazzValue, superClassCtor, fakeNewInstances)
+        ref(jsdefn.Runtime_createLocalJSClass).appliedTo(clazzValue, superClassCtor, ref(defn.Predef_undefined))
       }
 
       val jsclassVal = state.localClass2jsclassVal(sym)
@@ -556,23 +550,6 @@ class ExplicitJSClasses extends MiniPhase with InfoTransformer { thisPhase =>
     }
   }
 
-  /** Creates a fake invocation of the the given class with the given constructor. */
-  def fakeNew(cls: ClassSymbol, ctor: TermSymbol)(using Context): Tree = {
-    /* TODO This is not entirely good enough, as it break -Ycheck for generic
-     * classes. Erasure restores the consistency of the fake invocations.
-     * Improving this is left for later.
-     */
-
-    val tycon = cls.typeRef
-    val targs = cls.typeParams.map(_ => TypeBounds.emptyPolyKind)
-    val argss = ctor.info.paramInfoss.map(_.map(_ => ref(defn.Predef_undefined)))
-
-    New(tycon)
-      .select(TermRef(tycon, ctor))
-      .appliedToTypes(targs)
-      .appliedToArgss(argss)
-  }
-
   // This method, together with transformTypeApply and transformSelect, implements step (E)
   override def transformApply(tree: Apply)(using Context): Tree = {
     if (!isFullyApplied(tree)) {

project/Build.scala

@@ -1009,6 +1009,9 @@ object Build {
       scalaJSLinkerConfig ~= { _.withSemantics(build.TestSuiteLinkerOptions.semantics _) },
       scalaJSModuleInitializers in Test ++= build.TestSuiteLinkerOptions.moduleInitializers,
 
+      // Perform Ycheck after the Scala.js-specific transformation phases
+      scalacOptions += "-Ycheck:explicitJSClasses,addLocalJSFakeNews",
+
       jsEnvInput in Test := {
         val resourceDir = fetchScalaJSSource.value / "test-suite/js/src/test/resources"
         val f = (resourceDir / "NonNativeJSTypeTestNatives.js").toPath