Lambda-owned param ref in ctor incurs no field

Author: som-snytt

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

@@ -45,10 +45,10 @@ class Constructors extends MiniPhase with IdentityDenotTransformer { thisPhase =
     // to more symbols being retained as parameters. Test case in run/capturing.scala.
 
   /** The private vals that are known to be retained as class fields */
-  private val retainedPrivateVals = mutable.Set[Symbol]()
+  private val retainedPrivateVals = mutable.Set.empty[Symbol]
 
   /** The private vals whose definition comes before the current focus */
-  private val seenPrivateVals = mutable.Set[Symbol]()
+  private val seenPrivateVals = mutable.Set.empty[Symbol]
 
   // Collect all private parameter accessors and value definitions that need
   // to be retained. There are several reasons why a parameter accessor or
@@ -57,31 +57,36 @@ class Constructors extends MiniPhase with IdentityDenotTransformer { thisPhase =
   // 2. It is accessed before it is defined
   // 3. It is accessed on an object other than `this`
   // 4. It is a mutable parameter accessor
-  // 5. It is has a wildcard initializer `_`
-  private def markUsedPrivateSymbols(tree: RefTree)(using Context): Unit = {
+  // 5. It has a wildcard initializer `_`
+  private def markUsedPrivateSymbols(tree: RefTree)(using Context): Unit =
 
     val sym = tree.symbol
     def retain() = retainedPrivateVals.add(sym)
 
-    if (sym.exists && sym.owner.isClass && mightBeDropped(sym)) {
-      val owner = sym.owner.asClass
-
-        tree match {
-          case Ident(_) | Select(This(_), _) =>
-            def inConstructor = {
-              val method = ctx.owner.enclosingMethod
-              method.isPrimaryConstructor && ctx.owner.enclosingClass == owner
-            }
-            if (inConstructor &&
-                (sym.is(ParamAccessor) || seenPrivateVals.contains(sym))) {
-              // used inside constructor, accessed on this,
-              // could use constructor argument instead, no need to retain field
-            }
-            else retain()
-          case _ => retain()
-      }
-    }
-  }
+    if sym.exists && sym.owner.isClass && sym.mightBeDropped then
+      tree match
+      case Ident(_) | Select(This(_), _) =>
+        val method = ctx.owner.enclosingMethod
+        // template exprs are moved (below) to constructor, where lifted anonfun will take its captured env as an arg
+        inline def methodOwnerIsLocalDummyOrSibling =
+          val owner = method.owner
+          owner.isLocalDummy || owner.owner == sym.owner && !owner.isOneOf(MethodOrLazy)
+        inline def inAnonFunInCtor =
+             method.isAnonymousFunction
+          && methodOwnerIsLocalDummyOrSibling
+          && !sym.owner.is(Module) // lambdalift doesn't transform correctly (to do)
+        val inConstructor =
+             (method.isPrimaryConstructor || inAnonFunInCtor)
+          && ctx.owner.enclosingClass == sym.owner
+        val noField =
+             inConstructor
+          && (sym.is(ParamAccessor) || seenPrivateVals.contains(sym))
+          // used inside constructor, accessed on this,
+          // could use constructor argument instead, no need to retain field
+        if !noField then
+          retain()
+      case _ =>
+        retain()
 
   override def transformIdent(tree: tpd.Ident)(using Context): tpd.Tree = {
     markUsedPrivateSymbols(tree)
@@ -94,7 +99,7 @@ class Constructors extends MiniPhase with IdentityDenotTransformer { thisPhase =
   }
 
   override def transformValDef(tree: tpd.ValDef)(using Context): tpd.Tree = {
-    if (mightBeDropped(tree.symbol)) seenPrivateVals += tree.symbol
+    if tree.symbol.mightBeDropped then seenPrivateVals += tree.symbol
     tree
   }
 
@@ -123,7 +128,7 @@ class Constructors extends MiniPhase with IdentityDenotTransformer { thisPhase =
   /** Class members that can be eliminated if referenced only from their own
    *  constructor.
    */
-  private def mightBeDropped(sym: Symbol)(using Context) =
+  extension (sym: Symbol) private def mightBeDropped(using Context) =
     sym.is(Private, butNot = MethodOrLazy) && !sym.isAllOf(MutableParamAccessor)
 
   private final val MutableParamAccessor = Mutable | ParamAccessor
@@ -184,8 +189,9 @@ class Constructors extends MiniPhase with IdentityDenotTransformer { thisPhase =
         transform(tree).changeOwnerAfter(prevOwner, constr.symbol, thisPhase)
     }
 
+    // mightBeDropped is trivially false for NoSymbol -> NoSymbol isRetained
     def isRetained(acc: Symbol) =
-      !mightBeDropped(acc) || retainedPrivateVals(acc)
+      !acc.mightBeDropped || retainedPrivateVals(acc)
 
     val constrStats, clsStats = new mutable.ListBuffer[Tree]
 
@@ -209,30 +215,28 @@ class Constructors extends MiniPhase with IdentityDenotTransformer { thisPhase =
       }
     }
 
-    val dropped = mutable.Set[Symbol]()
+    val dropped = mutable.Set.empty[Symbol]
 
     // Split class body into statements that go into constructor and
     // definitions that are kept as members of the class.
-    def splitStats(stats: List[Tree]): Unit = stats match {
-      case stat :: stats1 =>
+    def splitStats(stats: List[Tree]): Unit = stats match
+      case stat :: stats =>
+        val sym = stat.symbol
         stat match {
-          case stat @ ValDef(name, tpt, _) if !stat.symbol.is(Lazy) && !stat.symbol.hasAnnotation(defn.ScalaStaticAnnot) =>
-            val sym = stat.symbol
+          case stat @ ValDef(name, tpt, _) if !sym.is(Lazy) && !sym.hasAnnotation(defn.ScalaStaticAnnot) =>
             if (isRetained(sym)) {
               if (!stat.rhs.isEmpty && !isWildcardArg(stat.rhs))
                 constrStats += Assign(ref(sym), intoConstr(stat.rhs, sym)).withSpan(stat.span)
               clsStats += cpy.ValDef(stat)(rhs = EmptyTree)
             }
-            else if (!stat.rhs.isEmpty) {
-              dropped += sym
-              sym.copySymDenotation(
-                initFlags = sym.flags &~ Private,
-                owner = constr.symbol).installAfter(thisPhase)
-              constrStats += intoConstr(stat, sym)
-            } else
+            else
               dropped += sym
+              if !stat.rhs.isEmpty then
+                sym.copySymDenotation(
+                  initFlags = sym.flags &~ Private,
+                  owner = constr.symbol).installAfter(thisPhase)
+                constrStats += intoConstr(stat, sym)
           case stat @ DefDef(name, _, tpt, _) if stat.symbol.isGetter && !stat.symbol.is(Lazy) =>
-            val sym = stat.symbol
             assert(isRetained(sym), sym)
             if sym.isConstExprFinalVal then
               if stat.rhs.isInstanceOf[Literal] then
@@ -271,9 +275,9 @@ class Constructors extends MiniPhase with IdentityDenotTransformer { thisPhase =
           case _ =>
             constrStats += intoConstr(stat, tree.symbol)
         }
-        splitStats(stats1)
+        splitStats(stats)
       case Nil =>
-    }
+    end splitStats
 
     /** Check that we do not have both a private field with name `x` and a private field
      *  with name `FieldName(x)`. These will map to the same JVM name and therefore cause
@@ -303,14 +307,15 @@ class Constructors extends MiniPhase with IdentityDenotTransformer { thisPhase =
         dropped += acc
         Nil
       }
-      else if (!isRetained(acc.field)) { // It may happen for unit fields, tests/run/i6987.scala
+      else if (acc.field.exists && !isRetained(acc.field)) { // It may happen for unit fields, tests/run/i6987.scala
         dropped += acc.field
         Nil
       }
       else {
         val param = acc.subst(accessors, paramSyms)
-        if (param.hasAnnotation(defn.ConstructorOnlyAnnot))
-          report.error(em"${acc.name} is marked `@constructorOnly` but it is retained as a field in ${acc.owner}", acc.srcPos)
+        if param.hasAnnotation(defn.ConstructorOnlyAnnot) then
+          val msg = em"${acc.name} is marked `@constructorOnly` but it is retained as a field in ${acc.owner}"
+          report.error(msg, acc.srcPos)
         val target = if (acc.is(Method)) acc.field else acc
         if (!target.exists) Nil // this case arises when the parameter accessor is an alias
         else {

tests/neg/i22979.scala

@@ -0,0 +1,11 @@
+
+import annotation.*
+
+class C(@constructorOnly s: String): // error
+  def g: Any => String = _ => s
+  def f[A](xs: List[A]): List[String] = xs.map(g)
+
+import scala.util.boundary
+
+class Leak()(using @constructorOnly l: boundary.Label[String]): // error
+  lazy val broken = Option("stop").foreach(boundary.break(_))

tests/pos/i22979.scala

@@ -0,0 +1,18 @@
+
+import annotation.*
+
+class C(@constructorOnly s: String):
+  val g: Any => String = _ => s
+  def f[A](xs: List[A]): List[String] = xs.map(g)
+
+import scala.util.boundary
+
+class Leak()(using @constructorOnly l: boundary.Label[String]):
+  Option("stop").foreach(boundary.break(_))
+
+
+class Lapse:
+  def f = Lapse.DefaultSentinelFn()
+object Lapse:
+  private val DefaultSentinel: AnyRef = new AnyRef
+  private val DefaultSentinelFn: () => AnyRef = () => DefaultSentinel

tests/run/i22979/Leak.scala

@@ -0,0 +1,13 @@
+import annotation.*
+
+class Leak()(using @constructorOnly l: boundary.Label[String]) {
+  Seq("a", "b").foreach(_ => boundary.break("stop"))
+}
+
+object boundary {
+  final class Break[T] private[boundary](val label: Label[T], val value: T)
+    extends RuntimeException(
+          /*message*/ null, /*cause*/ null, /*enableSuppression=*/ false, /*writableStackTrace*/ false)
+  final class Label[-T]
+  def break[T](value: T)(using label: Label[T]): Nothing = throw new Break(label, value)
+}

tests/run/i22979/test.scala

@@ -0,0 +1,23 @@
+// scalajs: --skip
+
+import util.Try
+
+@main def Test =
+  assert(Try(classOf[Leak].getDeclaredField("l")).isFailure)
+  assert(classOf[Leak].getFields.length == 0)
+  //classOf[Leak].getFields.map(_.getName).foreach(println) //DEBUG
+  assert(classOf[C].getFields.length == 0)
+  //classOf[Lapse.type].getFields.map(_.getName).foreach(println) //DEBUG
+
+class C:
+  private val x = 42
+  println(x)
+  println(List(27).map(_ + x))
+
+// The easy tweak for lambdas does not work for module owner.
+// The lambdalifted anonfun is not transformed correctly.
+class Lapse:
+  def f = Lapse.DefaultSentinelFn()
+object Lapse:
+  private val DefaultSentinel: AnyRef = new AnyRef
+  private val DefaultSentinelFn: () => AnyRef = () => DefaultSentinel