SI-10032 Fix code gen with returns in nested try-finally blocks

Return statements within `try` or `catch` blocks need special treatement
if there's also a `finally`

    try { return 1 } finally { println() }

For the return, the code generator emits a store to a local and a jump
to a "cleanup" version of the finally block. There will be 3 versions
of the finally block:

  - One reached through a handler, if the code in the try block
    throws; re-throws at the end
  - A "cleanup" version reached from returns within the try; reads the
    local and returns the value at the end
  - One reached for ordinary control flow, if there's no return and no
    exception within the try

If there are multiple enclosing finally blocks, a "cleanup" version is
emitted for each of them. The nested ones jump to the enclosing ones,
the outermost one reads the local and returns.

A global variable `shouldEmitCleanup` stores whether cleanup versions
are required for the curren finally blocks. By mistake, this variable
was not reset to `false` when emitting a `try-finally` nested within a
`finally`:

    try {
      try { return 1 }
      finally { println() } // need cleanup version
    } finally {             // need cleanup version
      try { println() }
      finally { println() } // no cleanup version needed!
    }

In this commit we ensure that the variable is reset when emitting
nested `try-finally` blocks.

3 files changed, 200 insertions, 7 deletions

diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BCodeSyncAndTry.scala b/src/compiler/scala/tools/nsc/backend/jvm/BCodeSyncAndTry.scala
index 466793010f..9d4ef44546 100644
--- a/src/compiler/scala/tools/nsc/backend/jvm/BCodeSyncAndTry.scala
+++ b/src/compiler/scala/tools/nsc/backend/jvm/BCodeSyncAndTry.scala
@@ -36,7 +36,7 @@ abstract class 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") else null;
+      val monitorResult: Symbol = if (hasResult) locals.makeLocal(tpeTK(args.head), "monitorResult") else null
 
       /* ------ (1) pushing and entering the monitor, also keeping a reference to it in a local var. ------ */
       genLoadQualifier(fun)
@@ -215,7 +215,7 @@ abstract class 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") else null;
+      val tmp          = if (guardResult) locals.makeLocal(tpeTK(tree), "tmp") else null
 
       /*
        * upon early return from the try-body or one of its EHs (but not the EH-version of the finally-clause)
@@ -238,6 +238,34 @@ abstract class 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:
@@ -246,8 +274,7 @@ abstract class BCodeSyncAndTry extends BCodeBodyBuilder {
        * ------
        */
 
-      for (ch <- caseHandlers) {
-
+      for (ch <- caseHandlers) withFreshCleanupScope {
         // (2.a) emit case clause proper
         val startHandler = currProgramPoint()
         var endHandler: asm.Label = null
@@ -277,9 +304,13 @@ abstract class BCodeSyncAndTry extends BCodeBodyBuilder {
         protect(startTryBody, endTryBody, startHandler, excType)
         // (2.c) emit jump to the program point where the finally-clause-for-normal-exit starts, or in effect `after` if no finally-clause was given.
         bc goTo postHandlers
-
       }
 
+      // 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.
@@ -288,7 +319,7 @@ abstract class BCodeSyncAndTry extends BCodeBodyBuilder {
 
       // a note on terminology: this is not "postHandlers", despite appearances.
       // "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)
@@ -316,7 +347,7 @@ abstract class 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) {
+      if (hasFinally && currentFinallyBlockNeedsCleanup) {
         val savedInsideCleanup = insideCleanupBlock
         insideCleanupBlock = true
         markProgramPoint(finCleanup)
diff --git a/test/files/run/t10032.check b/test/files/run/t10032.check
new file mode 100644
index 0000000000..c8f0bdf034
--- /dev/null
+++ b/test/files/run/t10032.check
@@ -0,0 +1,49 @@
+t10032.scala:72: warning: Return statement found in finally-clause, discarding its return-value in favor of that of a more deeply nested return.
+      finally { return i2 }
+                ^
+t1
+ i1
+ a1
+t2
+ i1
+ a1
+ a2
+ a3
+t3
+ i1
+ a1
+ a3
+t3
+ e1
+ a1
+ i2
+ a2
+ a3
+t4
+ i1
+ i2
+t5
+ i1
+ a1
+ a3
+t5
+ e1
+ a1
+ i2
+ a3
+t6
+ i1
+ i2
+ i3
+t7
+ i1
+ a1
+t7
+ e1
+ i2
+ a1
+t8
+ i1
+ i2
+ a1
+ a2
diff --git a/test/files/run/t10032.scala b/test/files/run/t10032.scala
new file mode 100644
index 0000000000..df6b114d05
--- /dev/null
+++ b/test/files/run/t10032.scala
@@ -0,0 +1,113 @@
+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: Int = {
+    println("t4")
+    try {
+      return i1
+    } 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: Int = {
+    println("t6")
+    try {
+      try { return i1 }
+      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(): Int = {
+    println("t8")
+    try {
+      try { i1 }
+      finally {           // no cleanup version
+        try { return i2 }
+        finally { a1() }  // cleanup version required
+      }
+    } finally {           // cleanup version required
+      a2()
+    }
+  }
+
+  assert(t1 == 1)
+  assert(t2 == 1)
+  assert(t3(i1) == 1)
+  assert(t3(e1) == 2)
+  assert(t4 == 2)
+  assert(t5(i1) == 1)
+  assert(t5(e1) == 2)
+  assert(t6 == 3)
+  assert(t7(i1) == 1)
+  assert(t7(e1) == 2)
+  assert(t8 == 2)
+}