Add check for discarded exceptions

CHANGELOG.md

@@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
+- `MissingRaise` analysis rule, which flags exception constructions for which `raise` has been omitted.
 - **API:** `ProceduralType::directives` method.
 - **API:** `ProceduralTypeNode::getDirectives` method.
 - **API:** `ProceduralTypeNode::hasDirective` method.
@@ -31,7 +32,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
-- `FullyQualifiedImportCheck` analysis rule, which flags non-fully qualified imports.
+- `FullyQualifiedImport` analysis rule, which flags non-fully qualified imports.
 - **API:** `UnitImportNameDeclaration::isAlias` method.
 
 ### Changed

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

@@ -107,6 +107,7 @@ public final class CheckList {
           LowercaseKeywordCheck.class,
           MathFunctionSingleOverloadCheck.class,
           MemberDeclarationOrderCheck.class,
+          MissingRaiseCheck.class,
           MissingSemicolonCheck.class,
           MixedNamesCheck.class,
           NilComparisonCheck.class,

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

@@ -0,0 +1,84 @@
+/*
+ * Sonar Delphi Plugin
+ * Copyright (C) 2024 Integrated Application Development
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02
+ */
+package au.com.integradev.delphi.checks;
+
+import org.sonar.check.Rule;
+import org.sonar.plugins.communitydelphi.api.ast.ExpressionNode;
+import org.sonar.plugins.communitydelphi.api.ast.ExpressionStatementNode;
+import org.sonar.plugins.communitydelphi.api.ast.NameReferenceNode;
+import org.sonar.plugins.communitydelphi.api.ast.PrimaryExpressionNode;
+import org.sonar.plugins.communitydelphi.api.check.DelphiCheck;
+import org.sonar.plugins.communitydelphi.api.check.DelphiCheckContext;
+import org.sonar.plugins.communitydelphi.api.reporting.QuickFix;
+import org.sonar.plugins.communitydelphi.api.reporting.QuickFixEdit;
+import org.sonar.plugins.communitydelphi.api.symbol.declaration.NameDeclaration;
+import org.sonar.plugins.communitydelphi.api.symbol.declaration.RoutineKind;
+import org.sonar.plugins.communitydelphi.api.symbol.declaration.RoutineNameDeclaration;
+import org.sonar.plugins.communitydelphi.api.symbol.declaration.TypeNameDeclaration;
+import org.sonar.plugins.communitydelphi.api.type.Type;
+
+@Rule(key = "MissingRaise")
+public class MissingRaiseCheck extends DelphiCheck {
+  private static final String BASE_EXCEPTION = "System.SysUtils.Exception";
+
+  @Override
+  public DelphiCheckContext visit(
+      ExpressionStatementNode expressionStatement, DelphiCheckContext context) {
+    if (isExceptionConstructorInvocation(expressionStatement.getExpression())) {
+      context
+          .newIssue()
+          .onNode(expressionStatement)
+          .withMessage("Raise or delete this exception.")
+          .withQuickFixes(
+              QuickFix.newFix("Raise exception")
+                  .withEdit(
+                      QuickFixEdit.insertBefore("raise ", expressionStatement.getExpression())))
+          .report();
+    }
+
+    return context;
+  }
+
+  private boolean isExceptionConstructorInvocation(ExpressionNode expression) {
+    if (!(expression instanceof PrimaryExpressionNode)
+        || !(expression.getChild(0) instanceof NameReferenceNode)) {
+      return false;
+    }
+
+    NameDeclaration declaration =
+        ((NameReferenceNode) expression.getChild(0)).getLastName().getNameDeclaration();
+    if (!(declaration instanceof RoutineNameDeclaration)) {
+      return false;
+    }
+
+    RoutineNameDeclaration routineDeclaration = (RoutineNameDeclaration) declaration;
+    if (routineDeclaration.getRoutineKind() != RoutineKind.CONSTRUCTOR) {
+      return false;
+    }
+
+    TypeNameDeclaration typeDecl = routineDeclaration.getTypeDeclaration();
+    if (typeDecl == null) {
+      return false;
+    }
+
+    Type type = typeDecl.getType();
+
+    return type.is(BASE_EXCEPTION) || type.isDescendantOf(BASE_EXCEPTION);
+  }
+}

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

@@ -0,0 +1,32 @@
+<h2>Why is this an issue?</h2>
+<p>
+  When writing an exception raise in Delphi, a common mistake is to omit the <code>raise</code>,
+  causing the exception to be created but not actually used. This is bad for a number of reasons:
+</p>
+<ul>
+  <li>The exception object is never freed, causing a memory leak.</li>
+  <li>Control flow continues as usual, even though an undesired exceptional state has been reached.</li>
+  <li>The bug is easy to miss but can greatly alter the path of execution.</li>
+</ul>
+<h2>How to fix it</h2>
+<p>Add the <code>raise</code> keyword. If the exception is not required, delete the constructor invocation instead.</p>
+<pre data-diff-id="1" data-diff-type="noncompliant">
+procedure DeleteDatabase;
+begin
+  if InProductionEnvironment then begin
+    EDontBreakProduction.Create('DeleteDatabase attempted in production');
+  end;
+
+  Database.DeleteAllImportantRecords;
+end;
+</pre>
+<pre data-diff-id="1" data-diff-type="compliant">
+procedure DeleteDatabase;
+begin
+  if InProductionEnvironment then begin
+    raise EDontBreakProduction.Create('DeleteDatabase attempted in production');
+  end;
+
+  Database.DeleteAllImportantRecords;
+end;
+</pre>

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

@@ -0,0 +1,21 @@
+{
+  "title": "Exceptions should be raised",
+  "type": "BUG",
+  "status": "ready",
+  "remediation": {
+    "func": "Constant/Issue",
+    "constantCost": "1min"
+  },
+  "code": {
+    "attribute": "COMPLETE",
+    "impacts": {
+      "RELIABILITY": "HIGH"
+    }
+  },
+  "tags": [
+    "suspicious"
+  ],
+  "defaultSeverity": "Critical",
+  "scope": "ALL",
+  "quickfix": "unknown"
+}

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

@@ -52,6 +52,7 @@
     "LowercaseKeyword",
     "MathFunctionSingleOverload",
     "MemberDeclarationOrder",
+    "MissingRaise",
     "MissingSemicolon",
     "MixedNames",
     "NilComparison",

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

@@ -0,0 +1,182 @@
+/*
+ * Sonar Delphi Plugin
+ * Copyright (C) 2024 Integrated Application Development
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02
+ */
+package au.com.integradev.delphi.checks;
+
+import au.com.integradev.delphi.builders.DelphiTestUnitBuilder;
+import au.com.integradev.delphi.checks.verifier.CheckVerifier;
+import org.junit.jupiter.api.Test;
+
+class MissingRaiseCheckTest {
+  private DelphiTestUnitBuilder sysUtils() {
+    return new DelphiTestUnitBuilder()
+        .unitName("System.SysUtils")
+        .appendDecl("type")
+        .appendDecl("  Exception = class(TObject)")
+        .appendDecl("  public")
+        .appendDecl("    constructor Create(Text: string);")
+        .appendDecl("  end;");
+  }
+
+  @Test
+  void testDiscardedBaseExceptionShouldAddIssue() {
+    CheckVerifier.newVerifier()
+        .withCheck(new MissingRaiseCheck())
+        .withStandardLibraryUnit(sysUtils())
+        .onFile(
+            new DelphiTestUnitBuilder()
+                .appendImpl("uses System.SysUtils;")
+                .appendImpl("initialization")
+                .appendImpl("  Exception.Create('Error!'); // Noncompliant"))
+        .verifyIssues();
+  }
+
+  @Test
+  void testDiscardedDescendantExceptionShouldAddIssue() {
+    CheckVerifier.newVerifier()
+        .withCheck(new MissingRaiseCheck())
+        .withStandardLibraryUnit(sysUtils())
+        .onFile(
+            new DelphiTestUnitBuilder()
+                .appendDecl("uses System.SysUtils;")
+                .appendDecl("type")
+                .appendDecl("  ECalculatorError = class(Exception)")
+                .appendDecl("  public")
+                .appendDecl("    constructor Create(A: Integer; B: Integer);")
+                .appendDecl("  end;")
+                .appendImpl("initialization")
+                .appendImpl("  ECalculatorError.Create(1, 2); // Noncompliant"))
+        .verifyIssues();
+  }
+
+  @Test
+  void testDiscardedNonExceptionShouldNotAddIssue() {
+    CheckVerifier.newVerifier()
+        .withCheck(new MissingRaiseCheck())
+        .withStandardLibraryUnit(sysUtils())
+        .onFile(
+            new DelphiTestUnitBuilder()
+                .appendDecl("uses System.SysUtils;")
+                .appendDecl("type")
+                .appendDecl("  EConfusinglyNamedNormalObject = class(TObject)")
+                .appendDecl("  public")
+                .appendDecl("    constructor Create(A: Integer; B: Integer);")
+                .appendDecl("  end;")
+                .appendImpl("initialization")
+                .appendImpl("  EConfusinglyNamedNormalObject.Create(1, 2);"))
+        .verifyNoIssues();
+  }
+
+  @Test
+  void testRaisedBaseExceptionShouldNotAddIssue() {
+    CheckVerifier.newVerifier()
+        .withCheck(new MissingRaiseCheck())
+        .withStandardLibraryUnit(sysUtils())
+        .onFile(
+            new DelphiTestUnitBuilder()
+                .appendImpl("uses System.SysUtils;")
+                .appendImpl("initialization")
+                .appendImpl("  raise Exception.Create('Error!');"))
+        .verifyNoIssues();
+  }
+
+  @Test
+  void testDiscardedInvocationShouldNotAddIssue() {
+    CheckVerifier.newVerifier()
+        .withCheck(new MissingRaiseCheck())
+        .withStandardLibraryUnit(sysUtils())
+        .onFile(
+            new DelphiTestUnitBuilder()
+                .appendImpl("uses System.SysUtils;")
+                .appendImpl("function Foo: Exception;")
+                .appendImpl("begin")
+                .appendImpl("end;")
+                .appendImpl("initialization")
+                .appendImpl("  Foo;"))
+        .verifyNoIssues();
+  }
+
+  @Test
+  void testDiscardedNonConstructorInvocationShouldNotAddIssue() {
+    CheckVerifier.newVerifier()
+        .withCheck(new MissingRaiseCheck())
+        .withStandardLibraryUnit(sysUtils())
+        .onFile(
+            new DelphiTestUnitBuilder()
+                .appendDecl("uses System.SysUtils;")
+                .appendDecl("type")
+                .appendDecl("  ECalculatorError = class(Exception)")
+                .appendDecl("  public")
+                .appendDecl("    class function Add(A: Integer; B: Integer);")
+                .appendDecl("  end;")
+                .appendImpl("initialization")
+                .appendImpl("  ECalculatorError.Add(1, 2);"))
+        .verifyNoIssues();
+  }
+
+  @Test
+  void testRaisedDescendantExceptionShouldNotAddIssue() {
+    CheckVerifier.newVerifier()
+        .withCheck(new MissingRaiseCheck())
+        .withStandardLibraryUnit(sysUtils())
+        .onFile(
+            new DelphiTestUnitBuilder()
+                .appendDecl("uses System.SysUtils;")
+                .appendDecl("type")
+                .appendDecl("  ECalculatorError = class(Exception)")
+                .appendDecl("  public")
+                .appendDecl("    constructor Create(A: Integer; B: Integer);")
+                .appendDecl("  end;")
+                .appendImpl("initialization")
+                .appendImpl("  raise ECalculatorError.Create(1, 2);"))
+        .verifyNoIssues();
+  }
+
+  @Test
+  void testAssignedBaseExceptionShouldNotAddIssue() {
+    CheckVerifier.newVerifier()
+        .withCheck(new MissingRaiseCheck())
+        .withStandardLibraryUnit(sysUtils())
+        .onFile(
+            new DelphiTestUnitBuilder()
+                .appendImpl("uses System.SysUtils;")
+                .appendImpl("var MyError: ECalculatorError;")
+                .appendImpl("initialization")
+                .appendImpl("  MyError := Exception.Create('Error!');"))
+        .verifyNoIssues();
+  }
+
+  @Test
+  void testAssignedDescendantExceptionShouldNotAddIssue() {
+    CheckVerifier.newVerifier()
+        .withCheck(new MissingRaiseCheck())
+        .withStandardLibraryUnit(sysUtils())
+        .onFile(
+            new DelphiTestUnitBuilder()
+                .appendDecl("uses System.SysUtils;")
+                .appendDecl("type")
+                .appendDecl("  ECalculatorError = class(Exception)")
+                .appendDecl("  public")
+                .appendDecl("    constructor Create(A: Integer; B: Integer);")
+                .appendDecl("  end;")
+                .appendImpl("var MyError: ECalculatorError;")
+                .appendImpl("initialization")
+                .appendImpl("  MyError := ECalculatorError.Create(1, 2);"))
+        .verifyNoIssues();
+  }
+}