Skip to content

Commit 5d14a1a

Browse files
fglockfglock
committed
Implement control flow detection using visitor pattern
- Created ControlFlowDetectorVisitor implementing the Visitor interface - Uses proper visitor pattern to detect unsafe control flow (next/last/redo/goto) - Replaces manual AST traversal with clean, maintainable visitor implementation - EmitBlock now uses the visitor to check for unsafe control flow before refactoring This ensures blocks containing control flow statements that could jump outside are not refactored into separate subroutines, preserving program correctness. The visitor pattern makes the code more maintainable and follows established patterns in the codebase.
1 parent ee2b43a commit 5d14a1a

File tree

2 files changed

+215
-7
lines changed

2 files changed

+215
-7
lines changed

‎src/main/java/org/perlonjava/astvisitor/ControlFlowDetectorVisitor.java‎

Lines changed: 207 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,207 @@
1+
package org.perlonjava.astvisitor;
2+
3+
import org.perlonjava.astnode.*;
4+
5+
/**
6+
* Visitor that detects control flow statements (next, last, redo, goto)
7+
* that could potentially jump outside of a refactored block.
8+
*/
9+
public class ControlFlowDetectorVisitor implements Visitor {
10+
private boolean hasUnsafeControlFlow = false;
11+
12+
/**
13+
* Check if unsafe control flow was detected during traversal.
14+
* @return true if unsafe control flow statements were found
15+
*/
16+
public boolean hasUnsafeControlFlow() {
17+
return hasUnsafeControlFlow;
18+
}
19+
20+
/**
21+
* Reset the detector for reuse.
22+
*/
23+
public void reset() {
24+
hasUnsafeControlFlow = false;
25+
}
26+
27+
@Override
28+
public void visit(OperatorNode node) {
29+
// Check for control flow operators
30+
if ("next".equals(node.operator) || "last".equals(node.operator) ||
31+
"redo".equals(node.operator) || "goto".equals(node.operator)) {
32+
hasUnsafeControlFlow = true;
33+
return;
34+
}
35+
// Continue traversing
36+
if (node.operand != null) {
37+
node.operand.accept(this);
38+
}
39+
}
40+
41+
@Override
42+
public void visit(BlockNode node) {
43+
for (Node element : node.elements) {
44+
if (element != null) {
45+
element.accept(this);
46+
if (hasUnsafeControlFlow) {
47+
return; // Early exit once found
48+
}
49+
}
50+
}
51+
}
52+
53+
@Override
54+
public void visit(ListNode node) {
55+
for (Node element : node.elements) {
56+
if (element != null) {
57+
element.accept(this);
58+
if (hasUnsafeControlFlow) {
59+
return; // Early exit once found
60+
}
61+
}
62+
}
63+
}
64+
65+
@Override
66+
public void visit(BinaryOperatorNode node) {
67+
if (node.left != null) {
68+
node.left.accept(this);
69+
}
70+
if (!hasUnsafeControlFlow && node.right != null) {
71+
node.right.accept(this);
72+
}
73+
}
74+
75+
@Override
76+
public void visit(TernaryOperatorNode node) {
77+
if (node.condition != null) {
78+
node.condition.accept(this);
79+
}
80+
if (!hasUnsafeControlFlow && node.trueExpr != null) {
81+
node.trueExpr.accept(this);
82+
}
83+
if (!hasUnsafeControlFlow && node.falseExpr != null) {
84+
node.falseExpr.accept(this);
85+
}
86+
}
87+
88+
@Override
89+
public void visit(IfNode node) {
90+
if (node.condition != null) {
91+
node.condition.accept(this);
92+
}
93+
if (!hasUnsafeControlFlow && node.thenBranch != null) {
94+
node.thenBranch.accept(this);
95+
}
96+
if (!hasUnsafeControlFlow && node.elseBranch != null) {
97+
node.elseBranch.accept(this);
98+
}
99+
}
100+
101+
// For loops can contain control flow
102+
@Override
103+
public void visit(For1Node node) {
104+
if (node.variable != null) {
105+
node.variable.accept(this);
106+
}
107+
if (!hasUnsafeControlFlow && node.list != null) {
108+
node.list.accept(this);
109+
}
110+
if (!hasUnsafeControlFlow && node.body != null) {
111+
node.body.accept(this);
112+
}
113+
}
114+
115+
@Override
116+
public void visit(For3Node node) {
117+
if (node.initialization != null) {
118+
node.initialization.accept(this);
119+
}
120+
if (!hasUnsafeControlFlow && node.condition != null) {
121+
node.condition.accept(this);
122+
}
123+
if (!hasUnsafeControlFlow && node.increment != null) {
124+
node.increment.accept(this);
125+
}
126+
if (!hasUnsafeControlFlow && node.body != null) {
127+
node.body.accept(this);
128+
}
129+
}
130+
131+
@Override
132+
public void visit(TryNode node) {
133+
if (node.tryBlock != null) {
134+
node.tryBlock.accept(this);
135+
}
136+
if (!hasUnsafeControlFlow && node.catchBlock != null) {
137+
node.catchBlock.accept(this);
138+
}
139+
if (!hasUnsafeControlFlow && node.finallyBlock != null) {
140+
node.finallyBlock.accept(this);
141+
}
142+
}
143+
144+
// Simple implementations for other node types
145+
@Override
146+
public void visit(IdentifierNode node) {
147+
// No control flow in identifiers
148+
}
149+
150+
@Override
151+
public void visit(NumberNode node) {
152+
// No control flow in numbers
153+
}
154+
155+
@Override
156+
public void visit(StringNode node) {
157+
// No control flow in strings
158+
}
159+
160+
@Override
161+
public void visit(HashLiteralNode node) {
162+
for (Node element : node.elements) {
163+
if (element != null) {
164+
element.accept(this);
165+
if (hasUnsafeControlFlow) return;
166+
}
167+
}
168+
}
169+
170+
@Override
171+
public void visit(ArrayLiteralNode node) {
172+
for (Node element : node.elements) {
173+
if (element != null) {
174+
element.accept(this);
175+
if (hasUnsafeControlFlow) return;
176+
}
177+
}
178+
}
179+
180+
@Override
181+
public void visit(SubroutineNode node) {
182+
if (node.block != null) {
183+
node.block.accept(this);
184+
}
185+
}
186+
187+
@Override
188+
public void visit(LabelNode node) {
189+
// Labels themselves don't have control flow
190+
// The labeled statement is handled separately in the AST
191+
}
192+
193+
@Override
194+
public void visit(CompilerFlagNode node) {
195+
// Compiler flags don't have control flow
196+
}
197+
198+
@Override
199+
public void visit(FormatNode node) {
200+
// Formats don't have control flow statements
201+
}
202+
203+
@Override
204+
public void visit(FormatLine node) {
205+
// Format lines don't have control flow statements
206+
}
207+
}

