Skip to content

Commit bb71ba5

Browse files
concavelenzcopybara-github
authored andcommitted
Fix incorrect code generation for try...finally with break in a generator.
When a `break` statement appeared inside a `finally` block, the transpiler would generate an extra `break` statement, which would cause the `leaveFinallyBlock` call to be unreachable. This change fixes the issue by introducing an `inFinallyBlock` flag to the `TranspilationContext`. This flag is used in `replaceBreakContinueWithJump` to prevent the extra `break` from being generated when inside a `finally` block. PiperOrigin-RevId: 795630876
1 parent c33db38 commit bb71ba5

File tree

2 files changed

+101
-9
lines changed

2 files changed

+101
-9
lines changed

src/com/google/javascript/jscomp/Es6RewriteGenerators.java

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1139,8 +1139,12 @@ private class TranspilationContext {
11391139
*/
11401140
Case currentCase;
11411141

1142+
// A counter for the number of finally blocks we are currently inside.
1143+
// This value is used for two purposes:
1144+
// 1. At COMPILE-TIME, to determine if a break/continue is inside a finally block.
1145+
// 2. At RUNTIME, its value is emitted into the generated code to manage the
1146+
// exception-handling stack.
11421147
int nestedFinallyBlockCount = 0;
1143-
11441148
boolean thisReferenceFound;
11451149
boolean argumentsReferenceFound;
11461150

@@ -1514,7 +1518,11 @@ Node createJumpToBlock(Case section, boolean allowEmbedding, Node sourceNode) {
15141518
/** Converts "break" and "continue" statements into state machine jumps. */
15151519
void replaceBreakContinueWithJump(Node sourceNode, Case section, int breakSuppressors) {
15161520
final String jumpMethod;
1517-
if (finallyCases.isEmpty() || finallyCases.getFirst().id < section.id) {
1521+
if (nestedFinallyBlockCount > 0) {
1522+
// If we are in a finally block, we need to use jumpThroughFinallyBlocks to ensure that
1523+
// the finally block is correctly exited.
1524+
jumpMethod = "jumpThroughFinallyBlocks";
1525+
} else if (finallyCases.isEmpty() || finallyCases.getFirst().id < section.id) {
15181526
// There are no finally blocks that should be exectuted pior to jumping
15191527
jumpMethod = "jumpTo";
15201528
} else {
@@ -1530,7 +1538,14 @@ void replaceBreakContinueWithJump(Node sourceNode, Case section, int breakSuppre
15301538
type(StandardColors.NULL_OR_VOID),
15311539
section.getNumber(sourceNode))
15321540
.insertBefore(sourceNode);
1533-
sourceNode.replaceWith(createBreakNodeFor(sourceNode));
1541+
if (nestedFinallyBlockCount == 0) {
1542+
sourceNode.replaceWith(createBreakNodeFor(sourceNode));
1543+
} else {
1544+
// If we are in a finally block, we need to detach the source node to prevent an extra
1545+
// break from being generated. The break is not needed because the
1546+
// jumpThroughFinallyBlocks call will handle the control flow.
1547+
sourceNode.detach();
1548+
}
15341549
} else {
15351550
// "break;" inside a loop or swtich statement:
15361551
// for (...) {

test/com/google/javascript/jscomp/Es6RewriteGeneratorsTest.java

Lines changed: 83 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
import org.junit.runner.RunWith;
3434
import org.junit.runners.JUnit4;
3535

36+
/** Tests for {@link Es6RewriteGenerators} */
3637
@RunWith(JUnit4.class)
3738
public final class Es6RewriteGeneratorsTest extends CompilerTestCase {
3839

@@ -1914,9 +1915,46 @@ private Node getAndCheckGeneratorProgram(Node genFunction) {
19141915
return createGenerator.getNext().getNext();
19151916
}
19161917

1917-
// The following tests demonstrate the bug in how "break" is handled in a "finally" block.
1918-
// The "expected" output is the actual, incorrect output of the compiler, and a comment
1919-
// is added to the test to indicate that the test is expected to fail with the fix.
1918+
@Test
1919+
public void testBreakInNestedFinally() {
1920+
rewriteGeneratorSwitchBodyWithVars(
1921+
"""
1922+
OUTER: do {
1923+
try {
1924+
try {
1925+
yield 1;
1926+
} finally {
1927+
break OUTER;
1928+
}
1929+
} finally {
1930+
break OUTER;
1931+
}
1932+
} while (false);
1933+
""",
1934+
"",
1935+
"""
1936+
GEN_CONTEXT$0.setFinallyBlock(5);
1937+
GEN_CONTEXT$0.setFinallyBlock(7);
1938+
return GEN_CONTEXT$0.yield(1, 7);
1939+
case 7:
1940+
GEN_CONTEXT$0.enterFinallyBlock(0, 5);
1941+
GEN_CONTEXT$0.jumpThroughFinallyBlocks(0);
1942+
GEN_CONTEXT$0.leaveFinallyBlock(5);
1943+
break;
1944+
case 5:
1945+
GEN_CONTEXT$0.enterFinallyBlock();
1946+
GEN_CONTEXT$0.jumpThroughFinallyBlocks(0);
1947+
GEN_CONTEXT$0.leaveFinallyBlock(2);
1948+
break;
1949+
case 2:
1950+
if (false) {
1951+
GEN_CONTEXT$0.jumpTo(1);
1952+
break;
1953+
}
1954+
GEN_CONTEXT$0.jumpToEnd();
1955+
""");
1956+
}
1957+
19201958
@Test
19211959
public void testFinallyReturnCancelledByOuterBreak() {
19221960
rewriteGeneratorSwitchBodyWithVars(
@@ -1938,8 +1976,7 @@ public void testFinallyReturnCancelledByOuterBreak() {
19381976
return GEN_CONTEXT$0.return("innerTry");
19391977
case 5:
19401978
GEN_CONTEXT$0.enterFinallyBlock();
1941-
GEN_CONTEXT$0.jumpTo(0);
1942-
break; // This break is wrong. It should not be here.
1979+
GEN_CONTEXT$0.jumpThroughFinallyBlocks(0);
19431980
GEN_CONTEXT$0.leaveFinallyBlock(2);
19441981
break;
19451982
case 2:
@@ -1978,7 +2015,6 @@ public void testFinallyReturnCancelledByOuterBreak_nested() {
19782015
case 7:
19792016
GEN_CONTEXT$0.enterFinallyBlock(0, 5);
19802017
GEN_CONTEXT$0.jumpThroughFinallyBlocks(0);
1981-
break; // This break is wrong. It should not be here.
19822018
GEN_CONTEXT$0.leaveFinallyBlock(5);
19832019
break;
19842020
case 5:
@@ -1995,4 +2031,45 @@ public void testFinallyReturnCancelledByOuterBreak_nested() {
19952031
GEN_CONTEXT$0.jumpToEnd();
19962032
""");
19972033
}
2034+
2035+
@Test
2036+
public void testContinueInNestedFinallyInLoop() {
2037+
rewriteGeneratorSwitchBodyWithVars(
2038+
"""
2039+
OUT: do {
2040+
try {
2041+
yield 1;
2042+
} finally {
2043+
try {
2044+
yield 2;
2045+
} finally {
2046+
continue OUT;
2047+
}
2048+
}
2049+
} while (false);
2050+
""",
2051+
"",
2052+
"""
2053+
GEN_CONTEXT$0.setFinallyBlock(5);
2054+
return GEN_CONTEXT$0.yield(1, 5);
2055+
case 5:
2056+
GEN_CONTEXT$0.enterFinallyBlock();
2057+
GEN_CONTEXT$0.setFinallyBlock(8);
2058+
return GEN_CONTEXT$0.yield(2, 8);
2059+
case 8:
2060+
GEN_CONTEXT$0.enterFinallyBlock(0, 0, 1);
2061+
GEN_CONTEXT$0.jumpThroughFinallyBlocks(2);
2062+
GEN_CONTEXT$0.leaveFinallyBlock(9, 1);
2063+
break;
2064+
case 9:
2065+
GEN_CONTEXT$0.leaveFinallyBlock(2);
2066+
break;
2067+
case 2:
2068+
if (false) {
2069+
GEN_CONTEXT$0.jumpTo(1);
2070+
break;
2071+
}
2072+
GEN_CONTEXT$0.jumpToEnd();
2073+
""");
2074+
}
19982075
}

0 commit comments

Comments
 (0)