Fix #101: Interaction between TryFinally and Labeled/Return.

[sjrd]

See the big "HERE BE DRAGONS" comment for details.

[sjrd]

Well, that was even harder than I thought ^^ But now it should handle all the possible weird situations.

[tanishiking]

Thank you very much @sjrd great work!
I left several comments and questions, but, overall, LGTM 🎉

[tanishiking]

Do I understand correctly that

• Scala3's boundary break is also compiled to Labeled/Return?
• We hit this case in the following case?

import scala.util.boundary, boundary.break

try:
 boundary: // Labeled(l)
    // ...
    break(x) // Return(l)
finally:
  // ...

[sjrd]

Scala 3's boundary/break is indeed compiled to Labeled/Return. However the optimization is explicitly not allowed if there is a try..finally in the middle of the boundary/break. In that case it falls back to actually throwing and catching an exception. That's because the JVM backend cannot deal with it. The JVM backend only handles return in that situation.

We have to deal with it because a return keyword in Scala source is compiled as a Labeled/Return as well. After inlining (once we enable the optimizer), we can truly get arbitrary combinations.

---

The case that would result in what we have here would be:

boundary:
  try
    break(x)
  finally
    ...

When both the boundary and break are inside the try, there is no issue because we're not crossing the try..finally.

[tanishiking]

[note]

Suggested change

	* body
	* body ;; `Return` in body will be compiled to `br $cross`

[tanishiking]

Suggested change

	// If the `exnref` is non-null, rethrow it, but stay within the `$done` block
	// If the `exnref` is non-null, rethrow it
	// Otherwise, stay within the `$done` block

You meant like this?

It's understandable to stay in $done if none of the uncaught exception is thrown (this is the case where (1) we hit Return (destinationTag = 0, fall-through) or (2) executed return without throwing exception / exception is caught by catch clauses.
We need to jump to the appropriate destination using br_table.

IIUC, throw_ref goes out from the $done block to find an enclosing try-catch-finally block.

[sjrd]

Oops, yes, good catch!

[tanishiking]

Suggested change

	* based on the value of the function's destinationTagLocal.
	* based on the value of the try..finally's destinationTagLocal, which is set by `Return` or 0 as fall-through.

Is this more accurate? I couldn't find what is the "function" in this context.

[tanishiking]

[note]
Set by genReturn or 0 for fall-through

[tanishiking]

[note]

alpha[int]: {
  beta[int]: {
    try {
      // set `destinationTag` of the try..finally to `alpha`,
      // store the result into locals,
      // and go-to finally block
      if (A) return@alpha 5
      else if (B) return@beta 10
      else //...
    } finally {
      doTheFinally()
      // Where we jump to after the `finally` block?
      // Jump to the appropriate position based on the
      // `destionalTag` of this try..finally block, set by return or 0 as fall-through
    }
    // fall-through (destinationTag = 0)
    someOtherThings(bar)
  }
  // destinationTag for `beta`
}
// destionalTag for `alpha`

[sjrd]

I elaborated with a complete example in the comments, with the description of how it gets compiled, based on your example here.

[tanishiking]

[note]

alpha[int]: {
  try {
    try {
      // ...
    } finally {
      // ...
      // There's outer try..finally block (inside `alpha` Labeled block)
      // In this case, we need to execute the outer `finally` before jump to `alpha`
    }
    // fall-through
  } finally {
    // ...
  }
}

[tanishiking]

[solved] 😅

Added test that the fall-through is executed before the outer finally block

diff --git a/test-suite/src/main/scala/testsuite/core/TryFinallyReturnTest.scala b/test-suite/src/main/scala/testsuite/core/TryFinallyReturnTest.scala
index 9a71700..2b3d15c 100644
--- a/test-suite/src/main/scala/testsuite/core/TryFinallyReturnTest.scala
+++ b/test-suite/src/main/scala/testsuite/core/TryFinallyReturnTest.scala
@@ -45,6 +45,12 @@ Running nestedFinallyBlocks
 in finally 1
 in finally 2
 ----------------------------------------
+Running nestedFinallyBlocks2
+in try 1
+in finally 1
+in fall-through
+in finally 2
+----------------------------------------
 """
 
   def main(): Unit = {
@@ -56,6 +62,7 @@ in finally 2
     test(retFinally(), "retFinally")
     test(throwFinally(), "throwFinally")
     test(nestedFinallyBlocks(), "nestedFinallyBlocks")
+    test(nestedFinallyBlocks2(), "nestedFinallyBlocks2")
 
     assertSame(expectedOutput, printlnOutput)
   }
@@ -162,6 +169,19 @@ in finally 2
       println("in finally 2")
     }
 
+  def nestedFinallyBlocks2(): Int =
+    try {
+      try {
+        println("in try 1")
+      } finally {
+        println("in finally 1")
+      }
+      println("in fall-through")
+      0
+    } finally {
+      println("in finally 2")
+    }
+
   def test[A](m: => A, name: String): Unit = {
     println("Running %s".format(name))
     try {

and this test passed. While I assumed that this test will fail because it seems that we anyway overwrite the destinationTag to the outer finally block instead of the fall-through.

How this test is passing?

[tanishiking]

Oops, never mind, possibleTargetEntries doesn't contain the fall-thgrough

[sjrd]

I added the test anyway. ;)

[sjrd]

I think I addressed all the comments.

[tanishiking]

LGTM! Thank you very much for elaborating on things