Improve RedundantJump and fix bugs with control flow graph

CHANGELOG.md

@@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Fixed
+
+- Exceptions from empty structures (e.g., `if`) in `LoopExecutingAtMostOnce` and `RedundantJump`.
+- False positives from case statements in `LoopExecutingAtMostOnce`.
+- False positives from nested finally-except blocks in `RedundantJump`.
+
 ## [1.14.1] - 2025-03-05
 
 ### Fixed

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

@@ -18,13 +18,12 @@
  */
 package au.com.integradev.delphi.checks;
 
-import au.com.integradev.delphi.antlr.ast.node.AnonymousMethodNodeImpl;
-import au.com.integradev.delphi.antlr.ast.node.RoutineImplementationNodeImpl;
 import au.com.integradev.delphi.cfg.ControlFlowGraphFactory;
 import au.com.integradev.delphi.cfg.api.Block;
 import au.com.integradev.delphi.cfg.api.Branch;
 import au.com.integradev.delphi.cfg.api.ControlFlowGraph;
 import au.com.integradev.delphi.cfg.api.Terminated;
+import au.com.integradev.delphi.utils.ControlFlowGraphUtils;
 import java.util.ArrayDeque;
 import java.util.ArrayList;
 import java.util.Deque;
@@ -33,22 +32,16 @@
 import java.util.Optional;
 import java.util.Queue;
 import java.util.Set;
-import java.util.function.Supplier;
 import org.sonar.check.Rule;
-import org.sonar.plugins.communitydelphi.api.ast.CompoundStatementNode;
-import org.sonar.plugins.communitydelphi.api.ast.DelphiAst;
 import org.sonar.plugins.communitydelphi.api.ast.DelphiNode;
-import org.sonar.plugins.communitydelphi.api.ast.FinalizationSectionNode;
 import org.sonar.plugins.communitydelphi.api.ast.ForInStatementNode;
 import org.sonar.plugins.communitydelphi.api.ast.ForStatementNode;
 import org.sonar.plugins.communitydelphi.api.ast.ForToStatementNode;
 import org.sonar.plugins.communitydelphi.api.ast.GotoStatementNode;
 import org.sonar.plugins.communitydelphi.api.ast.IfStatementNode;
-import org.sonar.plugins.communitydelphi.api.ast.InitializationSectionNode;
 import org.sonar.plugins.communitydelphi.api.ast.NameReferenceNode;
 import org.sonar.plugins.communitydelphi.api.ast.RaiseStatementNode;
 import org.sonar.plugins.communitydelphi.api.ast.RepeatStatementNode;
-import org.sonar.plugins.communitydelphi.api.ast.StatementListNode;
 import org.sonar.plugins.communitydelphi.api.ast.StatementNode;
 import org.sonar.plugins.communitydelphi.api.ast.WhileStatementNode;
 import org.sonar.plugins.communitydelphi.api.check.DelphiCheck;
@@ -282,34 +275,11 @@ private static boolean isDescendant(DelphiNode descendant, DelphiNode target) {
     return false;
   }
 
-  private static Supplier<ControlFlowGraph> getCFGSupplier(DelphiNode node) {
-    if (node instanceof RoutineImplementationNodeImpl) {
-      return ((RoutineImplementationNodeImpl) node)::getControlFlowGraph;
-    }
-    if (node instanceof AnonymousMethodNodeImpl) {
-      return ((AnonymousMethodNodeImpl) node)::getControlFlowGraph;
-    }
-    if (node instanceof CompoundStatementNode && node.getParent() instanceof DelphiAst) {
-      return () -> ControlFlowGraphFactory.create((CompoundStatementNode) node);
-    }
-    if (node instanceof StatementListNode
-        && (node.getParent() instanceof InitializationSectionNode
-            || node.getParent() instanceof FinalizationSectionNode)) {
-      return () -> ControlFlowGraphFactory.create((StatementListNode) node);
-    }
-    return null;
-  }
-
   private static ControlFlowGraph getCFG(DelphiNode loop) {
-    DelphiNode parent = loop.getParent();
-    Supplier<ControlFlowGraph> cfgSupplier = getCFGSupplier(parent);
-    while (parent != null && cfgSupplier == null) {
-      parent = parent.getParent();
-      cfgSupplier = getCFGSupplier(parent);
-    }
-    if (cfgSupplier != null) {
-      return cfgSupplier.get();
+    ControlFlowGraph cfg = ControlFlowGraphUtils.findContainingCFG(loop);
+    if (cfg == null) {
+      return ControlFlowGraphFactory.create(loop.findChildrenOfType(StatementNode.class));
     }
-    return ControlFlowGraphFactory.create(loop.findChildrenOfType(StatementNode.class));
+    return cfg;
   }
 }

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

