diff --git a/CHANGELOG.md b/CHANGELOG.md index 78f638e11..120fc5336 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- `EmptyBlock` now ignores all empty blocks containing an explanatory comment. + ## [1.17.2] - 2025-07-03 ### Fixed diff --git a/delphi-checks/src/main/java/au/com/integradev/delphi/checks/EmptyBlockCheck.java b/delphi-checks/src/main/java/au/com/integradev/delphi/checks/EmptyBlockCheck.java index 2cb368d47..910285968 100644 --- a/delphi-checks/src/main/java/au/com/integradev/delphi/checks/EmptyBlockCheck.java +++ b/delphi-checks/src/main/java/au/com/integradev/delphi/checks/EmptyBlockCheck.java @@ -20,8 +20,6 @@ import org.sonar.check.Rule; import org.sonar.plugins.communitydelphi.api.ast.AnonymousMethodNode; -import org.sonar.plugins.communitydelphi.api.ast.CaseItemStatementNode; -import org.sonar.plugins.communitydelphi.api.ast.CaseStatementNode; import org.sonar.plugins.communitydelphi.api.ast.CompoundStatementNode; import org.sonar.plugins.communitydelphi.api.ast.DelphiNode; import org.sonar.plugins.communitydelphi.api.ast.ElseBlockNode; @@ -50,37 +48,31 @@ public DelphiCheckContext visit(CompoundStatementNode block, DelphiCheckContext private static boolean shouldAddViolation(CompoundStatementNode block) { DelphiNode parent = block.getParent(); - if (parent instanceof RoutineBodyNode || parent instanceof AnonymousMethodNode) { - // Handled by EmptyRoutineRule + if (!block.getComments().isEmpty()) { + // An empty block is OK if it has an explanatory comment. return false; } - if (parent instanceof ExceptItemNode) { - // Handled by SwallowedExceptionsRule + if (parent instanceof RoutineBodyNode || parent instanceof AnonymousMethodNode) { + // Handled by EmptyRoutine return false; } - if (parent instanceof CaseItemStatementNode) { - // Handling all cases in a case statement is a reasonable thing to do. - // With that being said, a comment is required. - return block.getComments().isEmpty(); + if (parent instanceof ExceptItemNode) { + // Handled by SwallowedException + return false; } if (parent instanceof StatementListNode) { StatementListNode statementList = (StatementListNode) parent; - DelphiNode grandparent = parent.getParent(); + DelphiNode enclosing = statementList.getParent(); if (statementList.getStatements().size() == 1) { - if (grandparent instanceof ElseBlockNode - && grandparent.getParent() instanceof CaseStatementNode) { - // Handling all cases in a case statement is a reasonable thing to do. - // With that being said, a comment is required. - return block.getComments().isEmpty(); + if (enclosing instanceof ElseBlockNode) { + enclosing = enclosing.getParent(); } - - // Handled by SwallowedExceptionsRule - return !(grandparent instanceof ElseBlockNode) - || !(grandparent.getParent() instanceof ExceptBlockNode); + // Handled by SwallowedException + return !(enclosing instanceof ExceptBlockNode); } } diff --git a/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/EmptyBlock.html b/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/EmptyBlock.html index c71182fa1..f9438300e 100644 --- a/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/EmptyBlock.html +++ b/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/EmptyBlock.html @@ -5,7 +5,7 @@

Why is this an issue?

Exceptions

\ No newline at end of file diff --git a/delphi-checks/src/test/java/au/com/integradev/delphi/checks/EmptyBlockCheckTest.java b/delphi-checks/src/test/java/au/com/integradev/delphi/checks/EmptyBlockCheckTest.java index 14e39141c..816d2d513 100644 --- a/delphi-checks/src/test/java/au/com/integradev/delphi/checks/EmptyBlockCheckTest.java +++ b/delphi-checks/src/test/java/au/com/integradev/delphi/checks/EmptyBlockCheckTest.java @@ -84,8 +84,8 @@ void testEmptyBeginStatementsShouldAddIssue() { .appendImpl("end;") .appendImpl("procedure TEmptyProcs.Three;") .appendImpl("begin") - .appendImpl(" if Foo then begin // Noncompliant") - .appendImpl(" // Do nothing") + .appendImpl(" if Foo then begin") + .appendImpl(" // This exists for X reason") .appendImpl(" end;") .appendImpl("end;") .appendImpl("procedure GlobalProcedureFour;") @@ -94,8 +94,11 @@ void testEmptyBeginStatementsShouldAddIssue() { .appendImpl("end;") .appendImpl("procedure GlobalProcedureFive;") .appendImpl("begin") - .appendImpl(" if Foo then begin // Noncompliant") - .appendImpl(" // Do nothing") + .appendImpl(" // Noncompliant@+1") + .appendImpl(" if Foo then begin") + .appendImpl(" end") + .appendImpl(" // Noncompliant@+1") + .appendImpl(" else begin") .appendImpl(" end;") .appendImpl("end;")) .verifyIssues(); @@ -122,7 +125,7 @@ void testEmptyBlocksInCaseStatementShouldAddIssue() { } @Test - void testEmptyBlocksInCaseStatementShouldNotAddIssue() { + void testNonEmptyBlocksInCaseStatementShouldNotAddIssue() { CheckVerifier.newVerifier() .withCheck(new EmptyBlockCheck()) .onFile( @@ -177,4 +180,51 @@ void testEmptyAnonymousMethodShouldNotAddIssue() { .appendImpl("end;")) .verifyNoIssues(); } + + @Test + void testEmptyExceptShouldNotAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new EmptyBlockCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendImpl("procedure Foo;") + .appendImpl("begin") + .appendImpl(" try") + .appendImpl(" // Do risky stuff") + .appendImpl(" except") + .appendImpl(" end;") + .appendImpl(" try") + .appendImpl(" // Do risky stuff") + .appendImpl(" except") + .appendImpl(" on E: Exception do begin") + .appendImpl(" end;") + .appendImpl(" else begin") + .appendImpl(" end;") + .appendImpl(" end;") + .appendImpl("end;")) + .verifyNoIssues(); + } + + @Test + void testNestedEmptyElsesShouldAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new EmptyBlockCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendImpl("procedure Foo;") + .appendImpl("begin") + .appendImpl(" // Noncompliant@+1") + .appendImpl(" if Bar then begin") + .appendImpl(" end") + .appendImpl(" else begin") + .appendImpl(" // Noncompliant@+1") + .appendImpl(" if Baz then begin") + .appendImpl(" end") + .appendImpl(" // Noncompliant@+1") + .appendImpl(" else begin") + .appendImpl(" end;") + .appendImpl(" end;") + .appendImpl("end;")) + .verifyIssues(); + } }