Fix #17549: Unify how Memoize and Constructors decide what fields need storing. (#17560)

The Memoize and Constructors have to work together and agree on which
`final val`s actually need storing in a field. Previously, they used
slightly different criteria: one on the result type, and one on the rhs
(with an additional Scala.js-specific eligibility condition). That
discrepancy resulted in the crash/wrong codegen in the issue.

We now unify both: we avoid a field if and only if all of the following
apply:

* it is a `final val`,
* its result *type* is a `ConstantType`, and
* it is eligible according to Scala.js semantics.

In particular, there is no condition on the right-hand-side. We avoid a
field even if the right-hand-side has side effects. The side effects are
moved to the constructor regardless.

---

This introduces both progressions and regressions in the amount of
fields we generate. We can avoid fields even for side-effecting rhs'es,
as long as the result type is constant. On the other hand, we now create
a field for `final val`s with non-constant result type, even if the rhs
is a literal.

While the latter is a regression for Scala 3, it aligns with the
behavior of Scala 2. It also has the nice benefit that whether or not a
val has a field is now independent of its *implementation*, and only
dependent on its *API*. Overall, I think this is a trade-off worth
taking.

We could reintroduce that optimization in the future (but in classes
only; not in traits), if we really want to, although that would require
dedicated code.

Author: smarter

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

@@ -226,31 +226,39 @@ class Constructors extends MiniPhase with IdentityDenotTransformer { thisPhase =
               constrStats += intoConstr(stat, sym)
             } else
               dropped += sym
-          case stat @ DefDef(name, _, tpt, _)
-              if stat.symbol.isGetter && stat.symbol.owner.is(Trait) && !stat.symbol.is(Lazy) && !stat.symbol.isConstExprFinalVal =>
+          case stat @ DefDef(name, _, tpt, _) if stat.symbol.isGetter && !stat.symbol.is(Lazy) =>
             val sym = stat.symbol
             assert(isRetained(sym), sym)
-            if !stat.rhs.isEmpty && !isWildcardArg(stat.rhs) then
-              /* !!! Work around #9390
-               * This should really just be `sym.setter`. However, if we do that, we'll miss
-               * setters for mixed in `private var`s. Even though the scope clearly contains the
-               * setter symbol with the correct Name structure (since the `find` finds it),
-               * `.decl(setterName)` used by `.setter` through `.accessorNamed` will *not* find it.
-               * Could it be that the hash table of the `Scope` is corrupted?
-               * We still try `sym.setter` first as an optimization, since it will work for all
-               * public vars in traits and all (public or private) vars in classes.
-               */
-              val symSetter =
-                if sym.setter.exists then
-                  sym.setter
-                else
-                  val setterName = sym.asTerm.name.setterName
-                  sym.owner.info.decls.find(d => d.is(Accessor) && d.name == setterName)
-              val setter =
-                if (symSetter.exists) symSetter
-                else sym.accessorNamed(Mixin.traitSetterName(sym.asTerm))
-              constrStats += Apply(ref(setter), intoConstr(stat.rhs, sym).withSpan(stat.span) :: Nil)
-            clsStats += cpy.DefDef(stat)(rhs = EmptyTree)
+            if sym.isConstExprFinalVal then
+              if stat.rhs.isInstanceOf[Literal] then
+                clsStats += stat
+              else
+                constrStats += intoConstr(stat.rhs, sym)
+                clsStats += cpy.DefDef(stat)(rhs = Literal(sym.constExprFinalValConstantType.value).withSpan(stat.span))
+            else if !sym.owner.is(Trait) then
+              clsStats += stat
+            else
+              if !stat.rhs.isEmpty && !isWildcardArg(stat.rhs) then
+                /* !!! Work around #9390
+                 * This should really just be `sym.setter`. However, if we do that, we'll miss
+                 * setters for mixed in `private var`s. Even though the scope clearly contains the
+                 * setter symbol with the correct Name structure (since the `find` finds it),
+                 * `.decl(setterName)` used by `.setter` through `.accessorNamed` will *not* find it.
+                 * Could it be that the hash table of the `Scope` is corrupted?
+                 * We still try `sym.setter` first as an optimization, since it will work for all
+                 * public vars in traits and all (public or private) vars in classes.
+                 */
+                val symSetter =
+                  if sym.setter.exists then
+                    sym.setter
+                  else
+                    val setterName = sym.asTerm.name.setterName
+                    sym.owner.info.decls.find(d => d.is(Accessor) && d.name == setterName)
+                val setter =
+                  if (symSetter.exists) symSetter
+                  else sym.accessorNamed(Mixin.traitSetterName(sym.asTerm))
+                constrStats += Apply(ref(setter), intoConstr(stat.rhs, sym).withSpan(stat.span) :: Nil)
+              clsStats += cpy.DefDef(stat)(rhs = EmptyTree)
           case DefDef(nme.CONSTRUCTOR, ((outerParam @ ValDef(nme.OUTER, _, _)) :: _) :: Nil, _, _) =>
             clsStats += mapOuter(outerParam.symbol).transform(stat)
           case _: DefTree =>

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