@@ -18,18 +18,20 @@
  */
 package au.com.integradev.delphi.checks;
 
-import au.com.integradev.delphi.antlr.ast.node.RoutineImplementationNodeImpl;
-import au.com.integradev.delphi.cfg.ControlFlowGraphFactory;
 import au.com.integradev.delphi.cfg.api.Block;
 import au.com.integradev.delphi.cfg.api.ControlFlowGraph;
 import au.com.integradev.delphi.cfg.api.Finally;
+import au.com.integradev.delphi.cfg.api.Terminated;
 import au.com.integradev.delphi.cfg.api.UnconditionalJump;
+import au.com.integradev.delphi.utils.ControlFlowGraphUtils;
+import java.util.ArrayDeque;
+import java.util.Deque;
 import org.sonar.check.Rule;
-import org.sonar.plugins.communitydelphi.api.ast.AnonymousMethodNode;
 import org.sonar.plugins.communitydelphi.api.ast.ArgumentListNode;
 import org.sonar.plugins.communitydelphi.api.ast.DelphiNode;
+import org.sonar.plugins.communitydelphi.api.ast.FinallyBlockNode;
 import org.sonar.plugins.communitydelphi.api.ast.NameReferenceNode;
-import org.sonar.plugins.communitydelphi.api.ast.RoutineImplementationNode;
+import org.sonar.plugins.communitydelphi.api.ast.TryStatementNode;
 import org.sonar.plugins.communitydelphi.api.check.DelphiCheck;
 import org.sonar.plugins.communitydelphi.api.check.DelphiCheckContext;
 import org.sonar.plugins.communitydelphi.api.symbol.declaration.NameDeclaration;
