diff --git a/CHANGELOG.md b/CHANGELOG.md index fd0ff9f41..67318357a 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/delphi-checks/src/main/java/au/com/integradev/delphi/checks/CheckList.java b/delphi-checks/src/main/java/au/com/integradev/delphi/checks/CheckList.java index 720fee091..1d9de99ac 100644 --- a/delphi-checks/src/main/java/au/com/integradev/delphi/checks/CheckList.java +++ b/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, diff --git a/delphi-checks/src/main/java/au/com/integradev/delphi/checks/MissingRaiseCheck.java b/delphi-checks/src/main/java/au/com/integradev/delphi/checks/MissingRaiseCheck.java new file mode 100644 index 000000000..46c5ba3ac --- /dev/null +++ b/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); + } +} diff --git a/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/MissingRaise.html b/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/MissingRaise.html new file mode 100644 index 000000000..538413156 --- /dev/null +++ b/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/MissingRaise.html @@ -0,0 +1,32 @@ +

Why is this an issue?

+

+ When writing an exception raise in Delphi, a common mistake is to omit the raise, + causing the exception to be created but not actually used. This is bad for a number of reasons: +

+ +

How to fix it

+

Add the raise keyword. If the exception is not required, delete the constructor invocation instead.

+
+procedure DeleteDatabase;
+begin
+  if InProductionEnvironment then begin
+    EDontBreakProduction.Create('DeleteDatabase attempted in production');
+  end;
+
+  Database.DeleteAllImportantRecords;
+end;
+
+
+procedure DeleteDatabase;
+begin
+  if InProductionEnvironment then begin
+    raise EDontBreakProduction.Create('DeleteDatabase attempted in production');
+  end;
+
+  Database.DeleteAllImportantRecords;
+end;
+
\ No newline at end of file diff --git a/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/MissingRaise.json b/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/MissingRaise.json new file mode 100644 index 000000000..7513788a8 --- /dev/null +++ b/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" +} diff --git a/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/Sonar_way_profile.json b/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/Sonar_way_profile.json index 5a35d3911..6c31c4c6a 100644 --- a/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/Sonar_way_profile.json +++ b/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", diff --git a/delphi-checks/src/test/java/au/com/integradev/delphi/checks/MissingRaiseCheckTest.java b/delphi-checks/src/test/java/au/com/integradev/delphi/checks/MissingRaiseCheckTest.java new file mode 100644 index 000000000..61544be94 --- /dev/null +++ b/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(); + } +}