Merge pull request #14205 from som-snytt/issue/14198

Clear temp var for captured var expr to permit GC

Author: smarter

compiler/src/dotty/tools/dotc/ast/tpd.scala

@@ -206,8 +206,8 @@ object tpd extends Trees.Instance[Type] with TypedTreeInfo {
   def ValDef(sym: TermSymbol, rhs: LazyTree = EmptyTree)(using Context): ValDef =
     ta.assignType(untpd.ValDef(sym.name, TypeTree(sym.info), rhs), sym)
 
-  def SyntheticValDef(name: TermName, rhs: Tree)(using Context): ValDef =
-    ValDef(newSymbol(ctx.owner, name, Synthetic, rhs.tpe.widen, coord = rhs.span), rhs)
+  def SyntheticValDef(name: TermName, rhs: Tree, flags: FlagSet = EmptyFlags)(using Context): ValDef =
+    ValDef(newSymbol(ctx.owner, name, Synthetic | flags, rhs.tpe.widen, coord = rhs.span), rhs)
 
   def DefDef(sym: TermSymbol, paramss: List[List[Symbol]],
              resultType: Type, rhs: Tree)(using Context): DefDef =

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

@@ -10,14 +10,16 @@ import core.Decorators._
 import core.StdNames.nme
 import core.Names._
 import core.NameKinds.TempResultName
+import core.Constants._
 import ast.Trees._
 import util.Store
 import collection.mutable
 
 /** This phase translates variables that are captured in closures to
  *  heap-allocated refs.
  */
-class CapturedVars extends MiniPhase with IdentityDenotTransformer { thisPhase =>
+class CapturedVars extends MiniPhase with IdentityDenotTransformer:
+  thisPhase =>
   import ast.tpd._
 
   /** the following two members override abstract members in Transform */
@@ -45,6 +47,9 @@ class CapturedVars extends MiniPhase with IdentityDenotTransformer { thisPhase =
 
     val boxedRefClasses: collection.Set[Symbol] =
       refClassKeys.flatMap(k => Set(refClass(k), volatileRefClass(k)))
+
+    val objectRefClasses: collection.Set[Symbol] =
+      Set(refClass(defn.ObjectClass), volatileRefClass(defn.ObjectClass))
   }
 
   private var myRefInfo: RefInfo = null
@@ -123,32 +128,42 @@ class CapturedVars extends MiniPhase with IdentityDenotTransformer { thisPhase =
   }
 
   /** If assignment is to a boxed ref type, e.g.
-    *
-    *      intRef.elem = expr
-    *
-    *  rewrite using a temporary var to
-    *
-    *      val ev$n = expr
-    *      intRef.elem = ev$n
-    *
-    *  That way, we avoid the problem that `expr` might contain a `try` that would
-    *  run on a non-empty stack (which is illegal under JVM rules). Note that LiftTry
-    *  has already run before, so such `try`s would not be eliminated.
-    *
-    *  Also: If the ref type lhs is followed by a cast (can be an artifact of nested translation),
-    *  drop the cast.
-    */
-  override def transformAssign(tree: Assign)(using Context): Tree = {
-    def recur(lhs: Tree): Tree = lhs match {
-      case TypeApply(Select(qual, nme.asInstanceOf_), _) =>
-        val Select(_, nme.elem) = qual
+   *
+   *      intRef.elem = expr
+   *
+   *  rewrite using a temporary var to
+   *
+   *      val ev$n = expr
+   *      intRef.elem = ev$n
+   *
+   *  That way, we avoid the problem that `expr` might contain a `try` that would
+   *  run on a non-empty stack (which is illegal under JVM rules). Note that LiftTry
+   *  has already run before, so such `try`s would not be eliminated.
+   *
+   *  If the ref type lhs is followed by a cast (can be an artifact of nested translation),
+   *  drop the cast.
+   *
+   *  If the ref type is `ObjectRef` or `VolatileObjectRef`, immediately assign `null`
+   *  to the temporary to make the underlying target of the reference available for
+   *  garbage collection. Nullification is omitted if the `expr` is already `null`.
+   *
+   *      var ev$n: RHS = expr
+   *      objRef.elem = ev$n
+   *      ev$n = null.asInstanceOf[RHS]
+   */
+  override def transformAssign(tree: Assign)(using Context): Tree =
+    def absolved: Boolean = tree.rhs match
+      case Literal(Constant(null)) | Typed(Literal(Constant(null)), _) => true
+      case _ => false
+    def recur(lhs: Tree): Tree = lhs match
+      case TypeApply(Select(qual@Select(_, nme.elem), nme.asInstanceOf_), _) =>
         recur(qual)
       case Select(_, nme.elem) if refInfo.boxedRefClasses.contains(lhs.symbol.maybeOwner) =>
-        val tempDef = transformFollowing(SyntheticValDef(TempResultName.fresh(), tree.rhs))
-        transformFollowing(Block(tempDef :: Nil, cpy.Assign(tree)(lhs, ref(tempDef.symbol))))
+        val tempDef = transformFollowing(SyntheticValDef(TempResultName.fresh(), tree.rhs, flags = Mutable))
+        val update  = cpy.Assign(tree)(lhs, ref(tempDef.symbol))
+        def reset   = cpy.Assign(tree)(ref(tempDef.symbol), nullLiteral.cast(tempDef.symbol.info))
+        val res     = if refInfo.objectRefClasses(lhs.symbol.maybeOwner) && !absolved then reset else unitLiteral
+        transformFollowing(Block(tempDef :: update :: Nil, res))
       case _ =>
         tree
-    }
     recur(tree.lhs)
-  }
-}

tests/run/i14198.scala

@@ -0,0 +1,60 @@
+
+import java.lang.ref.WeakReference
+import java.util.concurrent.atomic.AtomicReference
+
+final class Mark
+
+object Test:
+
+  def main(args: Array[String]): Unit =
+    myTest
+    trying
+
+  final def myAssert(cond: => Boolean): Unit = assert(cond)
+
+  def terminally(cond: => Boolean): Unit =
+    System.gc()
+    var n = 10
+    while (n > 0 && !cond)
+    do
+      System.gc()
+      Thread.`yield`()
+      //print(".")
+      n -= 1
+    assert(cond)
+
+  def myTest: Unit =
+    val ref = new AtomicReference[WeakReference[AnyRef]]
+    var mark: AnyRef = null
+    assert(ref.compareAndSet(null, WeakReference(Mark())))
+    mark = ref.get().get()
+    myAssert(mark ne null) // in theory this could fail, but it isn't
+    mark = null
+    terminally(ref.get().get() == null)
+
+  def trying: Unit =
+    def ignore[A]: (Throwable => A) = _ => null.asInstanceOf[A]
+    var i: Int = 21
+    var s: String = "hello"
+    var r: WeakReference[String] = null
+    def f(n: => Int) = n + n + 1
+    def g(x: => String) =
+      r = WeakReference(x + "/" + x)
+      r.get()
+    i = try f(i) catch ignore
+    s = try g(s) catch ignore
+    assert(s == "hello/hello")
+    assert(r.get() == "hello/hello")
+    s = null
+    terminally(r.get() == null)
+    s = "bye"
+    s = try g(s) catch ignore
+    assert(s == "bye/bye")
+    assert(r.get() == "bye/bye")
+    s = null.asInstanceOf[String]
+    terminally(r.get() == null)
+    @volatile var z: String = "whoa"
+    z = try g(z) catch ignore
+    assert(r.get() == "whoa/whoa")
+    z = null
+    terminally(r.get() == null)

tests/run/liftedTry.scala

@@ -16,15 +16,34 @@ object Test {
     assert(x == 1)
     assert(foo(2) == 2)
     assert(foo(try raise(3) catch handle) == 3)
-    Tr.foo
+    assert(Tr.foo == 3)
   }
 }
 
 object Tr {
   def fun(a: Int => Unit) = a(2)
   def foo: Int = {
     var s = 1
-    s = try {fun(s = _); 3} catch{ case ex: Throwable => val x = 4; s = x; 5 }
+    s = try {fun(s = _); 3} catch { case ex: Throwable => val x = 4; s = x; 5 }
     s
   }
 }
+
+/* was:
+Caused by: java.lang.VerifyError: Inconsistent stackmap frames at branch target 33
+Exception Details:
+  Location:
+    Tr$.foo()I @30: goto
+  Reason:
+    Current frame's stack size doesn't match stackmap.
+  Current Frame:
+    bci: @30
+    flags: { }
+    locals: { 'Tr$', 'scala/runtime/IntRef', 'java/lang/Throwable', integer }
+    stack: { integer }
+  Stackmap Frame:
+    bci: @33
+    flags: { }
+    locals: { 'Tr$', 'scala/runtime/IntRef' }
+    stack: { top, integer }
+ */