From 185b5013fe2154c2c8f9a0f2a2f044b49d14cfb9 Mon Sep 17 00:00:00 2001
From: Mikhael Bogdanov <mikhael.bogdanov@jetbrains.com>
Date: Fri, 16 Aug 2013 13:03:14 +0400
Subject: [PATCH] Properly process nested try and loop blocks KT-3894: Loops
 and finally: finally block executed twice when break and return exists in
 try/finally block KT-3874: Compilation error on try catch block contains
 break and continue KT-3869: Loops and finally: outer finally block not run

 #KT-3869 Fixed
 #KT-3894 Fixed
 #KT-3874 Fixed
---
 .../jet/codegen/ExpressionCodegen.java        | 74 +++++++++++--------
 .../codegen/box/finally/finallyAndFinally.kt  | 39 ++++++++++
 .../testData/codegen/box/finally/kt3874.kt    | 35 +++++++++
 .../testData/codegen/box/finally/kt3894.kt    | 36 +++++++++
 .../codegen/box/finally/loopAndFinally.kt     | 69 +++++++++++++++++
 .../codegen/box/finally/tryLoopTry.kt         | 36 +++++++++
 .../BlackBoxCodegenTestGenerated.java         | 25 +++++++
 7 files changed, 284 insertions(+), 30 deletions(-)
 create mode 100644 compiler/testData/codegen/box/finally/finallyAndFinally.kt
 create mode 100644 compiler/testData/codegen/box/finally/kt3874.kt
 create mode 100644 compiler/testData/codegen/box/finally/kt3894.kt
 create mode 100644 compiler/testData/codegen/box/finally/loopAndFinally.kt
 create mode 100644 compiler/testData/codegen/box/finally/tryLoopTry.kt