@@ -20,8 +20,6 @@ import sjs.JSSymUtils._
 
 import util.Store
 
-import dotty.tools.backend.sjs.JSDefinitions.jsdefn
-
 object Memoize {
   val name: String = "memoize"
   val description: String = "add private fields to getters and setters"
@@ -130,32 +128,17 @@ class Memoize extends MiniPhase with IdentityDenotTransformer { thisPhase =>
       }
 
     if sym.is(Accessor, butNot = NoFieldNeeded) then
-      /* Tests whether the semantics of Scala.js require a field for this symbol, irrespective of any
-       * optimization we think we can do. This is the case if one of the following is true:
-       * - it is a member of a JS type, since it needs to be visible as a JavaScript field
-       * - is is exported as static member of the companion class, since it needs to be visible as a JavaScript static field
-       * - it is exported to the top-level, since that can only be done as a true top-level variable, i.e., a field
-       */
-      def sjsNeedsField: Boolean =
-        ctx.settings.scalajs.value && (
-          sym.owner.isJSType
-            || sym.hasAnnotation(jsdefn.JSExportTopLevelAnnot)
-            || sym.hasAnnotation(jsdefn.JSExportStaticAnnot)
-        )
-
       def adaptToField(field: Symbol, tree: Tree): Tree =
         if (tree.isEmpty) tree else tree.ensureConforms(field.info.widen)
 
       def isErasableBottomField(field: Symbol, cls: Symbol): Boolean =
         !field.isVolatile
           && ((cls eq defn.NothingClass) || (cls eq defn.NullClass) || (cls eq defn.BoxedUnitClass))
-          && !sjsNeedsField
+          && !sym.sjsNeedsField
 
       if sym.isGetter then
-        val constantFinalVal =
-          sym.isAllOf(Accessor | Final, butNot = Mutable) && tree.rhs.isInstanceOf[Literal] && !sjsNeedsField
-        if constantFinalVal then
-          // constant final vals do not need to be transformed at all, and do not need a field
+        if sym.isConstExprFinalVal then
+          // const-expr final vals do not need to be transformed at all, and do not need a field
           tree
         else
           val field = newField.asTerm

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

@@ -18,6 +18,8 @@ import Annotations.Annotation
 import Phases._
 import ast.tpd.Literal
 
+import dotty.tools.dotc.transform.sjs.JSSymUtils.sjsNeedsField
+
 import scala.annotation.tailrec
 
 object SymUtils:
@@ -259,9 +261,29 @@ object SymUtils:
       self.owner.info.decl(fieldName).suchThat(!_.is(Method)).symbol
     }
 
+    /** Is this symbol a constant expression final val?
+     *
+     *  This is the case if all of the following are true:
+     *
+     *  - it is a `final val`,
+     *  - its result type is a `ConstantType`, and
+     *  - it does not need an explicit field because of Scala.js semantics (see `JSSymUtils.sjsNeedsField`).
+     *
+     *  Constant expression final vals do not need an explicit field to store
+     *  their value. See the Memoize-Mixin-Constructors phase trio.
+     */
     def isConstExprFinalVal(using Context): Boolean =
       atPhaseNoLater(erasurePhase) {
-        self.is(Final) && self.info.resultType.isInstanceOf[ConstantType]
+        self.is(Final, butNot = Mutable) && self.info.resultType.isInstanceOf[ConstantType]
+      } && !self.sjsNeedsField
+
+    /** The `ConstantType` of a val known to be `isConstrExprFinalVal`.
+     *
+     *  @pre `self.isConstantExprFinalVal` is true.
+     */
+    def constExprFinalValConstantType(using Context): ConstantType =
+      atPhaseNoLater(erasurePhase) {
+        self.info.resultType.asInstanceOf[ConstantType]
       }
 
     def isField(using Context): Boolean =

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

