Skip to content

Commit 434a17d

Browse files
jgardn3rjgardn3r
committed
Add check for loops executing at most once
1 parent f5544d6 commit 434a17d

File tree

6 files changed

+616
-0
lines changed

6 files changed

+616
-0
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
3232
- "Correct to (correct case)" quick fix for `LowercaseKeyword`.
3333
- Ability to create a control flow graph.
3434
- `RedundantJump` analysis rule, which flags redundant jump statements, e.g., `Continue`, `Exit`.
35+
- `LoopExecutingAtMostOnce` analysis rule, which flags loop statements that can execute at most once.
3536
- **API:** `PropertyNameDeclaration::getImplementedTypes` method.
3637
- **API:** `PropertyNode::getDefaultSpecifier` method.
3738
- **API:** `PropertyNode::getImplementsSpecifier` method.

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,7 @@ public final class CheckList {
103103
InterfaceGuidCheck.class,
104104
InterfaceNameCheck.class,
105105
LegacyInitializationSectionCheck.class,
106+
LoopExecutingAtMostOnceCheck.class,
106107
LowercaseKeywordCheck.class,
107108
MathFunctionSingleOverloadCheck.class,
108109
MemberDeclarationOrderCheck.class,

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

Lines changed: 254 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,254 @@
1+
/*
2+
* Sonar Delphi Plugin
3+
* Copyright (C) 2019 Integrated Application Development
4+
*
5+
* This program is free software; you can redistribute it and/or
6+
* modify it under the terms of the GNU Lesser General Public
7+
* License as published by the Free Software Foundation; either
8+
* version 3 of the License, or (at your option) any later version.
9+
*
10+
* This program is distributed in the hope that it will be useful,
11+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
12+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
13+
* Lesser General Public License for more details.
14+
*
15+
* You should have received a copy of the GNU Lesser General Public
16+
* License along with this program; if not, write to the Free Software
17+
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02
18+
*/
19+
package au.com.integradev.delphi.checks;
20+
21+
import au.com.integradev.delphi.antlr.ast.node.AnonymousMethodNodeImpl;
22+
import au.com.integradev.delphi.antlr.ast.node.RoutineImplementationNodeImpl;
23+
import au.com.integradev.delphi.cfg.ControlFlowGraphImpl;
24+
import au.com.integradev.delphi.cfg.ControlFlowGraphImpl.Block;
25+
import java.util.HashSet;
26+
import java.util.List;
27+
import java.util.Optional;
28+
import java.util.PriorityQueue;
29+
import java.util.Set;
30+
import java.util.function.Supplier;
31+
import org.sonar.check.Rule;
32+
import org.sonar.plugins.communitydelphi.api.ast.CompoundStatementNode;
33+
import org.sonar.plugins.communitydelphi.api.ast.DelphiNode;
34+
import org.sonar.plugins.communitydelphi.api.ast.ForInStatementNode;
35+
import org.sonar.plugins.communitydelphi.api.ast.ForStatementNode;
36+
import org.sonar.plugins.communitydelphi.api.ast.ForToStatementNode;
37+
import org.sonar.plugins.communitydelphi.api.ast.GotoStatementNode;
38+
import org.sonar.plugins.communitydelphi.api.ast.IfStatementNode;
39+
import org.sonar.plugins.communitydelphi.api.ast.NameReferenceNode;
40+
import org.sonar.plugins.communitydelphi.api.ast.RaiseStatementNode;
41+
import org.sonar.plugins.communitydelphi.api.ast.RepeatStatementNode;
42+
import org.sonar.plugins.communitydelphi.api.ast.StatementNode;
43+
import org.sonar.plugins.communitydelphi.api.ast.WhileStatementNode;
44+
import org.sonar.plugins.communitydelphi.api.cfg.ControlFlowGraph;
45+
import org.sonar.plugins.communitydelphi.api.check.DelphiCheck;
46+
import org.sonar.plugins.communitydelphi.api.check.DelphiCheckContext;
47+
import org.sonar.plugins.communitydelphi.api.symbol.declaration.NameDeclaration;
48+
import org.sonar.plugins.communitydelphi.api.symbol.declaration.RoutineNameDeclaration;
49+
50+
@Rule(key = "LoopExecutingAtMostOnce")
51+
public class LoopExecutingAtMostOnceCheck extends DelphiCheck {
52+
private static final Set<String> EXIT_METHODS =
53+
Set.of("System.Exit", "System.Break", "System.Halt");
54+
55+
@Override
56+
public DelphiCheckContext visit(RaiseStatementNode node, DelphiCheckContext context) {
57+
return visitExitingNode(node, context, "raise");
58+
}
59+
60+
@Override
61+
public DelphiCheckContext visit(GotoStatementNode node, DelphiCheckContext context) {
62+
return visitExitingNode(node, context, "goto");
63+
}
64+
65+
@Override
66+
public DelphiCheckContext visit(NameReferenceNode node, DelphiCheckContext context) {
67+
NameDeclaration declaration = node.getLastName().getNameDeclaration();
68+
if (!(declaration instanceof RoutineNameDeclaration)) {
69+
return context;
70+
}
71+
String fullyQualifiedName = ((RoutineNameDeclaration) declaration).fullyQualifiedName();
72+
if (!EXIT_METHODS.contains(fullyQualifiedName)) {
73+
return context;
74+
}
75+
76+
return visitExitingNode(node, context, fullyQualifiedName);
77+
}
78+
79+
private DelphiCheckContext visitExitingNode(
80+
DelphiNode node, DelphiCheckContext context, String description) {
81+
DelphiNode enclosingStatement = findEnclosingStatement(node);
82+
DelphiNode enclosingLoop = findEnclosingLoop(node);
83+
if (enclosingStatement == null || enclosingLoop == null) {
84+
return context;
85+
}
86+
87+
if (isViolation(enclosingLoop, node) && violationInRelevantStatement(enclosingStatement)) {
88+
reportIssue(
89+
context,
90+
node,
91+
String.format("Remove this \"%s\" statement or make it conditional.", description));
92+
}
93+
return context;
94+
}
95+
96+
private static boolean isLoopNode(DelphiNode node) {
97+
return node instanceof RepeatStatementNode
98+
|| node instanceof ForStatementNode
99+
|| node instanceof WhileStatementNode;
100+
}
101+
102+
private static DelphiNode findEnclosingStatement(DelphiNode node) {
103+
DelphiNode parent = node;
104+
if (!(parent instanceof StatementNode)) {
105+
// Finding the enclosing statement of the NameReferenceNode
106+
parent = parent.getFirstParentOfType(StatementNode.class);
107+
}
108+
if (parent == null) {
109+
return null;
110+
}
111+
do {
112+
parent = parent.getFirstParentOfType(StatementNode.class);
113+
} while (parent instanceof CompoundStatementNode);
114+
return parent;
115+
}
116+
117+
private static DelphiNode findEnclosingLoop(DelphiNode node) {
118+
DelphiNode parent = node;
119+
while (parent != null && !isLoopNode(parent)) {
120+
parent = parent.getFirstParentOfType(StatementNode.class);
121+
}
122+
return parent;
123+
}
124+
125+
private static boolean isViolation(DelphiNode loop, DelphiNode jump) {
126+
ControlFlowGraphImpl cfg = getCFG(loop);
127+
Block loopBlock =
128+
getTerminatorBlock(cfg, loop)
129+
.orElseThrow(
130+
() -> new IllegalStateException("CFG necessarily contains the loop block"));
131+
132+
return !hasPredecessorInBlock(loopBlock, loop) && !jumpsBeforeLoop(cfg, loopBlock, jump);
133+
}
134+
135+
private static boolean violationInRelevantStatement(DelphiNode enclosingStatement) {
136+
if (!(enclosingStatement instanceof IfStatementNode)) {
137+
return true;
138+
}
139+
140+
IfStatementNode ifStatement = (IfStatementNode) enclosingStatement;
141+
if (!ifStatement.hasElseBranch()) {
142+
return false;
143+
}
144+
return !(ifStatement.getElseStatement() instanceof IfStatementNode);
145+
}
146+
147+
private static Optional<Block> getTerminatorBlock(ControlFlowGraphImpl cfg, DelphiNode loop) {
148+
return cfg.blocks().stream().filter(block -> loop.equals(block.terminator())).findFirst();
149+
}
150+
151+
private static boolean hasPredecessorInBlock(Block block, DelphiNode loop) {
152+
for (Block predecessor : block.predecessors()) {
153+
List<DelphiNode> predecessorElements = predecessor.elements();
154+
if (predecessorElements.isEmpty()) {
155+
return hasPredecessorInBlock(predecessor, loop);
156+
}
157+
DelphiNode predecessorFirstElement = predecessorElements.get(0);
158+
159+
if (isForStatementInitializer(predecessorFirstElement, loop)) {
160+
continue;
161+
}
162+
163+
if (isDescendant(predecessorFirstElement, loop)) {
164+
return true;
165+
}
166+
}
167+
168+
return false;
169+
}
170+
171+
private static boolean jumpsBeforeLoop(
172+
ControlFlowGraphImpl cfg, Block loopBlock, DelphiNode jump) {
173+
if (!(jump instanceof GotoStatementNode)) {
174+
return false;
175+
}
176+
Optional<Block> jumpBlock = getTerminatorBlock(cfg, jump);
177+
if (jumpBlock.isEmpty()) {
178+
return false;
179+
}
180+
Block jumpTarget = jumpBlock.get().successors().iterator().next();
181+
if (jumpTarget == null) {
182+
return false;
183+
}
184+
if (loopBlock.terminator() instanceof RepeatStatementNode
185+
&& loopBlock.falseBlock().equals(jumpTarget)) {
186+
return true;
187+
}
188+
189+
Set<Block> visited = new HashSet<>();
190+
PriorityQueue<Block> queue =
191+
new PriorityQueue<>((a, b) -> Math.abs(a.id() - b.id() - loopBlock.id()));
192+
queue.add(jumpTarget);
193+
while (!queue.isEmpty()) {
194+
Block search = queue.poll();
195+
if (search.equals(loopBlock)) {
196+
return true;
197+
}
198+
if (search.successors().size() == 1 && search.successors().contains(jumpTarget)) {
199+
return false;
200+
}
201+
202+
if (search.equals(cfg.exitBlock())) {
203+
return false;
204+
}
205+
visited.add(search);
206+
search.successors().stream().filter(b -> !visited.contains(b)).forEach(queue::add);
207+
}
208+
209+
return false;
210+
}
211+
212+
private static boolean isForStatementInitializer(DelphiNode lastElement, DelphiNode loop) {
213+
if (loop instanceof ForToStatementNode) {
214+
return isDescendant(lastElement, ((ForToStatementNode) loop).getInitializerExpression())
215+
|| isDescendant(lastElement, ((ForToStatementNode) loop).getTargetExpression());
216+
}
217+
return loop instanceof ForInStatementNode
218+
&& isDescendant(lastElement, ((ForInStatementNode) loop).getEnumerable());
219+
}
220+
221+
private static boolean isDescendant(DelphiNode descendant, DelphiNode target) {
222+
DelphiNode parent = descendant;
223+
while (parent != null) {
224+
if (parent.equals(target)) {
225+
return true;
226+
}
227+
parent = parent.getParent();
228+
}
229+
return false;
230+
}
231+
232+
private static Supplier<ControlFlowGraph> getCFGSupplier(DelphiNode node) {
233+
if (node instanceof RoutineImplementationNodeImpl) {
234+
return ((RoutineImplementationNodeImpl) node)::cfg;
235+
}
236+
if (node instanceof AnonymousMethodNodeImpl) {
237+
return ((AnonymousMethodNodeImpl) node)::cfg;
238+
}
239+
return null;
240+
}
241+
242+
private static ControlFlowGraphImpl getCFG(DelphiNode loop) {
243+
DelphiNode parent = loop.getParent();
244+
Supplier<ControlFlowGraph> cfgSupplier = getCFGSupplier(parent);
245+
while (parent != null && cfgSupplier == null) {
246+
parent = parent.getParent();
247+
cfgSupplier = getCFGSupplier(parent);
248+
}
249+
if (cfgSupplier != null) {
250+
return (ControlFlowGraphImpl) cfgSupplier.get();
251+
}
252+
return new ControlFlowGraphImpl(loop.findChildrenOfType(StatementNode.class));
253+
}
254+
}

‎delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/LoopExecutingAtMostOnce.html

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
<h2>Why is this an issue?</h2>
2+
<p>
3+
Loops with at most one iteration are equivalent to an <code>if</code> statement. Using loops in
4+
this place makes the code less readable.
5+
</p>
6+
<p>
7+
If the intention was to execute the loop once, an <code>if</code> statement should be used.
8+
Otherwise, the jumping statement should be made conditional such that loop can execute multiple
9+
times.
10+
</p>
11+
<p>
12+
Loops with at most one iteration can happen with a statement that unconditionally transfers
13+
control is misplaced inside the body of the loop.
14+
</p>
15+
<p>
16+
These statements are:
17+
</p>
18+
<ul>
19+
<li><code>Exit</code></li>
20+
<li><code>Break</code></li>
21+
<li><code>Continue</code></li>
22+
<li><code>Halt</code></li>
23+
<li><code>raise</code></li>
24+
<li><code>goto</code></li>
25+
</ul>
26+
27+
<h2>How to fix it</h2>
28+
<p>
29+
Make the statement that affects execution of the loop conditional, or remove it all together.
30+
</p>
31+
<pre data-diff-id="1" data-diff-type="noncompliant">
32+
var I := 0;
33+
while I &lt; 10 do begin
34+
Inc(I);
35+
break; // Noncompliant
36+
end;
37+
</pre>
38+
<pre data-diff-id="2" data-diff-type="noncompliant">
39+
for var I := 0 to 10 do begin
40+
if I = 2 then
41+
break // Noncompliant
42+
else begin
43+
Writeln(I);
44+
return; // Noncompliant
45+
end;
46+
end;
47+
</pre>
48+
<h4>Compliant solution</h4>
49+
<pre data-diff-id="1" data-diff-type="compliant">
50+
var I := 0;
51+
while I &lt; 10 do begin
52+
Inc(I);
53+
end;
54+
</pre>
55+
<pre data-diff-id="2" data-diff-type="compliant">
56+
for var I := 0 to 10 do begin
57+
if I = 2 then
58+
break
59+
else
60+
Writeln(I);
61+
end;
62+
</pre>

‎delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/LoopExecutingAtMostOnce.json

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
{
2+
"title": "Loops with at most one iteration should be refactored",
3+
"type": "BUG",
4+
"status": "ready",
5+
"remediation": {
6+
"func": "Constant/Issue",
7+
"constantCost": "5min"
8+
},
9+
"code": {
10+
"attribute": "CLEAR",
11+
"impacts": {
12+
"RELIABILITY": "MEDIUM"
13+
}
14+
},
15+
"tags": ["clumsy"],
16+
"defaultSeverity": "Major",
17+
"scope": "ALL",
18+
"quickfix": "unknown"
19+
}

0 commit comments

Comments
 (0)