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.

1 files changed, 38 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)