@@ -39,103 +41,115 @@
 public class RedundantJumpCheck extends DelphiCheck {
   private static final String MESSAGE = "Remove this redundant jump.";
 
+  private final Deque<TryStatementNode> tryFinallyStatements = new ArrayDeque<>();
+
   @Override
-  public DelphiCheckContext visit(RoutineImplementationNode routine, DelphiCheckContext context) {
-    ControlFlowGraph cfg = ((RoutineImplementationNodeImpl) routine).getControlFlowGraph();
-    if (cfg != null) {
-      cfg.getBlocks().forEach(block -> checkBlock(block, context));
+  public DelphiCheckContext visit(TryStatementNode node, DelphiCheckContext data) {
+    boolean finallyBlock = node.hasFinallyBlock();
+    if (!finallyBlock) {
+      return super.visit(node, data);
     }
-
-    return super.visit(routine, context);
+    this.tryFinallyStatements.push(node);
+    super.visit(node.getStatementList(), data);
+    this.tryFinallyStatements.pop();
+    return super.visit(node.getFinallyBlock(), data);
   }
 
   @Override
-  public DelphiCheckContext visit(AnonymousMethodNode routine, DelphiCheckContext context) {
-    ControlFlowGraph cfg = ControlFlowGraphFactory.create(routine.getStatementBlock());
-    cfg.getBlocks().forEach(block -> checkBlock(block, context));
+  public DelphiCheckContext visit(NameReferenceNode node, DelphiCheckContext data) {
+    RoutineNameDeclaration routineNameDeclaration = getRoutineNameDeclaration(node);
+    if (routineNameDeclaration != null
+        && (isContinue(routineNameDeclaration)
+            || isExitWithoutArgs(routineNameDeclaration, node))) {
+      checkJumpNode(node, data);
+      return data;
+    }
+    return super.visit(node, data);
+  }
 
-    return super.visit(routine, context);
+  private static RoutineNameDeclaration getRoutineNameDeclaration(NameReferenceNode node) {
+    NameDeclaration nameDeclaration = node.getNameDeclaration();
+    if (nameDeclaration instanceof RoutineNameDeclaration) {
+      return (RoutineNameDeclaration) nameDeclaration;
+    }
+    return null;
   }
 
-  private void checkBlock(Block block, DelphiCheckContext context) {
-    if (!(block instanceof UnconditionalJump)) {
-      return;
+  private static boolean isContinue(RoutineNameDeclaration node) {
+    String name = node.fullyQualifiedName();
+    return name.equals("System.Continue");
+  }
+
+  private static boolean isExitWithoutArgs(RoutineNameDeclaration node, DelphiNode statement) {
+    String name = node.fullyQualifiedName();
+    if (!name.equals("System.Exit")) {
+      return false;
     }
 
-    UnconditionalJump jump = (UnconditionalJump) block;
-    DelphiNode terminator = jump.getTerminator();
+    var argumentList = statement.getParent().getFirstChildOfType(ArgumentListNode.class);
+    int arguments = argumentList == null ? 0 : argumentList.getArgumentNodes().size();
+
+    return arguments == 0;
+  }
+
+  private static Block findBlockWithTerminator(ControlFlowGraph cfg, DelphiNode node) {
+    for (Block block : cfg.getBlocks()) {
+      if ((block instanceof Terminated) && ((Terminated) block).getTerminator() == node) {
+        return block;
+      }
+    }
+    return null;
+  }
 
-    RoutineNameDeclaration routineNameDeclaration = getRoutineNameDeclaration(terminator);
-    if (routineNameDeclaration == null) {
+  private void checkJumpNode(NameReferenceNode node, DelphiCheckContext context) {
+    ControlFlowGraph cfg = ControlFlowGraphUtils.findContainingCFG(node);
+    if (cfg == null) {
       return;
     }
 
-    if (!isContinueOrExit(routineNameDeclaration)
-        || isExitWithExpression(routineNameDeclaration, terminator)) {
+    Block terminatedBlock = findBlockWithTerminator(cfg, node);
+    if (!(terminatedBlock instanceof UnconditionalJump)) {
+      // can't be a redundant jump without a jump
       return;
     }
 
+    UnconditionalJump jump = (UnconditionalJump) terminatedBlock;
     Block successor = jump.getSuccessor();
     if (!successor.equals(jump.getSuccessorIfRemoved())) {
+      // without the jump, the successor block would be different
       return;
     }
 
-    Block finallyBlock = getFinallyBlock(block);
-    if (finallyBlock != null) {
-      if (onlyFinallyBlocksBeforeEnd(finallyBlock)) {
-        reportIssue(context, terminator, MESSAGE);
-      }
-      return;
+    if (isViolation(cfg)) {
+      reportIssue(context, jump.getTerminator(), MESSAGE);
     }
-
-    reportIssue(context, terminator, MESSAGE);
-  }
-
-  private static Block getFinallyBlock(Block block) {
-    return block.getSuccessors().stream()
-        .filter(Finally.class::isInstance)
-        .findFirst()
-        .orElse(null);
   }
 
-  private static boolean onlyFinallyBlocksBeforeEnd(Block finallyBlock) {
-    while (finallyBlock.getSuccessors().size() == 1) {
-      Block finallySuccessor = finallyBlock.getSuccessors().iterator().next();
-      if (!(finallySuccessor instanceof Finally)) {
+  private boolean isViolation(ControlFlowGraph cfg) {
+    for (TryStatementNode tryFinally : this.tryFinallyStatements) {
+      Finally finallyBlock = findFinallyBlock(cfg, tryFinally.getFinallyBlock());
+      if (finallyBlock == null) {
+        // if the finally block cannot be found, we have traversed outside the scope of the cfg
         break;
       }
-      finallyBlock = finallySuccessor;
+      if (!finallyBlock.getSuccessor().equals(finallyBlock.getExceptionSuccessor())) {
+        // multiple paths after the finally corresponds to code that would be skipped from the jump
+        return false;
+      }
     }
-    return finallyBlock.getSuccessors().size() == 1
-        && finallyBlock.getSuccessors().iterator().next().getSuccessors().isEmpty();
+    // if no invalidating try-finally blocks are found, the use is a violation
+    return true;
   }
 
-  private static RoutineNameDeclaration getRoutineNameDeclaration(DelphiNode node) {
-    NameDeclaration nameDeclaration = null;
-    if (node instanceof NameReferenceNode) {
-      nameDeclaration = ((NameReferenceNode) node).getNameDeclaration();
+  private static Finally findFinallyBlock(ControlFlowGraph cfg, FinallyBlockNode element) {
+    if (element == null) {
+      return null;
     }
-    if (nameDeclaration instanceof RoutineNameDeclaration) {
-      return (RoutineNameDeclaration) nameDeclaration;
+    for (Block block : cfg.getBlocks()) {
+      if ((block instanceof Finally) && ((Finally) block).getTerminator().equals(element)) {
+        return (Finally) block;
+      }
     }
     return null;
   }
-
-  private static boolean isContinueOrExit(RoutineNameDeclaration routineNameDeclaration) {
-    String fullyQualifiedName = routineNameDeclaration.fullyQualifiedName();
-    return fullyQualifiedName.equals("System.Continue") || fullyQualifiedName.equals("System.Exit");
-  }
-
-  private static boolean isExitWithExpression(
-      RoutineNameDeclaration routineNameDeclaration, DelphiNode statement) {
-    String fullyQualifiedName = routineNameDeclaration.fullyQualifiedName();
-    if (!fullyQualifiedName.equals("System.Exit")) {
-      return false;
-    }
-
-    var argumentList = statement.getParent().getFirstChildOfType(ArgumentListNode.class);
-    int arguments = argumentList == null ? 0 : argumentList.getArgumentNodes().size();
-
-    return arguments > 0;
-  }
 }

delphi-checks/src/main/java/au/com/integradev/delphi/utils/ControlFlowGraphUtils.java

@@ -0,0 +1,66 @@
+/*
+ * Sonar Delphi Plugin
+ * Copyright (C) 2025 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.utils;
+
+import au.com.integradev.delphi.antlr.ast.node.AnonymousMethodNodeImpl;
+import au.com.integradev.delphi.antlr.ast.node.RoutineImplementationNodeImpl;
+import au.com.integradev.delphi.cfg.ControlFlowGraphFactory;
+import au.com.integradev.delphi.cfg.api.ControlFlowGraph;
+import java.util.function.Supplier;
+import org.sonar.plugins.communitydelphi.api.ast.CompoundStatementNode;
+import org.sonar.plugins.communitydelphi.api.ast.DelphiAst;
+import org.sonar.plugins.communitydelphi.api.ast.DelphiNode;
+import org.sonar.plugins.communitydelphi.api.ast.FinalizationSectionNode;
+import org.sonar.plugins.communitydelphi.api.ast.InitializationSectionNode;
+import org.sonar.plugins.communitydelphi.api.ast.StatementListNode;
+
+public final class ControlFlowGraphUtils {
+  private ControlFlowGraphUtils() {
+    // Utility class
+  }
+
+  private static Supplier<ControlFlowGraph> getCFGSupplier(DelphiNode node) {
+    if (node instanceof RoutineImplementationNodeImpl) {
+      return ((RoutineImplementationNodeImpl) node)::getControlFlowGraph;
+    }
+    if (node instanceof AnonymousMethodNodeImpl) {
+      return ((AnonymousMethodNodeImpl) node)::getControlFlowGraph;
+    }
+    if (node instanceof CompoundStatementNode && node.getParent() instanceof DelphiAst) {
+      return () -> ControlFlowGraphFactory.create((CompoundStatementNode) node);
+    }
+    if (node instanceof StatementListNode
+        && (node.getParent() instanceof InitializationSectionNode
+            || node.getParent() instanceof FinalizationSectionNode)) {
+      return () -> ControlFlowGraphFactory.create((StatementListNode) node);
+    }
+    return null;
+  }
+
+  public static ControlFlowGraph findContainingCFG(DelphiNode node) {
+    while (node != null) {
+      Supplier<ControlFlowGraph> cfgSupplier = getCFGSupplier(node);
+      if (cfgSupplier != null) {
+        return cfgSupplier.get();
+      }
+      node = node.getParent();
+    }
+    return null;
+  }
+}