Fix scala-js#4929: Fix logic for moving early assignements in JS ctors.

Previously, the logic was based on moving statements after the
constructor by default, but keep selected ones before. The list
of what to keep turned out to be too restrictive, and it was not
clear how to expand it in a principled way.

Instead, we now reverse that logic. We keep statements where they
are by default, and only move `C.this.field = ident;` statements
after the super constructor call.

This requires only a little bit of special-casing for outer pointer
null checks. Fortunately, we already some logic to identify and
decompose those, which we reuse here.

compiler/src/main/scala/org/scalajs/nscplugin/GenJSCode.scala

@@ -1472,41 +1472,113 @@ abstract class GenJSCode[G <: Global with Singleton](val global: G)
       val sym = dd.symbol
       assert(sym.isPrimaryConstructor, s"called with non-primary ctor: $sym")
 
+      var preSuperStats = List.newBuilder[js.Tree]
       var jsSuperCall: Option[js.JSSuperConstructorCall] = None
-      val jsStats = List.newBuilder[js.Tree]
+      val postSuperStats = List.newBuilder[js.Tree]
 
       /* Move all statements after the super constructor call since JS
        * cannot access `this` before the super constructor call.
        *
        * scalac inserts statements before the super constructor call for early
        * initializers and param accessor initializers (including val's and var's
-       * declared in the params). We move those after the super constructor
-       * call, and are therefore executed later than for a Scala class.
+       * declared in the params). Those statements include temporary local `val`
+       * definitions (for true early accessors only) and the assignments, whose
+       * rhs'es are always simple Idents (either constructor params or the
+       * temporary local `val`s).
+       *
+       * There can also be local `val`s before the super constructor call for
+       * default arguments to the super constructor. These must remain before.
+       *
+       * Our strategy is therefore to move only the field assignments after the
+       * super constructor call. They are therefore executed later than for a
+       * Scala class (as specified for non-native JS classes semantics).
+       * However, side effects and evaluation order of all the other
+       * computations remains unchanged.
+       *
+       * For a somewhat extreme example of the shapes we can get here, consider
+       * the source code:
+       *
+       *   class Parent(output: Any = "output", callbackObject: Any = "callbackObject") extends js.Object {
+       *     println(s"Parent constructor; $output; $callbackObject")
+       *   }
+       *
+       *   class Child(val foo: Int, callbackObject: Any, val bar: Int) extends {
+       *     val xyz = foo + bar
+       *     val yz = { println(xyz); xyz + 2 }
+       *   } with Parent(callbackObject = { println(foo); xyz + bar }) {
+       *     println("Child constructor")
+       *     println(xyz)
+       *   }
+       *
+       * At this phase, for the constructor of `Child`, we receive the following
+       * scalac Tree:
+       *
+       *   def <init>(foo: Int, callbackObject: Object, bar: Int): helloworld.Child = {
+       *     Child.this.foo = foo; // param accessor assignment, moved
+       *     Child.this.bar = bar; // param accessor assignment, moved
+       *     val xyz: Int = foo.+(bar); // note that these still use the ctor params, not the fields
+       *     Child.this.xyz = xyz; // early initializer, moved
+       *     val yz: Int = {
+       *       scala.Predef.println(scala.Int.box(xyz)); // note that this uses the local val, not the field
+       *       xyz.+(2)
+       *     };
+       *     Child.this.yz = yz; // early initializer, moved
+       *     {
+       *       <artifact> val x$1: Int = {
+       *         scala.Predef.println(scala.Int.box(foo));
+       *         xyz.+(bar) // here as well, we use the local vals, not the fields
+       *       };
+       *       <artifact> val x$2: Object = helloworld.this.Parent.<init>$default$1();
+       *       Child.super.<init>(x$2, scala.Int.box(x$1))
+       *     };
+       *     scala.Predef.println("Child constructor");
+       *     scala.Predef.println(scala.Int.box(Child.this.xyz()));
+       *     ()
+       *   }
+       *
        */
       withPerMethodBodyState(sym) {
+        def isThisFieldAssignment(tree: Tree): Boolean = tree match {
+          case Assign(Select(ths: This, _), Ident(_)) => ths.symbol == currentClassSym.get
+          case _                                      => false
+        }
+
         flatStats(stats).foreach {
           case tree @ Apply(fun @ Select(Super(This(_), _), _), args)
                 if fun.symbol.isClassConstructor =>
             assert(jsSuperCall.isEmpty, s"Found 2 JS Super calls at ${dd.pos}")
             implicit val pos = tree.pos
             jsSuperCall = Some(js.JSSuperConstructorCall(genPrimitiveJSArgs(fun.symbol, args)))
 
-          case stat =>
-            val jsStat = genStat(stat)
+          case tree if jsSuperCall.isDefined =>
+            // Once we're past the super constructor call, everything goes after.
+            postSuperStats += genStat(tree)
 
-            assert(jsSuperCall.isDefined || !jsStat.isInstanceOf[js.VarDef],
-                "Trying to move a local VarDef after the super constructor call " +
-                s"of a non-native JS class at ${dd.pos}")
+          case tree if isThisFieldAssignment(tree) =>
+            /* If that shape appears before the jsSuperCall, it is an early
+             * initializer or param accessor initializer. We move it.
+             */
+            postSuperStats += genStat(tree)
+
+          case tree @ OuterPointerNullCheck(outer, assign) if isThisFieldAssignment(assign) =>
+            /* Variant of the above with an outer pointer null check. The actual
+             * null check remains before the super call, while the associated
+             * assignment is moved after.
+             */
+            preSuperStats += js.GetClass(genExpr(outer))(tree.pos)
+            postSuperStats += genStat(assign)
 
-            jsStats += jsStat
+          case stat =>
+            // Other statements are left before.
+            preSuperStats += genStat(stat)
         }
       }
 
       assert(jsSuperCall.isDefined, "Did not find Super call in primary JS " +
           s"construtor at ${dd.pos}")
 
       new PrimaryJSCtor(sym, genParamsAndInfo(sym, vparamss),
-          js.JSConstructorBody(Nil, jsSuperCall.get, jsStats.result())(dd.pos))
+          js.JSConstructorBody(preSuperStats.result(), jsSuperCall.get, postSuperStats.result())(dd.pos))
     }
 
     private def genSecondaryJSClassCtor(dd: DefDef): SplitSecondaryJSCtor = {
@@ -2301,9 +2373,7 @@ abstract class GenJSCode[G <: Global with Singleton](val global: G)
              *   this.$outer = $outer // the `else` branch in general
              */
             tree match {
-              case If(Apply(fun @ Select(outer: Ident, nme.eq), Literal(Constant(null)) :: Nil),
-                  Throw(Literal(Constant(null))), elsep)
-                  if outer.symbol.isOuterParam && fun.symbol == definitions.Object_eq =>
+              case OuterPointerNullCheck(outer, elsep) =>
                 js.Block(
                   js.GetClass(genExpr(outer)), // null check
                   genStat(elsep)
@@ -2566,6 +2636,25 @@ abstract class GenJSCode[G <: Global with Singleton](val global: G)
       }
     } // end of GenJSCode.genExpr()
 
+    /** Extractor for the shape of outer pointer null check.
+     *
+     *  See the comment in `case If(...) =>` of `genExpr`.
+     *
+     *  When successful, returns the pair `(outer, elsep)` where `outer` is the
+     *  `Ident` of the outer constructor parameter, and `elsep` is the else
+     *  branch of the condition.
+     */
+    private object OuterPointerNullCheck {
+      def unapply(tree: If): Option[(Ident, Tree)] = tree match {
+        case If(Apply(fun @ Select(outer: Ident, nme.eq), Literal(Constant(null)) :: Nil),
+            Throw(Literal(Constant(null))), elsep)
+            if outer.symbol.isOuterParam && fun.symbol == definitions.Object_eq =>
+          Some((outer, elsep))
+        case _ =>
+          None
+      }
+    }
+
     /** Gen JS this of the current class.
      *  Normally encoded straightforwardly as a JS this.
      *  But must be replaced by the tail-jump-this local variable if there

test-suite/js/src/test/require-scala2/org/scalajs/testsuite/jsinterop/NonNativeJSTypeTestScala2.scala

@@ -12,6 +12,8 @@
 
 package org.scalajs.testsuite.jsinterop
 
+import scala.collection.mutable
+
 import scala.scalajs.js
 import scala.scalajs.js.annotation._
 
@@ -36,6 +38,23 @@ class NonNativeJSTypeTestScala2 {
     assertNull(obj.asInstanceOf[js.Dynamic].valueClass)
   }
 
+  @Test def callSuperConstructorWithDefaultParamsAndEarlyInitializers_Issue4929(): Unit = {
+    import ConstructorSuperCallWithDefaultParamsAndEarlyInitializers._
+
+    sideEffects.clear()
+
+    val child = new Child(4, "hello", 23)
+    assertEquals(4, child.foo)
+    assertEquals(23, child.bar)
+    assertEquals(27, child.xyz)
+    assertEquals(29, child.yz)
+
+    assertEquals(
+      List("27", "4", "Parent constructor; param1, 27", "Child constructor; 4, hello, 23", "27, 29"),
+      sideEffects.toList
+    )
+  }
+
 }
 
 object NonNativeJSTypeTestScala2 {
@@ -46,4 +65,20 @@ object NonNativeJSTypeTestScala2 {
 
   class SomeValueClass(val i: Int) extends AnyVal
 
+  object ConstructorSuperCallWithDefaultParamsAndEarlyInitializers {
+    val sideEffects = mutable.ListBuffer.empty[String]
+
+    class Parent(parentParam1: Any = "param1", parentParam2: Any = "param2") extends js.Object {
+      sideEffects += s"Parent constructor; $parentParam1, $parentParam2"
+    }
+
+    class Child(val foo: Int, parentParam2: Any, val bar: Int) extends {
+      val xyz = foo + bar
+      val yz = { sideEffects += xyz.toString(); xyz + 2 }
+    } with Parent(parentParam2 = { sideEffects += foo.toString(); foo + bar }) {
+      sideEffects += s"Child constructor; $foo, $parentParam2, $bar"
+      sideEffects += s"$xyz, $yz"
+    }
+  }
+
 }

test-suite/js/src/test/scala/org/scalajs/testsuite/jsinterop/NonNativeJSTypeTest.scala

@@ -12,6 +12,8 @@
 
 package org.scalajs.testsuite.jsinterop
 
+import scala.collection.mutable
+
 import scala.scalajs.js
 import scala.scalajs.js.annotation._
 
@@ -1306,6 +1308,21 @@ class NonNativeJSTypeTest {
     assertEquals(js.undefined, foo4.description)
   }
 
+  @Test def callSuperConstructorWithDefaultParams_Issue4929(): Unit = {
+    import ConstructorSuperCallWithDefaultParams._
+
+    sideEffects.clear()
+
+    val child = new Child(4, "hello", 23)
+    assertEquals(4, child.foo)
+    assertEquals(23, child.bar)
+
+    assertEquals(
+      List("4", "Parent constructor; param1, 27", "Child constructor; 4, hello, 23"),
+      sideEffects.toList
+    )
+  }
+
   @Test def callSuperConstructorWithColonAsterisk(): Unit = {
     class CallSuperCtorWithSpread(x: Int, y: Int, z: Int)
         extends NativeParentClassWithVarargs(x, Seq(y, z): _*)
@@ -2040,6 +2057,19 @@ object NonNativeJSTypeTest {
     def this(x: Int, y: Int) = this(x)(y.toString, js.undefined)
   }
 
+  object ConstructorSuperCallWithDefaultParams {
+    val sideEffects = mutable.ListBuffer.empty[String]
+
+    class Parent(parentParam1: Any = "param1", parentParam2: Any = "param2") extends js.Object {
+      sideEffects += s"Parent constructor; $parentParam1, $parentParam2"
+    }
+
+    class Child(val foo: Int, parentParam2: Any, val bar: Int)
+        extends Parent(parentParam2 = { sideEffects += foo.toString(); foo + bar }) {
+      sideEffects += s"Child constructor; $foo, $parentParam2, $bar"
+    }
+  }
+
   class OverloadedConstructorParamNumber(val foo: Int) extends js.Object {
     def this(x: Int, y: Int) = this(x + y)
     def this(x: Int, y: Int, z: Int) = this(x + y, z)