Add check for array iterations with out of range high bounds

Author: fourls

CHANGELOG.md

@@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - Support for MSBuild item and item metadata expressions in project files.
 - `ExhaustiveEnumCase` analysis rule, which flags `case` statements that do not handle all values in an enumeration.
+- `IterationPastHighBound` analysis rule, which flags `for` loops that iterate past the end of the collection.
 - **API:** `EnumeratorOccurrence` type.
 - **API:** `ForInStatementNode::getEnumeratorOccurrence` method.
 - **API:** `TypeOfTypeNode::getTypeReferenceNode` method.

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

@@ -104,6 +104,7 @@ public final class CheckList {
           InstanceInvokedConstructorCheck.class,
           InterfaceGuidCheck.class,
           InterfaceNameCheck.class,
+          IterationPastHighBoundCheck.class,
           LegacyInitializationSectionCheck.class,
           LoopExecutingAtMostOnceCheck.class,
           LowercaseKeywordCheck.class,

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

@@ -0,0 +1,98 @@
+/*
+ * 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.checks;
+
+import org.sonar.check.Rule;
+import org.sonar.plugins.communitydelphi.api.ast.ExpressionNode;
+import org.sonar.plugins.communitydelphi.api.ast.ForToStatementNode;
+import org.sonar.plugins.communitydelphi.api.ast.IntegerLiteralNode;
+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.RoutineNameDeclaration;
+import org.sonar.plugins.communitydelphi.api.symbol.declaration.TypedDeclaration;
+
+@Rule(key = "IterationPastHighBound")
+public class IterationPastHighBoundCheck extends DelphiCheck {
+  @Override
+  public DelphiCheckContext visit(ForToStatementNode forStatement, DelphiCheckContext context) {
+    var target = forStatement.getTargetExpression().skipParentheses();
+    var initializer = forStatement.getInitializerExpression().skipParentheses();
+
+    if (isZero(initializer) && isSuspiciousTargetExpression(target)) {
+      context
+          .newIssue()
+          .onNode(target)
+          .withMessage("Ensure this for loop is not iterating beyond the end of the collection.")
+          .withQuickFixes(
+              QuickFix.newFix("Subtract one from expression")
+                  .withEdit(QuickFixEdit.insertAfter(" - 1", target)))
+          .report();
+    }
+
+    return super.visit(forStatement, context);
+  }
+
+  private boolean isZero(ExpressionNode expression) {
+    if (!(expression instanceof PrimaryExpressionNode)) {
+      return false;
+    }
+
+    var child = expression.getChild(0);
+    return child instanceof IntegerLiteralNode
+        && ((IntegerLiteralNode) child).getValue().intValue() == 0;
+  }
+
+  private boolean isSuspiciousTargetExpression(ExpressionNode expression) {
+    if (!(expression instanceof PrimaryExpressionNode)) {
+      return false;
+    }
+
+    var reference = expression.getChild(0);
+    if (!(reference instanceof NameReferenceNode)) {
+      return false;
+    }
+
+    var declaration = ((NameReferenceNode) reference).getLastName().getNameDeclaration();
+    return isArrayLength(declaration) || isCount(declaration);
+  }
+
+  private boolean isCount(NameDeclaration declaration) {
+    if (!(declaration instanceof TypedDeclaration)) {
+      return false;
+    }
+
+    return declaration.getName().equalsIgnoreCase("Count")
+        && ((TypedDeclaration) declaration).getType().isInteger();
+  }
+
+  private boolean isArrayLength(NameDeclaration declaration) {
+    if (!(declaration instanceof RoutineNameDeclaration)) {
+      return false;
+    }
+
+    var routine = (RoutineNameDeclaration) declaration;
+    return routine.fullyQualifiedName().equals("System.Length")
+        && routine.getParameters().get(0).getType().is("<array>");
+  }
+}

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

@@ -0,0 +1,55 @@
+<h2>Why is this an issue?</h2>
+<p>
+  The upper range of <code>for</code> loops is inclusive in Delphi, meaning that a typical
+  zero-based collection iteration should start at 0 and end at one before the array length:
+</p>
+<pre>
+for I := 0 to Length(Array) - 1 do
+  WriteLn(Array[I]);
+</pre>
+<p>
+  Forgetting to subtract one from the length of the collection is a common mistake in Delphi code,
+  especially since in most other languages the convention is for the upper bound to be exclusive.
+  If forgotten, this may cause access violations, buffer overruns, and other memory issues.
+</p>
+<p>
+  When using non zero-based collections, the <code>Low</code> and <code>High</code> intrinsics
+  should be used instead of iterating based on length.
+</p>
+<p>
+  This rule catches suspicious cases of <code>System.Length</code> or <code>Count</code> properties
+  being used as an upper bound. Iterations over strings are excluded as they are typically
+  one-based.
+</p>
+<h2>How to fix it</h2>
+<p>
+  If the collection is zero-based, subtract one from the upper bound:
+</p>
+<pre data-diff-id="1" data-diff-type="noncompliant">
+for I := 0 to Length(Array) do
+  WriteLn(Array[I]);
+</pre>
+<pre data-diff-id="1" data-diff-type="compliant">
+for I := 0 to Length(Array) - 1 do
+  WriteLn(Array[I]);
+</pre>
+<p>
+  If the collection is not zero-based, use the <code>Low</code> and <code>High</code> intrinsics
+  to iterate instead:
+</p>
+<pre data-diff-id="2" data-diff-type="noncompliant">
+for I := 1 to Length(Array) do
+  WriteLn(Array[I]);
+</pre>
+<pre data-diff-id="2" data-diff-type="compliant">
+for I := Low(Array) to High(Array) do
+  WriteLn(Array[I]);
+</pre>
+<h2>Resources</h2>
+<ul>
+  <li>
+    <a href="https://docwiki.embarcadero.com/RADStudio/en/Type_Declarations">
+      Delphi's Object Pascal Style Guide: Type Declarations
+    </a>
+  </li>
+</ul>

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

@@ -0,0 +1,20 @@
+{
+  "title": "Loops should not iterate beyond the end of the collection",
+  "type": "BUG",
+  "status": "ready",
+  "remediation": {
+    "func": "Constant/Issue",
+    "constantCost": "15min"
+  },
+  "code": {
+    "attribute": "LOGICAL",
+    "impacts": {
+      "RELIABILITY": "MEDIUM",
+      "MAINTAINABILITY": "LOW"
+    }
+  },
+  "tags": ["suspicious"],
+  "defaultSeverity": "Critical",
+  "scope": "ALL",
+  "quickfix": "unknown"
+}

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

@@ -48,6 +48,7 @@
     "InlineDeclarationCapturedByAnonymousMethod",
     "InstanceInvokedConstructor",
     "InterfaceName",
+    "IterationPastHighBound",
     "LegacyInitializationSection",
     "LoopExecutingAtMostOnce",
     "LowercaseKeyword",