diff --git a/CHANGELOG.md b/CHANGELOG.md index ea1ba5444..3594934f9 100644 --- a/CHANGELOG.md +++ b/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. 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 1d30e23c4..9e004aa34 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 @@ -104,6 +104,7 @@ public final class CheckList { InstanceInvokedConstructorCheck.class, InterfaceGuidCheck.class, InterfaceNameCheck.class, + IterationPastHighBoundCheck.class, LegacyInitializationSectionCheck.class, LoopExecutingAtMostOnceCheck.class, LowercaseKeywordCheck.class, diff --git a/delphi-checks/src/main/java/au/com/integradev/delphi/checks/IterationPastHighBoundCheck.java b/delphi-checks/src/main/java/au/com/integradev/delphi/checks/IterationPastHighBoundCheck.java new file mode 100644 index 000000000..d31b41dec --- /dev/null +++ b/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(""); + } +} diff --git a/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/IterationPastHighBound.html b/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/IterationPastHighBound.html new file mode 100644 index 000000000..0a0c5c9b6 --- /dev/null +++ b/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/IterationPastHighBound.html @@ -0,0 +1,55 @@ +

Why is this an issue?

+

+ The upper range of for 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: +

+
+for I := 0 to Length(Array) - 1 do
+  WriteLn(Array[I]);
+
+

+ 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. +

+

+ When using non zero-based collections, the Low and High intrinsics + should be used instead of iterating based on length. +

+

+ This rule catches suspicious cases of System.Length or Count properties + being used as an upper bound. Iterations over strings are excluded as they are typically + one-based. +

+

How to fix it

+

+ If the collection is zero-based, subtract one from the upper bound: +

+
+for I := 0 to Length(Array) do
+  WriteLn(Array[I]);
+
+
+for I := 0 to Length(Array) - 1 do
+  WriteLn(Array[I]);
+
+

+ If the collection is not zero-based, use the Low and High intrinsics + to iterate instead: +

+
+for I := 1 to Length(Array) do
+  WriteLn(Array[I]);
+
+
+for I := Low(Array) to High(Array) do
+  WriteLn(Array[I]);
+
+

Resources

