Ignore all blocks with comments in EmptyBlock

Author: fourls

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

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);
       }
     }
 

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

@@ -5,7 +5,7 @@ <h2>Why is this an issue?</h2>
 </p>
 <h3>Exceptions</h3>
 <ul>
+  <li>Empty blocks with an explanatory comment</li>
   <li>Empty routine bodies (covered by the <code>EmptyRoutine</code> rule)</li>
   <li>Empty except blocks (covered by the <code>SwallowedException</code> rule)</li>
-  <li>Case blocks that are empty apart from a comment</li>
 </ul>

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();
+  }
 }