From c1e9b0a951ee5298244c6456af3641ee966e101b Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Wed, 9 Nov 2016 14:10:59 +0100 Subject: 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. --- .../tools/nsc/backend/jvm/BCodeBodyBuilder.scala | 13 ++-- .../tools/nsc/backend/jvm/BCodeSkelBuilder.scala | 1 - .../tools/nsc/backend/jvm/BCodeSyncAndTry.scala | 3 - test/files/run/t10032.check | 39 +++++++++++- test/files/run/t10032.scala | 71 +++++++++++++++++++--- 5 files changed, 101 insertions(+), 26 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 9d4ef44546..add2c5ffe6 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/BCodeSyncAndTry.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/BCodeSyncAndTry.scala @@ -348,13 +348,10 @@ abstract class 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 diff --git a/test/files/run/t10032.check b/test/files/run/t10032.check index c8f0bdf034..565fe25848 100644 --- a/test/files/run/t10032.check +++ b/test/files/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 diff --git a/test/files/run/t10032.scala b/test/files/run/t10032.scala index df6b114d05..f7e8ef459f 100644 --- a/test/files/run/t10032.scala +++ b/test/files/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) } -- cgit v1.2.3