Merge pull request #5910 from dotty-staging/fix/return-in-try

Fix code gen with returns in nested try-finally blocks

Author: nicolasstucki

compiler/src/dotty/tools/backend/jvm/BCodeBodyBuilder.scala

@@ -563,16 +563,11 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder {
             bc emitRETURN returnType
           case nextCleanup :: rest =>
             if (saveReturnValue) {
-              if (insideCleanupBlock) {
-                int.warning(r.pos, "Return statement found in finally-clause, discarding its return-value in favor of that of a more deeply nested return.")
-                bc drop returnType
-              } else {
-                // regarding return value, the protocol is: in place of a `return-stmt`, a sequence of `adapt, store, jump` are inserted.
-                if (earlyReturnVar == null) {
-                  earlyReturnVar = locals.makeLocal(returnType, "earlyReturnVar", expr.tpe, expr.pos)
-                }
-                locals.store(earlyReturnVar)
+              // regarding return value, the protocol is: in place of a `return-stmt`, a sequence of `adapt, store, jump` are inserted.
+              if (earlyReturnVar == null) {
+                earlyReturnVar = locals.makeLocal(returnType, "earlyReturnVar", expr.tpe, expr.pos)
               }
+              locals.store(earlyReturnVar)
             }
             bc goTo nextCleanup
             shouldEmitCleanup = true

compiler/src/dotty/tools/backend/jvm/BCodeSkelBuilder.scala

@@ -255,7 +255,6 @@ trait BCodeSkelBuilder extends BCodeHelpers {
     // used by genLoadTry() and genSynchronized()
     var earlyReturnVar: Symbol     = null
     var shouldEmitCleanup          = false
-    var insideCleanupBlock         = false
     // line numbers
     var lastEmittedLineNr          = -1
 

compiler/src/dotty/tools/backend/jvm/BCodeSyncAndTry.scala

@@ -28,7 +28,7 @@ trait BCodeSyncAndTry extends BCodeBodyBuilder {
       // if the synchronized block returns a result, store it in a local variable.
       // Just leaving it on the stack is not valid in MSIL (stack is cleaned when leaving try-blocks).
       val hasResult = (expectedType != UNIT)
-      val monitorResult: Symbol = if (hasResult) locals.makeLocal(tpeTK(args.head), "monitorResult", Object_Type, tree.pos) else null;
+      val monitorResult: Symbol = if (hasResult) locals.makeLocal(tpeTK(args.head), "monitorResult", Object_Type, tree.pos) else null
 
       /* ------ (1) pushing and entering the monitor, also keeping a reference to it in a local var. ------ */
       genLoadQualifier(fun)
@@ -206,7 +206,7 @@ trait BCodeSyncAndTry extends BCodeBodyBuilder {
        * please notice `tmp` has type tree.tpe, while `earlyReturnVar` has the method return type.
        * Because those two types can be different, dedicated vars are needed.
        */
-      val tmp          = if (guardResult) locals.makeLocal(tpeTK(tree), "tmp", tree.tpe, tree.pos) else null;
+      val tmp          = if (guardResult) locals.makeLocal(tpeTK(tree), "tmp", tree.tpe, tree.pos) else null
 
       /*
        * upon early return from the try-body or one of its EHs (but not the EH-version of the finally-clause)
@@ -229,6 +229,34 @@ trait BCodeSyncAndTry extends BCodeBodyBuilder {
       val endTryBody = currProgramPoint()
       bc goTo postHandlers
 
+      /**
+       * A return within a `try` or `catch` block where a `finally` is present ("early return")
+       * emits a store of the result to a local, jump to a "cleanup" version of the `finally` block,
+       * and sets `shouldEmitCleanup = true` (see [[PlainBodyBuilder.genReturn]]).
+       *
+       * If the try-catch is nested, outer `finally` blocks need to be emitted in a cleanup version
+       * as well, so the `shouldEmitCleanup` variable remains `true` until the outermost `finally`.
+       * Nested cleanup `finally` blocks jump to the next enclosing one. For the outermost, we emit
+       * a read of the local variable, a return, and we set `shouldEmitCleanup = false` (see
+       * [[pendingCleanups]]).
+       *
+       * Now, assume we have
+       *
+       *     try { return 1 } finally {
+       *       try { println() } finally { println() }
+       *     }
+       *
+       * Here, the outer `finally` needs a cleanup version, but the inner one does not. The method
+       * here makes sure that `shouldEmitCleanup` is only propagated outwards, not inwards to
+       * nested `finally` blocks.
+       */
+      def withFreshCleanupScope(body: => Unit) = {
+        val savedShouldEmitCleanup = shouldEmitCleanup
+        shouldEmitCleanup = false
+        body
+        shouldEmitCleanup = savedShouldEmitCleanup || shouldEmitCleanup
+      }
+
       /* ------ (2) One EH for each case-clause (this does not include the EH-version of the finally-clause)
        *            An EH in (2) is reached upon abrupt termination of (1).
        *            An EH in (2) is protected by:
@@ -237,7 +265,7 @@ trait BCodeSyncAndTry extends BCodeBodyBuilder {
        * ------
        */
 
-      for (ch <- caseHandlers) {
+      for (ch <- caseHandlers) withFreshCleanupScope {
 
         // (2.a) emit case clause proper
         val startHandler = currProgramPoint()
@@ -271,6 +299,11 @@ trait BCodeSyncAndTry extends BCodeBodyBuilder {
 
       }
 
+      // Need to save the state of `shouldEmitCleanup` at this point: while emitting the first
+      // version of the `finally` block below, the variable may become true. But this does not mean
+      // that we need a cleanup version for the current block, only for the enclosing ones.
+      val currentFinallyBlockNeedsCleanup = shouldEmitCleanup
+
       /* ------ (3.A) The exception-handler-version of the finally-clause.
        *              Reached upon abrupt termination of (1) or one of the EHs in (2).
        *              Protected only by whatever protects the whole try-catch-finally expression.
@@ -279,7 +312,7 @@ trait BCodeSyncAndTry extends BCodeBodyBuilder {
 
       // a note on terminology: this is not "postHandlers", despite appearences.
       // "postHandlers" as in the source-code view. And from that perspective, both (3.A) and (3.B) are invisible implementation artifacts.
-      if (hasFinally) {
+      if (hasFinally) withFreshCleanupScope {
         nopIfNeeded(startTryBody)
         val finalHandler = currProgramPoint() // version of the finally-clause reached via unhandled exception.
         protect(startTryBody, finalHandler, finalHandler, null)
@@ -307,14 +340,11 @@ trait BCodeSyncAndTry extends BCodeBodyBuilder {
       // this is not "postHandlers" either.
       // `shouldEmitCleanup` can be set, and at the same time this try expression may lack a finally-clause.
       // In other words, all combinations of (hasFinally, shouldEmitCleanup) are valid.
-      if (hasFinally && shouldEmitCleanup) {
-        val savedInsideCleanup = insideCleanupBlock
-        insideCleanupBlock = true
+      if (hasFinally && currentFinallyBlockNeedsCleanup) {
         markProgramPoint(finCleanup)
         // regarding return value, the protocol is: in place of a `return-stmt`, a sequence of `adapt, store, jump` are inserted.
         emitFinalizer(finalizer, null, isDuplicate = true)
         pendingCleanups()
-        insideCleanupBlock = savedInsideCleanup
       }
 
       /* ------ (4) finally-clause-for-normal-nonEarlyReturn-exit

tests/run/t10032.check

@@ -0,0 +1,82 @@
+t1
+ i1
+ a1
+t2
+ i1
+ a1
+ a2
+ a3
+t3
+ i1
+ a1
+ a3
+t3
+ e1
+ a1
+ i2
+ a2
+ a3
+t4
+ i1
+ i2
+t4
+ e1
+ i2
+t5
+ i1
+ a1
+ a3
+t5
+ e1
+ a1
+ i2
+ a3
+t6
+ i1
+ i2
+ i3
+t6
+ e1
+ i2
+ i3
+t7
+ i1
+ a1
+t7
+ e1
+ i2
+ a1
+t8
+ i1
+ i2
+ a1
+ a2
+t8
+ e1
+ i2
+ a1
+ a2
+t9
+ i1
+ i2
+ a1
+t9
+ e1
+ i2
+ a1
+t10
+ i1
+ i2
+ i3
+t10
+ e1
+ i2
+ i3
+t11
+ i1
+ i2
+ a1
+t11
+ e1
+ i2
+ a1

tests/run/t10032.scala

@@ -0,0 +1,164 @@
+object Test extends App {
+  def a1(): Unit = println(" a1")
+  def a2(): Unit = println(" a2")
+  def a3(): Unit = println(" a3")
+
+  def i1: Int = { println(" i1"); 1 }
+  def i2: Int = { println(" i2"); 2 }
+  def i3: Int = { println(" i3"); 3 }
+
+  def e1: Int = { println(" e1"); throw new Exception() }
+
+  def t1: Int = {
+    println("t1")
+    try {
+      synchronized { return i1 }
+    } finally {
+      synchronized { a1() }
+    }
+  }
+
+  def t2: Int = {
+    println("t2")
+    try {
+      try { return i1 }
+      finally { a1() }
+    } finally {
+      try { a2() }
+      finally { a3() }
+    }
+  }
+
+  def t3(i: => Int): Int = {
+    println("t3")
+    try {
+      try { return i }
+      finally { a1() }
+    } catch {
+      case _: Throwable =>
+        try { i2 }
+        finally { a2() } // no cleanup version
+    } finally {
+      a3()
+    }
+  }
+
+  def t4(i: => Int): Int = {
+    println("t4")
+    try {
+      return i
+    } finally {
+      return i2
+    }
+  }
+
+  def t5(i: => Int): Int = {
+    println("t5")
+    try {
+      try {
+        try { return i }
+        finally { a1() }
+      } catch {
+        case _: Throwable => i2
+      }
+    } finally {
+      a3()
+    }
+  }
+
+  def t6(i: => Int): Int = {
+    println("t6")
+    try {
+      try { return i }
+      finally { return i2 }
+    } finally {
+      return i3
+    }
+  }
+
+  def t7(i: => Int): Int = {
+    println("t7")
+    try { i }
+    catch {
+      case _: Throwable =>
+        return i2
+    } finally {
+      a1() // cleanup required, early return in handler
+    }
+  }
+
+  def t8(i: => Int): Int = {
+    println("t8")
+    try {
+      try { i }
+      finally {           // no cleanup version
+        try { return i2 }
+        finally { a1() }  // cleanup version required
+      }
+    } finally {           // cleanup version required
+      a2()
+    }
+  }
+
+  def t9(i: => Int): Int = {
+    println("t9")
+    try {
+      return i
+    } finally {
+      try { return i2 }
+      finally { a1() }
+    }
+  }
+
+  def t10(i: => Int): Int = {
+    println("t10")
+    try {
+      return i
+    } finally {
+      try { return i2 }
+      finally { return i3 }
+    }
+  }
+
+  // this changed semantics between 2.12.0 and 2.12.1, see https://github.com/scala/scala/pull/5509#issuecomment-259291609
+  def t11(i: => Int): Int = {
+    println("t11")
+    try {
+      try { return i }
+      finally { return i2 }
+    } finally {
+      a1()
+    }
+  }
+
+  assert(t1 == 1)
+
+  assert(t2 == 1)
+
+  assert(t3(i1) == 1)
+  assert(t3(e1) == 2)
+
+  assert(t4(i1) == 2)
+  assert(t4(e1) == 2)
+
+  assert(t5(i1) == 1)
+  assert(t5(e1) == 2)
+
+  assert(t6(i1) == 3)
+  assert(t6(e1) == 3)
+
+  assert(t7(i1) == 1)
+  assert(t7(e1) == 2)
+
+  assert(t8(i1) == 2)
+  assert(t8(e1) == 2)
+
+  assert(t9(i1) == 2)
+  assert(t9(e1) == 2)
+
+  assert(t10(i1) == 3)
+  assert(t10(e1) == 3)
+
+  assert(t11(i1) == 2)
+  assert(t11(e1) == 2)
+}