‎src/main/java/org/perlonjava/codegen/EmitBlock.java‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import org.objectweb.asm.Label;
44
import org.objectweb.asm.MethodVisitor;
55
import org.perlonjava.astnode.*;
6+
import org.perlonjava.astvisitor.ControlFlowDetectorVisitor;
67
import org.perlonjava.astvisitor.EmitterVisitor;
78
import org.perlonjava.runtime.GlobalVariable;
89
import org.perlonjava.runtime.RuntimeContextType;
@@ -13,6 +14,9 @@ public class EmitBlock {
1314
// Blocks with too many statements are emitted as a separate subroutine
1415
// in order to avoid "Method too large" error test: in t/re/pat.t
1516
final static int LARGE_BLOCK = 16;
17+
18+
// Reusable visitor for control flow detection
19+
private static final ControlFlowDetectorVisitor controlFlowDetector = new ControlFlowDetectorVisitor();
1620

1721
/**
1822
* Emits bytecode for a block of statements.
@@ -27,13 +31,10 @@ public static void emitBlock(EmitterVisitor emitterVisitor, BlockNode node) {
2731
// Check for control flow that would break if refactored
2832
boolean hasUnsafeControlFlow = false;
2933
if (node.elements.size() > LARGE_BLOCK && !node.getBooleanAnnotation("blockIsSubroutine")) {
30-
// Convert block to string and check for control flow keywords
31-
String blockString = node.toString();
32-
// Check for next, last, redo, goto that might jump outside the block
33-
if (blockString.contains(" next ") || blockString.contains(" last ") ||
34-
blockString.contains(" redo ") || blockString.contains(" goto ")) {
35-
hasUnsafeControlFlow = true;
36-
}
34+
// Use visitor pattern to check for unsafe control flow
35+
controlFlowDetector.reset();
36+
node.accept(controlFlowDetector);
37+
hasUnsafeControlFlow = controlFlowDetector.hasUnsafeControlFlow();
3738
}
3839

3940
if (node.elements.size() > LARGE_BLOCK

0 commit comments

Comments
 (0)