Skip to content

Commit dbc3a5c

Browse files
jgardn3rcirras
jgardn3r
authored and
cirras
committed
Improve RedundantJumpCheck around nested try-finally constructs
1 parent de70e45 commit dbc3a5c

File tree

3 files changed

+146
-69
lines changed

3 files changed

+146
-69
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1111

1212
- Exceptions from empty structures (e.g., `if`) in `LoopExecutingAtMostOnce` and `RedundantJump`.
1313
- False positives from case statements in `LoopExecutingAtMostOnce`.
14+
- False positives from nested finally-except blocks in `RedundantJump`.
1415

1516
## [1.14.1] - 2025-03-05
1617

‎delphi-checks/src/main/java/au/com/integradev/delphi/checks/RedundantJumpCheck.java‎

Lines changed: 83 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -18,18 +18,20 @@
1818
*/
1919
package au.com.integradev.delphi.checks;
2020

21-
import au.com.integradev.delphi.antlr.ast.node.RoutineImplementationNodeImpl;
22-
import au.com.integradev.delphi.cfg.ControlFlowGraphFactory;
2321
import au.com.integradev.delphi.cfg.api.Block;
2422
import au.com.integradev.delphi.cfg.api.ControlFlowGraph;
2523
import au.com.integradev.delphi.cfg.api.Finally;
24+
import au.com.integradev.delphi.cfg.api.Terminated;
2625
import au.com.integradev.delphi.cfg.api.UnconditionalJump;
26+
import au.com.integradev.delphi.utils.ControlFlowGraphUtils;
27+
import java.util.ArrayDeque;
28+
import java.util.Deque;
2729
import org.sonar.check.Rule;
28-
import org.sonar.plugins.communitydelphi.api.ast.AnonymousMethodNode;
2930
import org.sonar.plugins.communitydelphi.api.ast.ArgumentListNode;
3031
import org.sonar.plugins.communitydelphi.api.ast.DelphiNode;
32+
import org.sonar.plugins.communitydelphi.api.ast.FinallyBlockNode;
3133
import org.sonar.plugins.communitydelphi.api.ast.NameReferenceNode;
32-
import org.sonar.plugins.communitydelphi.api.ast.RoutineImplementationNode;
34+
import org.sonar.plugins.communitydelphi.api.ast.TryStatementNode;
3335
import org.sonar.plugins.communitydelphi.api.check.DelphiCheck;
3436
import org.sonar.plugins.communitydelphi.api.check.DelphiCheckContext;
3537
import org.sonar.plugins.communitydelphi.api.symbol.declaration.NameDeclaration;
@@ -39,103 +41,115 @@
3941
public class RedundantJumpCheck extends DelphiCheck {
4042
private static final String MESSAGE = "Remove this redundant jump.";
4143

44+
private final Deque<TryStatementNode> tryFinallyStatements = new ArrayDeque<>();
45+
4246
@Override
43-
public DelphiCheckContext visit(RoutineImplementationNode routine, DelphiCheckContext context) {
44-
ControlFlowGraph cfg = ((RoutineImplementationNodeImpl) routine).getControlFlowGraph();
45-
if (cfg != null) {
46-
cfg.getBlocks().forEach(block -> checkBlock(block, context));
47+
public DelphiCheckContext visit(TryStatementNode node, DelphiCheckContext data) {
48+
boolean finallyBlock = node.hasFinallyBlock();
49+
if (!finallyBlock) {
50+
return super.visit(node, data);
4751
}
48-
49-
return super.visit(routine, context);
52+
this.tryFinallyStatements.push(node);
53+
super.visit(node.getStatementList(), data);
54+
this.tryFinallyStatements.pop();
55+
return super.visit(node.getFinallyBlock(), data);
5056
}
5157

5258
@Override
53-
public DelphiCheckContext visit(AnonymousMethodNode routine, DelphiCheckContext context) {
54-
ControlFlowGraph cfg = ControlFlowGraphFactory.create(routine.getStatementBlock());
55-
cfg.getBlocks().forEach(block -> checkBlock(block, context));
59+
public DelphiCheckContext visit(NameReferenceNode node, DelphiCheckContext data) {
60+
RoutineNameDeclaration routineNameDeclaration = getRoutineNameDeclaration(node);
61+
if (routineNameDeclaration != null
62+
&& (isContinue(routineNameDeclaration)
63+
|| isExitWithoutArgs(routineNameDeclaration, node))) {
64+
checkJumpNode(node, data);
65+
return data;
66+
}
67+
return super.visit(node, data);
68+
}
5669

57-
return super.visit(routine, context);
70+
private static RoutineNameDeclaration getRoutineNameDeclaration(NameReferenceNode node) {
71+
NameDeclaration nameDeclaration = node.getNameDeclaration();
72+
if (nameDeclaration instanceof RoutineNameDeclaration) {
73+
return (RoutineNameDeclaration) nameDeclaration;
74+
}
75+
return null;
5876
}
5977

60-
private void checkBlock(Block block, DelphiCheckContext context) {
61-
if (!(block instanceof UnconditionalJump)) {
62-
return;
78+
private static boolean isContinue(RoutineNameDeclaration node) {
79+
String name = node.fullyQualifiedName();
80+
return name.equals("System.Continue");
81+
}
82+
83+
private static boolean isExitWithoutArgs(RoutineNameDeclaration node, DelphiNode statement) {
84+
String name = node.fullyQualifiedName();
85+
if (!name.equals("System.Exit")) {
86+
return false;
6387
}
6488

65-
UnconditionalJump jump = (UnconditionalJump) block;
66-
DelphiNode terminator = jump.getTerminator();
89+
var argumentList = statement.getParent().getFirstChildOfType(ArgumentListNode.class);
90+
int arguments = argumentList == null ? 0 : argumentList.getArgumentNodes().size();
91+
92+
return arguments == 0;
93+
}
94+
95+
private static Block findBlockWithTerminator(ControlFlowGraph cfg, DelphiNode node) {
96+
for (Block block : cfg.getBlocks()) {
97+
if ((block instanceof Terminated) && ((Terminated) block).getTerminator() == node) {
98+
return block;
99+
}
100+
}
101+
return null;
102+
}
67103

68-
RoutineNameDeclaration routineNameDeclaration = getRoutineNameDeclaration(terminator);
69-
if (routineNameDeclaration == null) {
104+
private void checkJumpNode(NameReferenceNode node, DelphiCheckContext context) {
105+
ControlFlowGraph cfg = ControlFlowGraphUtils.findContainingCFG(node);
106+
if (cfg == null) {
70107
return;
71108
}
72109

73-
if (!isContinueOrExit(routineNameDeclaration)
74-
|| isExitWithExpression(routineNameDeclaration, terminator)) {
110+
Block terminatedBlock = findBlockWithTerminator(cfg, node);
111+
if (!(terminatedBlock instanceof UnconditionalJump)) {
112+
// can't be a redundant jump without a jump
75113
return;
76114
}
77115

116+
UnconditionalJump jump = (UnconditionalJump) terminatedBlock;
78117
Block successor = jump.getSuccessor();
79118
if (!successor.equals(jump.getSuccessorIfRemoved())) {
119+
// without the jump, the successor block would be different
80120
return;
81121
}
82122

83-
Block finallyBlock = getFinallyBlock(block);
84-
if (finallyBlock != null) {
85-
if (onlyFinallyBlocksBeforeEnd(finallyBlock)) {
86-
reportIssue(context, terminator, MESSAGE);
87-
}
88-
return;
123+
if (isViolation(cfg)) {
124+
reportIssue(context, jump.getTerminator(), MESSAGE);
89125
}
90-
91-
reportIssue(context, terminator, MESSAGE);
92-
}
93-
94-
private static Block getFinallyBlock(Block block) {
95-
return block.getSuccessors().stream()
96-
.filter(Finally.class::isInstance)
97-
.findFirst()
98-
.orElse(null);
99126
}
100127

101-
private static boolean onlyFinallyBlocksBeforeEnd(Block finallyBlock) {
102-
while (finallyBlock.getSuccessors().size() == 1) {
103-
Block finallySuccessor = finallyBlock.getSuccessors().iterator().next();
104-
if (!(finallySuccessor instanceof Finally)) {
128+
private boolean isViolation(ControlFlowGraph cfg) {
129+
for (TryStatementNode tryFinally : this.tryFinallyStatements) {
130+
Finally finallyBlock = findFinallyBlock(cfg, tryFinally.getFinallyBlock());
131+
if (finallyBlock == null) {
132+
// if the finally block cannot be found, we have traversed outside the scope of the cfg
105133
break;
106134
}
107-
finallyBlock = finallySuccessor;
135+
if (!finallyBlock.getSuccessor().equals(finallyBlock.getExceptionSuccessor())) {
136+
// multiple paths after the finally corresponds to code that would be skipped from the jump
137+
return false;
138+
}
108139
}
109-
return finallyBlock.getSuccessors().size() == 1
110-
&& finallyBlock.getSuccessors().iterator().next().getSuccessors().isEmpty();
140+
// if no invalidating try-finally blocks are found, the use is a violation
141+
return true;
111142
}
112143

113-
private static RoutineNameDeclaration getRoutineNameDeclaration(DelphiNode node) {
114-
NameDeclaration nameDeclaration = null;
115-
if (node instanceof NameReferenceNode) {
116-
nameDeclaration = ((NameReferenceNode) node).getNameDeclaration();
144+
private static Finally findFinallyBlock(ControlFlowGraph cfg, FinallyBlockNode element) {
145+
if (element == null) {
146+
return null;
117147
}
118-
if (nameDeclaration instanceof RoutineNameDeclaration) {
119-
return (RoutineNameDeclaration) nameDeclaration;
148+
for (Block block : cfg.getBlocks()) {
149+
if ((block instanceof Finally) && ((Finally) block).getTerminator().equals(element)) {
150+
return (Finally) block;
151+
}
120152
}
121153
return null;
122154
}
123-
124-
private static boolean isContinueOrExit(RoutineNameDeclaration routineNameDeclaration) {
125-
String fullyQualifiedName = routineNameDeclaration.fullyQualifiedName();
126-
return fullyQualifiedName.equals("System.Continue") || fullyQualifiedName.equals("System.Exit");
127-
}
128-
129-
private static boolean isExitWithExpression(
130-
RoutineNameDeclaration routineNameDeclaration, DelphiNode statement) {
131-
String fullyQualifiedName = routineNameDeclaration.fullyQualifiedName();
132-
if (!fullyQualifiedName.equals("System.Exit")) {
133-
return false;
134-
}
135-
136-
var argumentList = statement.getParent().getFirstChildOfType(ArgumentListNode.class);
137-
int arguments = argumentList == null ? 0 : argumentList.getArgumentNodes().size();
138-
139-
return arguments > 0;
140-
}
141155
}

‎delphi-checks/src/test/java/au/com/integradev/delphi/checks/RedundantJumpCheckTest.java‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -262,6 +262,11 @@ void testExitInTryFinallyAtEndShouldAddIssue() {
262262
List.of("try", " if True then Exit; // Noncompliant", "finally", " Foo1;", "end;"), true);
263263
}
264264

265+
@Test
266+
void testExitInExceptShouldAddIssue() {
267+
doExitTest(List.of("try", " Exit; // Noncompliant", "except", " Foo2;", "end;"), true);
268+
}
269+
265270
@Test
266271
void testExitInNestedTryFinallyAtEndShouldAddIssue() {
267272
doExitTest(
@@ -330,6 +335,27 @@ void testExceptExitShouldNotAddIssue() {
330335
false);
331336
}
332337

338+
@Test
339+
void testExceptExitWithStatementAfterShouldNotAddIssue() {
340+
doExitTest(
341+
List.of(
342+
"try",
343+
" try",
344+
" A;",
345+
" except",
346+
" on E: Exception do begin",
347+
" Exit;",
348+
" end;",
349+
" end;",
350+
"finally;",
351+
" if B then begin",
352+
" C;",
353+
" end;",
354+
"end;",
355+
"C;"),
356+
false);
357+
}
358+
333359
@Test
334360
void testForLoopAfterConditionalTryFinallyExitShouldNotAddIssue() {
335361
doExitTest(
@@ -363,6 +389,42 @@ void testConditionalExitInForLoopTryFinallyShouldNotAddIssue() {
363389
false);
364390
}
365391

392+
@Test
393+
void testRedundantTryFinallyNestedAnonymousMethodShouldAddIssue() {
394+
doExitTest(
395+
List.of(
396+
"try",
397+
" var A := procedure",
398+
" begin",
399+
" try",
400+
" Exit; // Noncompliant",
401+
" finally",
402+
" end;",
403+
" end;",
404+
"finally",
405+
"end;",
406+
"A;"),
407+
true);
408+
}
409+
410+
@Test
411+
void testNonRedundantTryFinallyNestedAnonymousMethodShouldNotAddIssue() {
412+
doExitTest(
413+
List.of(
414+
"try",
415+
" var A := procedure",
416+
" begin",
417+
" try",
418+
" Exit;",
419+
" finally",
420+
" end;",
421+
" A;",
422+
" end;",
423+
"finally",
424+
"end;"),
425+
false);
426+
}
427+
366428
@Test
367429
void testAnonymousMethodExitShouldAddIssue() {
368430
doExitTest(

0 commit comments

Comments
 (0)