Fix returns from within finalizers

When a return in a finalizer was reached through a return within the try
block, the backend ignored the return in the finalizer:

    try {
      try { return 1 }
      finally { return 2 }
    } finally { println() }

This expression should evaluate to 2 (it does in 2.11.8), but in 2.12.0
it the result is 1.

The Scala spec is currently incomplete, it does not say that a finalizer
should be exectuted if a return occurs within a try block, and it does
not specify what happens if also the finally block has a return.

So we follow the Java spec, which basically says: if the finally blocks
completes abruptly for reason S, then the entire try statement completes
abruptly with reason S. An abrupt termination of the try block for a
different reason R is discarded.

Abrupt completion is basically returning or throwing.

Author: lrytz

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

@@ -341,13 +341,10 @@ trait BCodeSyncAndTry extends BCodeBodyBuilder {
       // `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 && currentFinallyBlockNeedsCleanup) {
-        val savedInsideCleanup = insideCleanupBlock
-        insideCleanupBlock = true
         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

@@ -1,6 +1,3 @@
-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
@@ -22,6 +19,9 @@ t3
 t4
  i1
  i2
+t4
+ e1
+ i2
 t5
  i1
  a1
@@ -35,6 +35,10 @@ t6
  i1
  i2
  i3
+t6
+ e1
+ i2
+ i3
 t7
  i1
  a1
@@ -47,3 +51,32 @@ t8
  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

@@ -24,7 +24,8 @@ object Test extends App {
       try { return i1 }
       finally { a1() }
     } finally {
-      try { a2() } finally { a3() }
+      try { a2() }
+      finally { a3() }
     }
   }
 
@@ -42,10 +43,10 @@ object Test extends App {
     }
   }
 
-  def t4: Int = {
+  def t4(i: => Int): Int = {
     println("t4")
     try {
-      return i1
+      return i
     } finally {
       return i2
     }
@@ -65,10 +66,10 @@ object Test extends App {
     }
   }
 
-  def t6: Int = {
+  def t6(i: => Int): Int = {
     println("t6")
     try {
-      try { return i1 }
+      try { return i }
       finally { return i2 }
     } finally {
       return i3
@@ -86,10 +87,10 @@ object Test extends App {
     }
   }
 
-  def t8(): Int = {
+  def t8(i: => Int): Int = {
     println("t8")
     try {
-      try { i1 }
+      try { i }
       finally {           // no cleanup version
         try { return i2 }
         finally { a1() }  // cleanup version required
@@ -99,15 +100,65 @@ object Test extends App {
     }
   }
 
+  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 == 2)
+
+  assert(t4(i1) == 2)
+  assert(t4(e1) == 2)
+
   assert(t5(i1) == 1)
   assert(t5(e1) == 2)
-  assert(t6 == 3)
+
+  assert(t6(i1) == 3)
+  assert(t6(e1) == 3)
+
   assert(t7(i1) == 1)
   assert(t7(e1) == 2)
-  assert(t8 == 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)
 }