Merge pull request #5509 from lrytz/t10032

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

3 files changed, 42 insertions, 20 deletions

diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala b/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala
index 0b07e12917..b0815b0008 100644
--- a/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala
+++ b/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala
@@ -488,16 +488,11 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder {
           bc emitRETURN returnType
         case nextCleanup :: rest =>
           if (saveReturnValue) {
-            if (insideCleanupBlock) {
-              reporter.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")
-              }
-              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")
             }
+            locals.store(earlyReturnVar)
           }
           bc goTo nextCleanup
           shouldEmitCleanup = true
diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BCodeSkelBuilder.scala b/src/compiler/scala/tools/nsc/backend/jvm/BCodeSkelBuilder.scala
index dbad37cd5b..fdb5687311 100644
--- a/src/compiler/scala/tools/nsc/backend/jvm/BCodeSkelBuilder.scala
+++ b/src/compiler/scala/tools/nsc/backend/jvm/BCodeSkelBuilder.scala
@@ -255,7 +255,6 @@ abstract class BCodeSkelBuilder extends BCodeHelpers {
     // used by genLoadTry() and genSynchronized()
     var earlyReturnVar: Symbol     = null
     var shouldEmitCleanup          = false
-    var insideCleanupBlock         = false
     // line numbers
     var lastEmittedLineNr          = -1
 
diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BCodeSyncAndTry.scala b/src/compiler/scala/tools/nsc/backend/jvm/BCodeSyncAndTry.scala
index 466793010f..add2c5ffe6 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,14 +347,11 @@ 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) {
-        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