+ \ No newline at end of file diff --git a/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/IterationPastHighBound.json b/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/IterationPastHighBound.json new file mode 100644 index 000000000..422eff399 --- /dev/null +++ b/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" +} 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 246e0380e..d3e6aacf7 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 @@ -48,6 +48,7 @@ "InlineDeclarationCapturedByAnonymousMethod", "InstanceInvokedConstructor", "InterfaceName", + "IterationPastHighBound", "LegacyInitializationSection", "LoopExecutingAtMostOnce", "LowercaseKeyword", diff --git a/delphi-checks/src/test/java/au/com/integradev/delphi/checks/IterationPastHighBoundCheckTest.java b/delphi-checks/src/test/java/au/com/integradev/delphi/checks/IterationPastHighBoundCheckTest.java new file mode 100644 index 000000000..eeb2eb86b --- /dev/null +++ b/delphi-checks/src/test/java/au/com/integradev/delphi/checks/IterationPastHighBoundCheckTest.java @@ -0,0 +1,344 @@ +/* + * 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 au.com.integradev.delphi.builders.DelphiTestUnitBuilder; +import au.com.integradev.delphi.checks.verifier.CheckVerifier; +import org.junit.jupiter.api.Test; + +class IterationPastHighBoundCheckTest { + @Test + void testIterationToLengthMinusOneShouldNotAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new IterationPastHighBoundCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendImpl("procedure Test(MyArr: TArray);") + .appendImpl("var") + .appendImpl(" I: Integer;") + .appendImpl("begin") + .appendImpl(" for I := 0 to Length(MyArr) - 1 do begin") + .appendImpl(" // ...") + .appendImpl(" end;") + .appendImpl("end;")) + .verifyNoIssues(); + } + + @Test + void testIterationToHighShouldNotAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new IterationPastHighBoundCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendImpl("procedure Test(MyArr: TArray);") + .appendImpl("var") + .appendImpl(" I: Integer;") + .appendImpl("begin") + .appendImpl(" for I := 0 to High(MyArr) do begin") + .appendImpl(" // ...") + .appendImpl(" end;") + .appendImpl("end;")) + .verifyNoIssues(); + } + + @Test + void testIterationFromNonZeroShouldNotAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new IterationPastHighBoundCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendImpl("procedure Test(MyArr: TArray);") + .appendImpl("var") + .appendImpl(" I: Integer;") + .appendImpl("begin") + .appendImpl(" for I := 1 to Length(MyArr) do begin") + .appendImpl(" // ...") + .appendImpl(" end;") + .appendImpl("end;")) + .verifyNoIssues(); + } + + @Test + void testIterationFromCustomBoundArrayStartingAtZeroShouldAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new IterationPastHighBoundCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendImpl("type TCustomArray = array[1..6] of string;") + .appendImpl("procedure Test(MyArr: TCustomArray);") + .appendImpl("var") + .appendImpl(" I: Integer;") + .appendImpl("begin") + .appendImpl(" for I := 0 to Length(MyArr) do begin // Noncompliant") + .appendImpl(" // ...") + .appendImpl(" end;") + .appendImpl("end;")) + .verifyIssues(); + } + + @Test + void testIterationFromCustomBoundArrayStartingAtNonZeroShouldNotAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new IterationPastHighBoundCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendImpl("type TCustomArray = array[1..6] of string;") + .appendImpl("procedure Test(MyArr: TCustomArray);") + .appendImpl("var") + .appendImpl(" I: Integer;") + .appendImpl("begin") + .appendImpl(" for I := 1 to Length(MyArr) do begin") + .appendImpl(" // ...") + .appendImpl(" end;") + .appendImpl("end;")) + .verifyNoIssues(); + } + + @Test + void testIterationFromNonZeroInlineVarShouldNotAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new IterationPastHighBoundCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendImpl("procedure Test(MyArr: TArray);") + .appendImpl("begin") + .appendImpl(" // Fix@[+1:33 to +1:33] << - 1>>") + .appendImpl(" for var I := 1 to Length(MyArr) do begin") + .appendImpl(" // ...") + .appendImpl(" end;") + .appendImpl("end;")) + .verifyNoIssues(); + } + + @Test + void testIterationToLengthShouldAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new IterationPastHighBoundCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendImpl("procedure Test(MyArr: TArray);") + .appendImpl("var") + .appendImpl(" I: Integer;") + .appendImpl("begin") + .appendImpl(" for I := 0 to Length(MyArr) do begin // Noncompliant") + .appendImpl(" // ...") + .appendImpl(" end;") + .appendImpl("end;")) + .verifyIssues(); + } + + @Test + void testIterationToCountShouldAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new IterationPastHighBoundCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendImpl("type") + .appendImpl(" TMyCollection = class(TObject)") + .appendImpl(" FCount: Integer;") + .appendImpl(" property Count: Integer read FCount write FCount;") + .appendImpl(" end;") + .appendImpl("procedure Test(MyArr: TMyCollection);") + .appendImpl("var") + .appendImpl(" I: Integer;") + .appendImpl("begin") + .appendImpl(" for I := 0 to MyArr.Count do begin // Noncompliant") + .appendImpl(" // ...") + .appendImpl(" end;") + .appendImpl("end;")) + .verifyIssues(); + } + + @Test + void testIterationToInt64CountShouldAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new IterationPastHighBoundCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendImpl("type") + .appendImpl(" TMyCollection = class(TObject)") + .appendImpl(" FCount: Int64;") + .appendImpl(" property Count: Int64 read FCount write FCount;") + .appendImpl(" end;") + .appendImpl("procedure Test(MyArr: TMyCollection);") + .appendImpl("var") + .appendImpl(" I: Integer;") + .appendImpl("begin") + .appendImpl(" for I := 0 to MyArr.Count do begin // Noncompliant") + .appendImpl(" // ...") + .appendImpl(" end;") + .appendImpl("end;")) + .verifyIssues(); + } + + @Test + void testIterationToCountMinusOneShouldNotAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new IterationPastHighBoundCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendImpl("type") + .appendImpl(" TMyCollection = class(TObject)") + .appendImpl(" FCount: Integer;") + .appendImpl(" property Count: Integer read FCount write FCount;") + .appendImpl(" end;") + .appendImpl("procedure Test(MyArr: TMyCollection);") + .appendImpl("var") + .appendImpl(" I: Integer;") + .appendImpl("begin") + .appendImpl(" for I := 0 to MyArr.Count - 1 do begin") + .appendImpl(" // ...") + .appendImpl(" end;") + .appendImpl("end;")) + .verifyNoIssues(); + } + + @Test + void testIterationToPredCountShouldNotAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new IterationPastHighBoundCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendImpl("type") + .appendImpl(" TMyCollection = class(TObject)") + .appendImpl(" FCount: Integer;") + .appendImpl(" property Count: Integer read FCount write FCount;") + .appendImpl(" end;") + .appendImpl("procedure Test(MyArr: TMyCollection);") + .appendImpl("var") + .appendImpl(" I: Integer;") + .appendImpl("begin") + .appendImpl(" for I := 0 to Pred(MyArr.Count) do begin") + .appendImpl(" // ...") + .appendImpl(" end;") + .appendImpl("end;")) + .verifyNoIssues(); + } + + @Test + void testIterationToNestedCountShouldAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new IterationPastHighBoundCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendImpl("type") + .appendImpl(" TMyCollection = class(TObject)") + .appendImpl(" FCount: Integer;") + .appendImpl(" property Count: Integer read FCount write FCount;") + .appendImpl(" end;") + .appendImpl(" TMyWrapper = class(TObject)") + .appendImpl(" FCollection: TMyCollection;") + .appendImpl( + " property Collection: TMyCollection read FCollection write FCollection;") + .appendImpl(" end;") + .appendImpl("procedure Test(Wrapper: TMyWrapper);") + .appendImpl("var") + .appendImpl(" I: Integer;") + .appendImpl("begin") + .appendImpl(" for I := 0 to Wrapper.Collection.Count do begin // Noncompliant") + .appendImpl(" // ...") + .appendImpl(" end;") + .appendImpl("end;")) + .verifyIssues(); + } + + @Test + void testIterationToOtherNestedPropertyShouldNotAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new IterationPastHighBoundCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendImpl("type") + .appendImpl(" TMyCollection = class(TObject)") + .appendImpl(" FLevel: Integer;") + .appendImpl(" property Level: Integer read FLevel write FLevel;") + .appendImpl(" end;") + .appendImpl(" TMyWrapper = class(TObject)") + .appendImpl(" FCollection: TMyCollection;") + .appendImpl( + " property Collection: TMyCollection read FCollection write FCollection;") + .appendImpl(" end;") + .appendImpl("procedure Test(Wrapper: TMyWrapper);") + .appendImpl("var") + .appendImpl(" I: Integer;") + .appendImpl("begin") + .appendImpl(" for I := 0 to Wrapper.Collection.Level do begin") + .appendImpl(" // ...") + .appendImpl(" end;") + .appendImpl("end;")) + .verifyNoIssues(); + } + + @Test + void testNonIntegerCountShouldNotAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new IterationPastHighBoundCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendImpl("type") + .appendImpl(" TMyCollection = class(TObject)") + .appendImpl(" FCount: Double;") + .appendImpl(" property Count: Double read FCount write FCount;") + .appendImpl(" end;") + .appendImpl("procedure Test(MyCollection: TMyCollection);") + .appendImpl("var") + .appendImpl(" I: Integer;") + .appendImpl("begin") + .appendImpl(" for I := 0 to MyCollection.Count do begin") + .appendImpl(" // ...") + .appendImpl(" end;") + .appendImpl("end;")) + .verifyNoIssues(); + } + + @Test + void testStringIterationShouldNotAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new IterationPastHighBoundCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendImpl("procedure Test(MyArr: string);") + .appendImpl("var") + .appendImpl(" I: Integer;") + .appendImpl("begin") + .appendImpl(" for I := 1 to Length(MyArr) do begin") + .appendImpl(" // ...") + .appendImpl(" end;") + .appendImpl("end;")) + .verifyNoIssues(); + } + + @Test + void testIterationToParenthesizedLengthShouldAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new IterationPastHighBoundCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendImpl("procedure Test(MyArr: TArray);") + .appendImpl("var") + .appendImpl(" I: Integer;") + .appendImpl("begin") + .appendImpl(" // Fix@[+1:30 to +1:30] << - 1>>") + .appendImpl(" for I := 0 to (Length(MyArr)) do begin // Noncompliant") + .appendImpl(" // ...") + .appendImpl(" end;") + .appendImpl("end;")) + .verifyIssues(); + } +}