Skip to content

Commit 255c15b

Browse files
rishipalcopybara-github
authored andcommitted
Make control flow analysis of infinite FOR consistent with infinite while loops
An upcoming change in MaybeReachingVariableUse unit tests exposed an issue with FOR loops where `for(i=0; ; i++) {x=1;} foo(x)` would still create an edge (Branch.ON_FALSE) in the CFG from the FOR node to the `foo(x)` node after the block ends. This behavior is incorrect because, with the Branch.ON_FALSE down edge from FOR to `foo(x)` the MaybeReachingVariableUse pass will consider the foo(x) use reachable to the def `x=1`, when in reality it is not reachable. Inconsistency: `for(;;) { x=1;} foo(x);` // considered not reachable use `for(;true;) { x=1;} foo(x);` // considered reachable use This CL fixes the above inconsistency. PiperOrigin-RevId: 322491736
1 parent 8e0c656 commit 255c15b

File tree

2 files changed

+55
-6
lines changed

2 files changed

+55
-6
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -450,7 +450,7 @@ private void handleFor(Node forNode) {
450450
// The edge that transfer control to the beginning of the loop body.
451451
createEdge(forNode, Branch.ON_TRUE, computeFallThrough(body));
452452
// The edge to end of the loop.
453-
if (!cond.isEmpty()) {
453+
if (!cond.isEmpty() && !cond.isTrue()) {
454454
createEdge(forNode, Branch.ON_FALSE, computeFollowNode(forNode, this));
455455
}
456456
// The end of the body will have a unconditional branch to our iter

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

Lines changed: 54 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -155,11 +155,10 @@ private static List<DiGraphEdge<Node, Branch>> getAllDownEdges(
155155
}
156156

157157
/**
158-
* Assert that there exists a control flow edge of the given type
159-
* from some node with the first token to some node with the second token.
158+
* Assert that there exists no control flow edge of the given type from some node with the first
159+
* token to some node with the second token.
160160
*/
161-
private static void assertNoEdge(ControlFlowGraph<Node> cfg, Token startToken,
162-
Token endToken) {
161+
private static void assertNoEdge(ControlFlowGraph<Node> cfg, Token startToken, Token endToken) {
163162
assertThat(getAllEdges(cfg, startToken, endToken)).isEmpty();
164163
}
165164

@@ -310,12 +309,62 @@ public void testSimpleIf() throws IOException {
310309

311310
@Test
312311
public void testBreakingBlock() throws IOException {
313-
// BUG #1382217
314312
String src = "X: { while(1) { break } }";
315313
ControlFlowGraph<Node> cfg = createCfg(src);
316314
assertUpEdge(cfg, Token.BREAK, Token.BLOCK, Branch.UNCOND);
317315
}
318316

317+
@Test
318+
public void testBreakingWhile() {
319+
String src = "var x; while(true) { break; } x();";
320+
ControlFlowGraph<Node> cfg = createCfg(src);
321+
assertDownEdge(cfg, Token.WHILE, Token.BLOCK, Branch.ON_TRUE);
322+
assertDownEdge(cfg, Token.BLOCK, Token.BREAK, Branch.UNCOND);
323+
assertCrossEdge(cfg, Token.BREAK, Token.EXPR_RESULT, Branch.UNCOND);
324+
}
325+
326+
@Test
327+
public void testInifiteLoopWhile() {
328+
String src = "var x; while(true) { } x();";
329+
ControlFlowGraph<Node> cfg = createCfg(src);
330+
assertDownEdge(cfg, Token.WHILE, Token.BLOCK, Branch.ON_TRUE);
331+
assertNoEdge(cfg, Token.WHILE, Token.EXPR_RESULT);
332+
}
333+
334+
@Test
335+
public void testInifiteLoopFor_emptyCond() {
336+
String src = "var x; for(;;) { } x();";
337+
ControlFlowGraph<Node> cfg = createCfg(src);
338+
assertDownEdge(cfg, Token.FOR, Token.BLOCK, Branch.ON_TRUE);
339+
assertNoEdge(cfg, Token.FOR, Token.EXPR_RESULT);
340+
}
341+
342+
@Test
343+
public void testBreakingFor_emptyCond() {
344+
String src = "var x; for(;;) { break; } x();";
345+
ControlFlowGraph<Node> cfg = createCfg(src);
346+
assertDownEdge(cfg, Token.FOR, Token.BLOCK, Branch.ON_TRUE);
347+
assertDownEdge(cfg, Token.BLOCK, Token.BREAK, Branch.UNCOND);
348+
assertCrossEdge(cfg, Token.BREAK, Token.EXPR_RESULT, Branch.UNCOND);
349+
}
350+
351+
@Test
352+
public void testInifiteLoopFor_trueCond() {
353+
String src = "var x; for(;true;) { } x();";
354+
ControlFlowGraph<Node> cfg = createCfg(src);
355+
assertDownEdge(cfg, Token.FOR, Token.BLOCK, Branch.ON_TRUE);
356+
assertNoEdge(cfg, Token.FOR, Token.EXPR_RESULT);
357+
}
358+
359+
@Test
360+
public void testBreakingFor_trueCond() {
361+
String src = "var x; for(;true;) { break; } x();";
362+
ControlFlowGraph<Node> cfg = createCfg(src);
363+
assertDownEdge(cfg, Token.FOR, Token.BLOCK, Branch.ON_TRUE);
364+
assertDownEdge(cfg, Token.BLOCK, Token.BREAK, Branch.UNCOND);
365+
assertCrossEdge(cfg, Token.BREAK, Token.EXPR_RESULT, Branch.UNCOND);
366+
}
367+
319368
@Test
320369
public void testThrowInCatchBlock() throws IOException {
321370
String src = "try { throw ''; } catch (e) { throw e;} finally {}";

0 commit comments

Comments
 (0)