Skip to content

Commit 802dad0

Browse files
fourlsfourls
committed
Add check for unraised exceptions
1 parent 83b0a66 commit 802dad0

File tree

7 files changed

+322
-0
lines changed

7 files changed

+322
-0
lines changed

‎CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
99

1010
### Added
1111

12+
- `MissingRaise` analysis rule, which flags exception constructions for which `raise` has been omitted.
1213
- **API:** `ProceduralType::directives` method.
1314
- **API:** `ProceduralTypeNode::getDirectives` method.
1415
- **API:** `ProceduralTypeNode::hasDirective` method.

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

+1
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,7 @@ public final class CheckList {
107107
LowercaseKeywordCheck.class,
108108
MathFunctionSingleOverloadCheck.class,
109109
MemberDeclarationOrderCheck.class,
110+
MissingRaiseCheck.class,
110111
MissingSemicolonCheck.class,
111112
MixedNamesCheck.class,
112113
NilComparisonCheck.class,

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

+84
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
/*
2+
* Sonar Delphi Plugin
3+
* Copyright (C) 2024 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 org.sonar.check.Rule;
22+
import org.sonar.plugins.communitydelphi.api.ast.ExpressionNode;
23+
import org.sonar.plugins.communitydelphi.api.ast.ExpressionStatementNode;
24+
import org.sonar.plugins.communitydelphi.api.ast.NameReferenceNode;
25+
import org.sonar.plugins.communitydelphi.api.ast.PrimaryExpressionNode;
26+
import org.sonar.plugins.communitydelphi.api.check.DelphiCheck;
27+
import org.sonar.plugins.communitydelphi.api.check.DelphiCheckContext;
28+
import org.sonar.plugins.communitydelphi.api.reporting.QuickFix;
29+
import org.sonar.plugins.communitydelphi.api.reporting.QuickFixEdit;
30+
import org.sonar.plugins.communitydelphi.api.symbol.declaration.NameDeclaration;
31+
import org.sonar.plugins.communitydelphi.api.symbol.declaration.RoutineKind;
32+
import org.sonar.plugins.communitydelphi.api.symbol.declaration.RoutineNameDeclaration;
33+
import org.sonar.plugins.communitydelphi.api.symbol.declaration.TypeNameDeclaration;
34+
import org.sonar.plugins.communitydelphi.api.type.Type;
35+
36+
@Rule(key = "MissingRaise")
37+
public class MissingRaiseCheck extends DelphiCheck {
38+
private static final String BASE_EXCEPTION = "System.SysUtils.Exception";
39+
40+
@Override
41+
public DelphiCheckContext visit(
42+
ExpressionStatementNode expressionStatement, DelphiCheckContext context) {
43+
if (isExceptionConstructorInvocation(expressionStatement.getExpression())) {
44+
context
45+
.newIssue()
46+
.onNode(expressionStatement)
47+
.withMessage("Raise or delete this exception.")
48+
.withQuickFixes(
49+
QuickFix.newFix("Raise exception")
50+
.withEdit(
51+
QuickFixEdit.insertBefore("raise ", expressionStatement.getExpression())))
52+
.report();
53+
}
54+
55+
return context;
56+
}
57+
58+
private boolean isExceptionConstructorInvocation(ExpressionNode expression) {
59+
if (!(expression instanceof PrimaryExpressionNode)
60+
|| !(expression.getChild(0) instanceof NameReferenceNode)) {
61+
return false;
62+
}
63+
64+
NameDeclaration declaration =
65+
((NameReferenceNode) expression.getChild(0)).getLastName().getNameDeclaration();
66+
if (!(declaration instanceof RoutineNameDeclaration)) {
67+
return false;
68+
}
69+
70+
RoutineNameDeclaration routineDeclaration = (RoutineNameDeclaration) declaration;
71+
if (routineDeclaration.getRoutineKind() != RoutineKind.CONSTRUCTOR) {
72+
return false;
73+
}
74+
75+
TypeNameDeclaration typeDecl = routineDeclaration.getTypeDeclaration();
76+
if (typeDecl == null) {
77+
return false;
78+
}
79+
80+
Type type = typeDecl.getType();
81+
82+
return type.is(BASE_EXCEPTION) || type.isDescendantOf(BASE_EXCEPTION);
83+
}
84+
}

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

+32
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
<h2>Why is this an issue?</h2>
2+
<p>
3+
When writing an exception raise in Delphi, a common mistake is to omit the <code>raise</code>,
4+
causing the exception to be created but not actually used. This is bad for a number of reasons:
5+
</p>
6+
<ul>
7+
<li>The exception object is never freed, causing a memory leak.</li>
8+
<li>Control flow continues as usual, even though an undesired exceptional state has been reached.</li>
9+
<li>The bug is easy to miss but can greatly alter the path of execution.</li>
10+
</ul>
11+
<h2>How to fix it</h2>
12+
<p>Add the <code>raise</code> keyword. If the exception is not required, delete the constructor invocation instead.</p>
13+
<pre data-diff-id="1" data-diff-type="noncompliant">
14+
procedure DeleteDatabase;
15+
begin
16+
if InProductionEnvironment then begin
17+
EDontBreakProduction.Create('DeleteDatabase attempted in production');
18+
end;
19+
20+
Database.DeleteAllImportantRecords;
21+
end;
22+
</pre>
23+
<pre data-diff-id="1" data-diff-type="compliant">
24+
procedure DeleteDatabase;
25+
begin
26+
if InProductionEnvironment then begin
27+
raise EDontBreakProduction.Create('DeleteDatabase attempted in production');
28+
end;
29+
30+
Database.DeleteAllImportantRecords;
31+
end;
32+
</pre>

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

+21
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
{
2+
"title": "Exceptions should be raised",
3+
"type": "BUG",
4+
"status": "ready",
5+
"remediation": {
6+
"func": "Constant/Issue",
7+
"constantCost": "1min"
8+
},
9+
"code": {
10+
"attribute": "COMPLETE",
11+
"impacts": {
12+
"RELIABILITY": "HIGH"
13+
}
14+
},
15+
"tags": [
16+
"suspicious"
17+
],
18+
"defaultSeverity": "Critical",
19+
"scope": "ALL",
20+
"quickfix": "unknown"
21+
}

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

+1
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@
5252
"LowercaseKeyword",
5353
"MathFunctionSingleOverload",
5454
"MemberDeclarationOrder",
55+
"MissingRaise",
5556
"MissingSemicolon",
5657
"MixedNames",
5758
"NilComparison",

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

+182
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,182 @@
1+
/*
2+
* Sonar Delphi Plugin
3+
* Copyright (C) 2024 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.builders.DelphiTestUnitBuilder;
22+
import au.com.integradev.delphi.checks.verifier.CheckVerifier;
23+
import org.junit.jupiter.api.Test;
24+
25+
class MissingRaiseCheckTest {
26+
private DelphiTestUnitBuilder sysUtils() {
27+
return new DelphiTestUnitBuilder()
28+
.unitName("System.SysUtils")
29+
.appendDecl("type")
30+
.appendDecl(" Exception = class(TObject)")
31+
.appendDecl(" public")
32+
.appendDecl(" constructor Create(Text: string);")
33+
.appendDecl(" end;");
34+
}
35+
36+
@Test
37+
void testDiscardedBaseExceptionShouldAddIssue() {
38+
CheckVerifier.newVerifier()
39+
.withCheck(new MissingRaiseCheck())
40+
.withStandardLibraryUnit(sysUtils())
41+
.onFile(
42+
new DelphiTestUnitBuilder()
43+
.appendImpl("uses System.SysUtils;")
44+
.appendImpl("initialization")
45+
.appendImpl(" Exception.Create('Error!'); // Noncompliant"))
46+
.verifyIssues();
47+
}
48+
49+
@Test
50+
void testDiscardedDescendantExceptionShouldAddIssue() {
51+
CheckVerifier.newVerifier()
52+
.withCheck(new MissingRaiseCheck())
53+
.withStandardLibraryUnit(sysUtils())
54+
.onFile(
55+
new DelphiTestUnitBuilder()
56+
.appendDecl("uses System.SysUtils;")
57+
.appendDecl("type")
58+
.appendDecl(" ECalculatorError = class(Exception)")
59+
.appendDecl(" public")
60+
.appendDecl(" constructor Create(A: Integer; B: Integer);")
61+
.appendDecl(" end;")
62+
.appendImpl("initialization")
63+
.appendImpl(" ECalculatorError.Create(1, 2); // Noncompliant"))
64+
.verifyIssues();
65+
}
66+
67+
@Test
68+
void testDiscardedNonExceptionShouldNotAddIssue() {
69+
CheckVerifier.newVerifier()
70+
.withCheck(new MissingRaiseCheck())
71+
.withStandardLibraryUnit(sysUtils())
72+
.onFile(
73+
new DelphiTestUnitBuilder()
74+
.appendDecl("uses System.SysUtils;")
75+
.appendDecl("type")
76+
.appendDecl(" EConfusinglyNamedNormalObject = class(TObject)")
77+
.appendDecl(" public")
78+
.appendDecl(" constructor Create(A: Integer; B: Integer);")
79+
.appendDecl(" end;")
80+
.appendImpl("initialization")
81+
.appendImpl(" EConfusinglyNamedNormalObject.Create(1, 2);"))
82+
.verifyNoIssues();
83+
}
84+
85+
@Test
86+
void testRaisedBaseExceptionShouldNotAddIssue() {
87+
CheckVerifier.newVerifier()
88+
.withCheck(new MissingRaiseCheck())
89+
.withStandardLibraryUnit(sysUtils())
90+
.onFile(
91+
new DelphiTestUnitBuilder()
92+
.appendImpl("uses System.SysUtils;")
93+
.appendImpl("initialization")
94+
.appendImpl(" raise Exception.Create('Error!');"))
95+
.verifyNoIssues();
96+
}
97+
98+
@Test
99+
void testDiscardedInvocationShouldNotAddIssue() {
100+
CheckVerifier.newVerifier()
101+
.withCheck(new MissingRaiseCheck())
102+
.withStandardLibraryUnit(sysUtils())
103+
.onFile(
104+
new DelphiTestUnitBuilder()
105+
.appendImpl("uses System.SysUtils;")
106+
.appendImpl("function Foo: Exception;")
107+
.appendImpl("begin")
108+
.appendImpl("end;")
109+
.appendImpl("initialization")
110+
.appendImpl(" Foo;"))
111+
.verifyNoIssues();
112+
}
113+
114+
@Test
115+
void testDiscardedNonConstructorInvocationShouldNotAddIssue() {
116+
CheckVerifier.newVerifier()
117+
.withCheck(new MissingRaiseCheck())
118+
.withStandardLibraryUnit(sysUtils())
119+
.onFile(
120+
new DelphiTestUnitBuilder()
121+
.appendDecl("uses System.SysUtils;")
122+
.appendDecl("type")
123+
.appendDecl(" ECalculatorError = class(Exception)")
124+
.appendDecl(" public")
125+
.appendDecl(" class function Add(A: Integer; B: Integer);")
126+
.appendDecl(" end;")
127+
.appendImpl("initialization")
128+
.appendImpl(" ECalculatorError.Add(1, 2);"))
129+
.verifyNoIssues();
130+
}
131+
132+
@Test
133+
void testRaisedDescendantExceptionShouldNotAddIssue() {
134+
CheckVerifier.newVerifier()
135+
.withCheck(new MissingRaiseCheck())
136+
.withStandardLibraryUnit(sysUtils())
137+
.onFile(
138+
new DelphiTestUnitBuilder()
139+
.appendDecl("uses System.SysUtils;")
140+
.appendDecl("type")
141+
.appendDecl(" ECalculatorError = class(Exception)")
142+
.appendDecl(" public")
143+
.appendDecl(" constructor Create(A: Integer; B: Integer);")
144+
.appendDecl(" end;")
145+
.appendImpl("initialization")
146+
.appendImpl(" raise ECalculatorError.Create(1, 2);"))
147+
.verifyNoIssues();
148+
}
149+
150+
@Test
151+
void testAssignedBaseExceptionShouldNotAddIssue() {
152+
CheckVerifier.newVerifier()
153+
.withCheck(new MissingRaiseCheck())
154+
.withStandardLibraryUnit(sysUtils())
155+
.onFile(
156+
new DelphiTestUnitBuilder()
157+
.appendImpl("uses System.SysUtils;")
158+
.appendImpl("var MyError: ECalculatorError;")
159+
.appendImpl("initialization")
160+
.appendImpl(" MyError := Exception.Create('Error!');"))
161+
.verifyNoIssues();
162+
}
163+
164+
@Test
165+
void testAssignedDescendantExceptionShouldNotAddIssue() {
166+
CheckVerifier.newVerifier()
167+
.withCheck(new MissingRaiseCheck())
168+
.withStandardLibraryUnit(sysUtils())
169+
.onFile(
170+
new DelphiTestUnitBuilder()
171+
.appendDecl("uses System.SysUtils;")
172+
.appendDecl("type")
173+
.appendDecl(" ECalculatorError = class(Exception)")
174+
.appendDecl(" public")
175+
.appendDecl(" constructor Create(A: Integer; B: Integer);")
176+
.appendDecl(" end;")
177+
.appendImpl("var MyError: ECalculatorError;")
178+
.appendImpl("initialization")
179+
.appendImpl(" MyError := ECalculatorError.Create(1, 2);"))
180+
.verifyNoIssues();
181+
}
182+
}

0 commit comments

Comments
 (0)