From 05c4b0d9b79ef5c0f4a8c356e8877ba64f516858 Mon Sep 17 00:00:00 2001 From: Jonah Jeleniewski Date: Thu, 5 Dec 2024 19:03:56 +1100 Subject: [PATCH] Fix FPs around binary expressions in `PlatformDependentTruncation` --- CHANGELOG.md | 1 + .../PlatformDependentTruncationCheck.java | 21 ++++-- .../PlatformDependentTruncationCheckTest.java | 75 +++++++++++++++++++ 3 files changed, 92 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a6012ea7..904c0b4bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Various intrinsic routines had incorrect signatures around dynamic and open arrays. +- False positives around platform-dependent binary expressions in `PlatformDependentTruncation`. ## [1.12.0] - 2024-12-02 diff --git a/delphi-checks/src/main/java/au/com/integradev/delphi/checks/PlatformDependentTruncationCheck.java b/delphi-checks/src/main/java/au/com/integradev/delphi/checks/PlatformDependentTruncationCheck.java index ce0599337..40917e83f 100644 --- a/delphi-checks/src/main/java/au/com/integradev/delphi/checks/PlatformDependentTruncationCheck.java +++ b/delphi-checks/src/main/java/au/com/integradev/delphi/checks/PlatformDependentTruncationCheck.java @@ -23,6 +23,7 @@ import org.sonar.plugins.communitydelphi.api.ast.ArgumentListNode; import org.sonar.plugins.communitydelphi.api.ast.ArgumentNode; import org.sonar.plugins.communitydelphi.api.ast.AssignmentStatementNode; +import org.sonar.plugins.communitydelphi.api.ast.BinaryExpressionNode; import org.sonar.plugins.communitydelphi.api.ast.ExpressionNode; import org.sonar.plugins.communitydelphi.api.ast.NameReferenceNode; import org.sonar.plugins.communitydelphi.api.ast.Node; @@ -44,7 +45,7 @@ public class PlatformDependentTruncationCheck extends DelphiCheck { @Override public DelphiCheckContext visit(AssignmentStatementNode assignment, DelphiCheckContext context) { - if (isViolation(assignment.getValue().getType(), assignment.getAssignee().getType())) { + if (isViolation(assignment.getValue(), assignment.getAssignee().getType())) { reportIssue(context, assignment, MESSAGE); } return super.visit(assignment, context); @@ -66,7 +67,7 @@ public DelphiCheckContext visit(ArgumentListNode argumentList, DelphiCheckContex List parameters = procedural.parameters(); for (int i = 0; i < arguments.size() && i < parameters.size(); ++i) { ExpressionNode argument = arguments.get(i).getExpression(); - if (isViolation(argument.getType(), parameters.get(i).getType())) { + if (isViolation(argument, parameters.get(i).getType())) { reportIssue(context, argument, MESSAGE); } } @@ -84,8 +85,8 @@ private static ProceduralType getProceduralType(NameReferenceNode reference) { return null; } - private static boolean isViolation(Type from, Type to) { - if (!from.isInteger() || !to.isInteger()) { + private static boolean isViolation(ExpressionNode from, Type to) { + if (!from.getType().isInteger() || !to.isInteger()) { return false; } @@ -93,7 +94,17 @@ private static boolean isViolation(Type from, Type to) { return false; } - return (isNativeInteger(from) && to.size() < 8) || (isNativeInteger(to) && from.size() > 4); + return (isNativeInteger(from) && to.size() < 8) + || (isNativeInteger(to) && from.getType().size() > 4); + } + + private static boolean isNativeInteger(ExpressionNode expression) { + expression = expression.skipParentheses(); + if (expression instanceof BinaryExpressionNode) { + BinaryExpressionNode binary = (BinaryExpressionNode) expression; + return isNativeInteger(binary.getLeft()) || isNativeInteger(binary.getRight()); + } + return isNativeInteger(expression.getType()); } private static boolean isNativeInteger(Type type) { diff --git a/delphi-checks/src/test/java/au/com/integradev/delphi/checks/PlatformDependentTruncationCheckTest.java b/delphi-checks/src/test/java/au/com/integradev/delphi/checks/PlatformDependentTruncationCheckTest.java index fe181088a..84bb884ab 100644 --- a/delphi-checks/src/test/java/au/com/integradev/delphi/checks/PlatformDependentTruncationCheckTest.java +++ b/delphi-checks/src/test/java/au/com/integradev/delphi/checks/PlatformDependentTruncationCheckTest.java @@ -21,6 +21,7 @@ import au.com.integradev.delphi.builders.DelphiTestUnitBuilder; import au.com.integradev.delphi.checks.verifier.CheckVerifier; import au.com.integradev.delphi.compiler.CompilerVersion; +import au.com.integradev.delphi.compiler.Toolchain; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; @@ -207,4 +208,78 @@ void testNativeIntArgumentToNativeIntParameterShouldNotAddIssue(String versionSy .appendImpl("end;")) .verifyNoIssues(); } + + @ParameterizedTest + @ValueSource(strings = {VERSION_ALEXANDRIA, VERSION_ATHENS}) + void testNativeIntAssignmentInBinaryExpressionShouldNotAddIssue(String versionSymbol) { + CheckVerifier.newVerifier() + .withCheck(new PlatformDependentTruncationCheck()) + .withCompilerVersion(CompilerVersion.fromVersionSymbol(versionSymbol)) + .withToolchain(Toolchain.DCC64) + .onFile( + new DelphiTestUnitBuilder() + .appendImpl("procedure Foo;") + .appendImpl("var") + .appendImpl(" Nat: NativeInt;") + .appendImpl("begin") + .appendImpl(" Nat := Nat + 1;") + .appendImpl("end;")) + .verifyNoIssues(); + } + + @ParameterizedTest + @ValueSource(strings = {VERSION_ALEXANDRIA, VERSION_ATHENS}) + void testNativeIntArgumentInBinaryExpressionShouldNotAddIssue(String versionSymbol) { + CheckVerifier.newVerifier() + .withCheck(new PlatformDependentTruncationCheck()) + .withCompilerVersion(CompilerVersion.fromVersionSymbol(versionSymbol)) + .withToolchain(Toolchain.DCC64) + .onFile( + new DelphiTestUnitBuilder() + .appendDecl("procedure Bar(Nat: NativeInt);") + .appendImpl("procedure Foo;") + .appendImpl("var") + .appendImpl(" Nat: NativeInt;") + .appendImpl("begin") + .appendImpl(" Bar(Nat + 1);") + .appendImpl("end;")) + .verifyNoIssues(); + } + + @ParameterizedTest + @ValueSource(strings = {VERSION_ALEXANDRIA, VERSION_ATHENS}) + void testNativeIntAssignmentInNestedBinaryExpressionShouldNotAddIssue(String versionSymbol) { + CheckVerifier.newVerifier() + .withCheck(new PlatformDependentTruncationCheck()) + .withCompilerVersion(CompilerVersion.fromVersionSymbol(versionSymbol)) + .withToolchain(Toolchain.DCC64) + .onFile( + new DelphiTestUnitBuilder() + .appendImpl("procedure Foo;") + .appendImpl("var") + .appendImpl(" Nat: NativeInt;") + .appendImpl("begin") + .appendImpl(" Nat := (Nat + 1) + 1;") + .appendImpl("end;")) + .verifyNoIssues(); + } + + @ParameterizedTest + @ValueSource(strings = {VERSION_ALEXANDRIA, VERSION_ATHENS}) + void testPlatformDependentCastInBinaryExpressionShouldNotAddIssue(String versionSymbol) { + CheckVerifier.newVerifier() + .withCheck(new PlatformDependentTruncationCheck()) + .withCompilerVersion(CompilerVersion.fromVersionSymbol(versionSymbol)) + .withToolchain(Toolchain.DCC64) + .onFile( + new DelphiTestUnitBuilder() + .appendImpl("procedure Foo;") + .appendImpl("var") + .appendImpl(" Int: Integer;") + .appendImpl(" Nat: NativeInt;") + .appendImpl("begin") + .appendImpl(" Int := Integer(Nat) + 1;") + .appendImpl("end;")) + .verifyNoIssues(); + } }