diff --git a/compiler/backend/src/org/jetbrains/jet/codegen/ExpressionCodegen.java b/compiler/backend/src/org/jetbrains/jet/codegen/ExpressionCodegen.java
index f7c0fc961c8..e7e5969e585 100644
--- a/compiler/backend/src/org/jetbrains/jet/codegen/ExpressionCodegen.java
+++ b/compiler/backend/src/org/jetbrains/jet/codegen/ExpressionCodegen.java
@@ -1156,17 +1156,17 @@ public class ExpressionCodegen extends JetVisitor<StackValue, StackValue> implem
     private StackValue visitBreakOrContinueExpression(@NotNull JetLabelQualifiedExpression expression, StackValue receiver, boolean isBreak) {
         assert expression instanceof JetContinueExpression || expression instanceof JetBreakExpression;
 
-        JetSimpleNameExpression labelElement = expression.getTargetLabel();
+        if (!blockStackElements.isEmpty()) {
+            BlockStackElement stackElement = blockStackElements.peek();
 
-        for (int i = blockStackElements.size() - 1; i >= 0; --i) {
-            BlockStackElement stackElement = blockStackElements.get(i);
             if (stackElement instanceof FinallyBlockStackElement) {
                 FinallyBlockStackElement finallyBlockStackElement = (FinallyBlockStackElement) stackElement;
                 //noinspection ConstantConditions
-                genFinallyBlockOrGoto(finallyBlockStackElement, false, null);
+                genFinallyBlockOrGoto(finallyBlockStackElement, null);
             }
             else if (stackElement instanceof LoopBlockStackElement) {
                 LoopBlockStackElement loopBlockStackElement = (LoopBlockStackElement) stackElement;
+                JetSimpleNameExpression labelElement = expression.getTargetLabel();
                 //noinspection ConstantConditions
                 if (labelElement == null ||
                     loopBlockStackElement.targetLabel != null &&
@@ -1176,11 +1176,18 @@ public class ExpressionCodegen extends JetVisitor<StackValue, StackValue> implem
                 }
             }
             else {
-                throw new UnsupportedOperationException();
+                throw new UnsupportedOperationException("Wrong BlockStackElement in processing stack");
             }
+
+            blockStackElements.pop();
+            StackValue result = visitBreakOrContinueExpression(expression, receiver, isBreak);
+            blockStackElements.push(stackElement);
+            return result;
+
+
         }
 
-        throw new UnsupportedOperationException();
+        throw new UnsupportedOperationException("Target label for break/continue not found");
     }
 
     private StackValue generateSingleBranchIf(
@@ -1510,32 +1517,36 @@ public class ExpressionCodegen extends JetVisitor<StackValue, StackValue> implem
     }
 
     private void doFinallyOnReturn() {
-        for (int i = blockStackElements.size() - 1; i >= 0; --i) {
-            BlockStackElement stackElement = blockStackElements.get(i);
+        if(!blockStackElements.isEmpty()) {
+            BlockStackElement stackElement = blockStackElements.peek();
             if (stackElement instanceof FinallyBlockStackElement) {
                 FinallyBlockStackElement finallyBlockStackElement = (FinallyBlockStackElement) stackElement;
-                genFinallyBlockOrGoto(finallyBlockStackElement, true, null);
+                genFinallyBlockOrGoto(finallyBlockStackElement, null);
             }
             else if (stackElement instanceof LoopBlockStackElement) {
-                break;
+
             } else {
-                throw new UnsupportedOperationException();
+                throw new UnsupportedOperationException("Wrong BlockStackElement in processing stack");
             }
+
+            blockStackElements.pop();
+            doFinallyOnReturn();
+            blockStackElements.push(stackElement);
         }
     }
 
     private void genFinallyBlockOrGoto(
             @Nullable FinallyBlockStackElement finallyBlockStackElement,
-            boolean doPop,
             @Nullable Label tryCatchBlockEnd
     ) {
-        if (finallyBlockStackElement != null) {
-            assert finallyBlockStackElement.gaps.size() % 2 == 0;
-            JetTryExpression jetTryExpression = finallyBlockStackElement.expression;
-            if (doPop) {
-                blockStackElements.pop();
-            }
 
+        if (finallyBlockStackElement != null) {
+            assert finallyBlockStackElement.gaps.size() % 2 == 0 : "Finally block gaps are inconsistent";
+
+            BlockStackElement topOfStack = blockStackElements.pop();
+            assert topOfStack == finallyBlockStackElement : "Top element of stack doesn't equals processing finally block";
+
+            JetTryExpression jetTryExpression = finallyBlockStackElement.expression;
             Label finallyStart = new Label();
             v.mark(finallyStart);
             finallyBlockStackElement.addGapLabel(finallyStart);
@@ -1553,9 +1564,7 @@ public class ExpressionCodegen extends JetVisitor<StackValue, StackValue> implem
             v.mark(finallyEnd);
             finallyBlockStackElement.addGapLabel(finallyEnd);
 
-            if (doPop) {
-                blockStackElements.push(finallyBlockStackElement);
-            }
+            blockStackElements.push(finallyBlockStackElement);
         }
     }
 
@@ -3473,11 +3482,11 @@ The "returned" value of try expression with no finally is either the last expres
         v.mark(tryEnd);
 
         //do it before finally block generation
-        List<Label> tryBlockRegions = getCatchIntervals(finallyBlockStackElement, tryStart, tryEnd);
+        List<Label> tryBlockRegions = getCurrentCatchIntervals(finallyBlockStackElement, tryStart, tryEnd);
 
         Label end = new Label();
 
-        genFinallyBlockOrGoto(finallyBlockStackElement, true, end);
+        genFinallyBlockOrGoto(finallyBlockStackElement, end);
 
         List<JetCatchClause> clauses = expression.getCatchClauses();
         for (int i = 0, size = clauses.size(); i < size; i++) {
@@ -3501,7 +3510,7 @@ The "returned" value of try expression with no finally is either the last expres
 
             myFrameMap.leave(descriptor);
 
-            genFinallyBlockOrGoto(finallyBlockStackElement, true, i != size - 1 || finallyBlock != null ? end : null);
+            genFinallyBlockOrGoto(finallyBlockStackElement, i != size - 1 || finallyBlock != null ? end : null);
 
             generateExceptionTable(clauseStart, tryBlockRegions, descriptorType.getInternalName());
         }
@@ -3517,11 +3526,11 @@ The "returned" value of try expression with no finally is either the last expres
             v.mark(defaultCatchEnd);
 
             //do it before finally block generation
-            //javac generates such info for default catch clause too!!!! so defaultCatchEnd as end parameter
-            List<Label> defaultCatchRegions = getCatchIntervals(finallyBlockStackElement, tryStart, defaultCatchEnd);
+            //javac also generates entry in exception table for default catch clause too!!!! so defaultCatchEnd as end parameter
+            List<Label> defaultCatchRegions = getCurrentCatchIntervals(finallyBlockStackElement, tryStart, defaultCatchEnd);
 
 
-            genFinallyBlockOrGoto(finallyBlockStackElement, true, null);
+            genFinallyBlockOrGoto(finallyBlockStackElement, null);
 
             v.load(savedException, JAVA_THROWABLE_TYPE);
             myFrameMap.leaveTemp(JAVA_THROWABLE_TYPE);
@@ -3546,7 +3555,7 @@ The "returned" value of try expression with no finally is either the last expres
         return StackValue.onStack(expectedAsmType);
     }
 
-    private void generateExceptionTable(Label catchStart, List<Label> catchedRegions, String exception) {
+    private void generateExceptionTable(@NotNull Label catchStart, @NotNull List<Label> catchedRegions, @Nullable String exception) {
         for (int i = 0; i < catchedRegions.size(); i += 2) {
             Label startRegion = catchedRegions.get(i);
             Label endRegion = catchedRegions.get(i+1);
@@ -3555,8 +3564,13 @@ The "returned" value of try expression with no finally is either the last expres
     }
 
 
-    private List<Label> getCatchIntervals(FinallyBlockStackElement finallyBlockStackElement, Label blockStart, Label blockEnd) {
-        List<Label> gapsInBlock = finallyBlockStackElement != null ? new ArrayList<Label>(finallyBlockStackElement.gaps) : Collections.<Label>emptyList();
+    private List<Label> getCurrentCatchIntervals(
+            @Nullable FinallyBlockStackElement finallyBlockStackElement,
+            @NotNull Label blockStart,
+            @NotNull Label blockEnd
+    ) {
+        List<Label> gapsInBlock =
+                finallyBlockStackElement != null ? new ArrayList<Label>(finallyBlockStackElement.gaps) : Collections.<Label>emptyList();
         assert gapsInBlock.size() % 2 == 0;
         List<Label> blockRegions = new ArrayList<Label>(gapsInBlock.size() + 2);
         blockRegions.add(blockStart);
diff --git a/compiler/testData/codegen/box/finally/finallyAndFinally.kt b/compiler/testData/codegen/box/finally/finallyAndFinally.kt
new file mode 100644
index 00000000000..d264bad1724
--- /dev/null
+++ b/compiler/testData/codegen/box/finally/finallyAndFinally.kt
@@ -0,0 +1,39 @@
+class MyString {
+    var s = ""
+    fun plus(x : String) : MyString {
+        s += x
+        return this
+    }
+
+    fun toString(): String {
+        return s
+    }
+}
+
+
+fun test1() : MyString {
+    var r = MyString()
+    try {
+        r + "Try1"
+
+        try {
+            r + "Try2"
+            if (true)
+                return r
+        } finally {
+            r + "Finally2"
+            if (true) {
+                return r
+            }
+        }
+
+        return r
+    } finally {
+        r + "Finally1"
+    }
+}
+
+
+fun box(): String {
+  return if (test1().toString() == "Try1Try2Finally2Finally1") "OK" else "fail: ${test1()}"
+}
\ No newline at end of file
diff --git a/compiler/testData/codegen/box/finally/kt3874.kt b/compiler/testData/codegen/box/finally/kt3874.kt
new file mode 100644
index 00000000000..fa0a4259d12
--- /dev/null
+++ b/compiler/testData/codegen/box/finally/kt3874.kt
@@ -0,0 +1,35 @@
+fun test1(): String {
+    var r = ""
+    for (i in 1..2)  {
+        try {
+            r += "O"
+            continue
+        } finally {
+            r += "K"
+            break
+        }
+    }
+    return r
+}
+
+fun test2(): String {
+    var r = ""
+    for (i in 1..2)  {
+        try {
+            r += "O"
+            break
+        } finally {
+            r += "K"
+            continue
+        }
+    }
+    return r
+}
+
+fun box(): String {
+  if (test1() != "OK") return "fail1: ${test1()}"
+
+  if (test2() != "OKOK") return "fail2: ${test2()}"
+
+  return "OK"
+}
\ No newline at end of file
diff --git a/compiler/testData/codegen/box/finally/kt3894.kt b/compiler/testData/codegen/box/finally/kt3894.kt
new file mode 100644
index 00000000000..5e194ac12ee
--- /dev/null
+++ b/compiler/testData/codegen/box/finally/kt3894.kt
@@ -0,0 +1,36 @@
+class MyString {
+    var s = ""
+    fun plus(x : String) : MyString {
+        s += x
+        return this
+    }
+
+    fun toString(): String {
+        return s
+    }
+}
+
+fun test1() : MyString {
+    var r = MyString()
+    while (true) {
+      try {
+          r + "Try"
+
+          if (true) {
+              r + "Break"
+              break
+          }
+
+      } finally {
+          return r + "Finally"
+      }
+    }
+}
+
+
+
+fun box(): String {
+    if (test1().toString() != "TryBreakFinally") return "fail1: ${test1().toString()}"
+
+    return "OK"
+}
\ No newline at end of file
diff --git a/compiler/testData/codegen/box/finally/loopAndFinally.kt b/compiler/testData/codegen/box/finally/loopAndFinally.kt
new file mode 100644
index 00000000000..57c3985027c
--- /dev/null
+++ b/compiler/testData/codegen/box/finally/loopAndFinally.kt
@@ -0,0 +1,69 @@
+//KT-3869 Loops and finally: outer finally block not run
+
+class MyString {
+    var s = ""
+    fun plus(x : String) : MyString {
+        s += x
+        return this
+    }
+
+    fun toString(): String {
+        return s
+    }
+}
+
+fun test1() : MyString {
+    var r = MyString()
+    try {
+        r + "Try"
+
+        while(r.toString() != "") {
+            return r + "Loop"
+        }
+
+        return r + "Fail"
+    } finally {
+        r + "Finally"
+    }
+}
+
+fun test2() : MyString {
+    var r = MyString()
+    try {
+        r + "Try"
+
+        do {
+            if (r.toString() != "") {
+                return r + "Loop"
+            }
+        } while (r.toString() != "")
+
+        return r + "Fail"
+    } finally {
+        r + "Finally"
+    }
+}
+
+fun test3() : MyString {
+    var r = MyString()
+    try {
+        r + "Try"
+
+        for(i in 1..2) {
+            r + "Loop"
+            return r
+        }
+
+        return r + "Fail"
+    } finally {
+        r + "Finally"
+    }
+}
+
+fun box(): String {
+    if (test1().toString() != "TryLoopFinally") return "fail1: ${test1()}"
+    if (test2().toString() != "TryLoopFinally") return "fail2: ${test2()}"
+    if (test3().toString() != "TryLoopFinally") return "fail3: ${test3()}"
+
+    return "OK"
+}
\ No newline at end of file
diff --git a/compiler/testData/codegen/box/finally/tryLoopTry.kt b/compiler/testData/codegen/box/finally/tryLoopTry.kt
new file mode 100644
index 00000000000..ae417be7136
--- /dev/null
+++ b/compiler/testData/codegen/box/finally/tryLoopTry.kt
@@ -0,0 +1,36 @@
+//test for appropriate
+
+class MyString {
+    var s = ""
+    fun plus(x : String) : MyString {
+        s += x
+        return this
+    }
+
+    fun toString(): String {
+        return s
+    }
+}
+
+
+fun test1() : MyString {
+    var r = MyString()
+    try {
+        r + "Try1"
+        for(i in 1..1) {
+            try {
+                r + "Try2"
+            } finally {
+                return r + "Finally2"
+            }
+        }
+
+    } finally {
+        r + "Finally1"
+    }
+    return r  + "Fail"
+}
+
+fun box(): String {
+  return if (test1().toString() == "Try1Try2Finally2Finally1") "OK" else "fail: ${test1()}"
+}
\ No newline at end of file
diff --git a/compiler/tests/org/jetbrains/jet/codegen/generated/BlackBoxCodegenTestGenerated.java b/compiler/tests/org/jetbrains/jet/codegen/generated/BlackBoxCodegenTestGenerated.java
index 7f27a106b88..5aeae6d89a8 100644
--- a/compiler/tests/org/jetbrains/jet/codegen/generated/BlackBoxCodegenTestGenerated.java
+++ b/compiler/tests/org/jetbrains/jet/codegen/generated/BlackBoxCodegenTestGenerated.java
@@ -2102,6 +2102,11 @@ public class BlackBoxCodegenTestGenerated extends AbstractBlackBoxCodegenTest {
             JetTestUtils.assertAllTestsPresentByMetadata(this.getClass(), "org.jetbrains.jet.generators.tests.GenerateTests", new File("compiler/testData/codegen/box/finally"), Pattern.compile("^(.+)\\.kt$"), true);
         }
         
+        @TestMetadata("finallyAndFinally.kt")
+        public void testFinallyAndFinally() throws Exception {
+            doTest("compiler/testData/codegen/box/finally/finallyAndFinally.kt");
+        }
+        
         @TestMetadata("kt3549.kt")
         public void testKt3549() throws Exception {
             doTest("compiler/testData/codegen/box/finally/kt3549.kt");
@@ -2112,6 +2117,21 @@ public class BlackBoxCodegenTestGenerated extends AbstractBlackBoxCodegenTest {
             doTest("compiler/testData/codegen/box/finally/kt3867.kt");
         }
         
+        @TestMetadata("kt3874.kt")
+        public void testKt3874() throws Exception {
+            doTest("compiler/testData/codegen/box/finally/kt3874.kt");
+        }
+        
+        @TestMetadata("kt3894.kt")
+        public void testKt3894() throws Exception {
+            doTest("compiler/testData/codegen/box/finally/kt3894.kt");
+        }
+        
+        @TestMetadata("loopAndFinally.kt")
+        public void testLoopAndFinally() throws Exception {
+            doTest("compiler/testData/codegen/box/finally/loopAndFinally.kt");
+        }
+        
         @TestMetadata("notChainCatch.kt")
         public void testNotChainCatch() throws Exception {
             doTest("compiler/testData/codegen/box/finally/notChainCatch.kt");
@@ -2122,6 +2142,11 @@ public class BlackBoxCodegenTestGenerated extends AbstractBlackBoxCodegenTest {
             doTest("compiler/testData/codegen/box/finally/tryFinally.kt");
         }
         
+        @TestMetadata("tryLoopTry.kt")
+        public void testTryLoopTry() throws Exception {
+            doTest("compiler/testData/codegen/box/finally/tryLoopTry.kt");
+        }
+        
     }
     
     @TestMetadata("compiler/testData/codegen/box/functions")