Fix FPs around binary expressions in PlatformDependentTruncation

integrated-application-development · Dec 6, 2024 · 05c4b0d · 05c4b0d
1 parent 66b6c55
commit 05c4b0d
Show file tree

Hide file tree

Showing 3 changed files with 92 additions and 5 deletions.
diff --git 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/...hecks/src/main/java/au/com/integradev/delphi/checks/PlatformDependentTruncationCheck.java b/...hecks/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<Parameter> 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,16 +85,26 @@ 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;
     }
 
     if (isNativeInteger(from) == isNativeInteger(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/...s/src/test/java/au/com/integradev/delphi/checks/PlatformDependentTruncationCheckTest.java b/...s/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();
+  }
 }