@@ -211,6 +211,23 @@ object JSSymUtils {
         }
       }
     }
+
+    /** Tests whether the semantics of Scala.js require a field for this symbol,
+     *  irrespective of any optimization we think we can do.
+     *
+     *  This is the case if one of the following is true:
+     *
+     *  - it is a member of a JS type, since it needs to be visible as a JavaScript field
+     *  - is is exported as static member of the companion class, since it needs to be visible as a JavaScript static field
+     *  - it is exported to the top-level, since that can only be done as a true top-level variable, i.e., a field
+     */
+    def sjsNeedsField(using Context): Boolean =
+      ctx.settings.scalajs.value && (
+        sym.owner.isJSType
+          || sym.hasAnnotation(jsdefn.JSExportTopLevelAnnot)
+          || sym.hasAnnotation(jsdefn.JSExportStaticAnnot)
+      )
+    end sjsNeedsField
   }
 
   private object JSUnaryOpMethodName {

tests/run/erased-inline-vals.scala

@@ -16,6 +16,27 @@ class D:
   inline def x: Int = 5
   inline val y = 6
 
+object SideEffects:
+  val sideEffects = scala.collection.mutable.ListBuffer.empty[String]
+
+trait E:
+  final val a: 7 =
+    SideEffects.sideEffects += "E.a"
+    7
+  final val b =
+    SideEffects.sideEffects += "E.b"
+    8
+end E
+
+class F extends E:
+  final val c: 9 =
+    SideEffects.sideEffects += "F.c"
+    9
+  final val d =
+    SideEffects.sideEffects += "F.d"
+    10
+end F
+
 @main def Test =
   val b: B = new B
   assert(b.x == 1)
@@ -37,12 +58,24 @@ class D:
   assert(d.x == 5)
   assert(d.y == 6)
 
+  val f: F = new F
+  assert(SideEffects.sideEffects.toList == List("E.a", "E.b", "F.c", "F.d"))
+  assert(f.a == 7)
+  assert(f.b == 8)
+  assert(f.c == 9)
+  assert(f.d == 10)
 
   assert(classOf[B].getDeclaredMethods.size == 2)
   assert(classOf[B].getDeclaredFields.isEmpty)
 
   assert(classOf[C].getDeclaredMethods.size == 2)
-  assert(classOf[C].getDeclaredFields.isEmpty)
+  assert(classOf[C].getDeclaredFields.size == 1) // x, but not y
 
   assert(classOf[D].getDeclaredMethods.isEmpty)
   assert(classOf[D].getFields.isEmpty)
+
+  assert(classOf[E].getDeclaredMethods.size == 5)
+  assert(classOf[E].getDeclaredFields.isEmpty)
+
+  assert(classOf[F].getDeclaredMethods.size == 2)
+  assert(classOf[F].getDeclaredFields.isEmpty)

tests/run/final-fields.check

@@ -2,6 +2,6 @@ T.f1
 T.f2
 T.f3
 T.f4
-3 2 -3 -4
+3 2 3 -4
 3
 g

tests/run/i17549.check

@@ -0,0 +1,6 @@
+T.x
+C.y
+1
+2
+1
+2

tests/run/i17549.scala

@@ -0,0 +1,27 @@
+trait T:
+  final val x: 1 =
+    println("T.x")
+    1
+end T
+
+trait U:
+  def x: Any
+  def y: Any
+
+class C extends T with U:
+  final val y: 2 =
+    println("C.y")
+    2
+end C
+
+object Test:
+  def main(args: Array[String]): Unit =
+    val c = new C
+    println(c.x)
+    println(c.y)
+
+    val u: U = c
+    println(u.x)
+    println(u.y)
+  end main
+end Test