Fix FPs in RedundantInheritedCheck

Key changes: - Ignore methods with `message` directive, useful until method resolution catches up - Remove need for overriding method parameter names to be equal
integrated-application-development · Jul 31, 2024 · 3acb536 · 3acb536
1 parent 37b312c
commit 3acb536
Show file tree

Hide file tree

Showing 4 changed files with 95 additions and 17 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -31,6 +31,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - Overly permissive parsing rules around `string` and `file` types.
 - Quick fixes removing comments and compiler directives in `RedundantInherited`.
+- False positives around `message` handler methods in `RedundantInherited`.
+- False positives when parameter names were mismatched to the inherited method in `RedundantInherited`.
 - **API:** `ArrayConstructorNode::getImage` returned an incorrect image containing `[` as an element.
 
 ## [1.7.0] - 2024-07-02

diff --git a/delphi-checks/src/main/java/au/com/integradev/delphi/checks/RedundantInheritedCheck.java b/delphi-checks/src/main/java/au/com/integradev/delphi/checks/RedundantInheritedCheck.java
@@ -35,6 +35,7 @@
 import org.sonar.plugins.communitydelphi.api.check.FilePosition;
 import org.sonar.plugins.communitydelphi.api.reporting.QuickFix;
 import org.sonar.plugins.communitydelphi.api.reporting.QuickFixEdit;
+import org.sonar.plugins.communitydelphi.api.symbol.declaration.RoutineDirective;
 import org.sonar.plugins.communitydelphi.api.symbol.declaration.RoutineNameDeclaration;
 import org.sonar.plugins.communitydelphi.api.token.DelphiToken;
 import org.sonar.plugins.communitydelphi.api.token.DelphiTokenType;
@@ -119,6 +120,15 @@ private static DelphiNode getLastStatementToken(
   }
 
   private static List<DelphiNode> findViolations(RoutineImplementationNode routine) {
+    RoutineNameDeclaration nameDeclaration = routine.getRoutineNameDeclaration();
+    if (nameDeclaration != null && nameDeclaration.hasDirective(RoutineDirective.MESSAGE)) {
+      // HACK:
+      //   Calls to inherited don't currently resolve to parent message handlers with different
+      //   signatures.
+      //   See: https://github.com/integrated-application-development/sonar-delphi/issues/271
+      return Collections.emptyList();
+    }
+
     CompoundStatementNode block = routine.getStatementBlock();
     if (block == null) {
       return Collections.emptyList();

diff --git a/delphi-checks/src/main/java/au/com/integradev/delphi/utils/RoutineUtils.java b/delphi-checks/src/main/java/au/com/integradev/delphi/utils/RoutineUtils.java
@@ -23,6 +23,7 @@
 import java.util.Collections;
 import java.util.List;
 import java.util.stream.Collectors;
+import java.util.stream.IntStream;
 import java.util.stream.Stream;
 import org.sonar.plugins.communitydelphi.api.ast.AssignmentStatementNode;
 import org.sonar.plugins.communitydelphi.api.ast.RaiseStatementNode;
@@ -76,25 +77,32 @@ private static Stream<Type> concreteParentTypesStream(Type type) {
 
   private static boolean isOverriddenMethod(
       RoutineNameDeclaration parent, RoutineNameDeclaration child) {
-    if (parent.getName().equalsIgnoreCase(child.getName())
-        && parent.getParameters().equals(child.getParameters())) {
-      if (parent.isClassInvocable()) {
-        if (parent.getRoutineKind() == RoutineKind.CONSTRUCTOR
-            || parent.getRoutineKind() == RoutineKind.DESTRUCTOR) {
-          // An instance constructor or destructor cannot inherit from a class constructor or
-          // destructor
-          return child.isClassInvocable();
-        } else {
-          // Any other type of invocable can inherit from a class invocable
-          return true;
-        }
-      } else {
-        // A class invocable cannot inherit from an instance invocable
-        return !child.isClassInvocable();
-      }
-    } else {
+    if (!(parent.getName().equalsIgnoreCase(child.getName())
+        && parent.getParametersCount() == child.getParametersCount())) {
+      return false;
+    }
+
+    if (!IntStream.range(0, parent.getParametersCount())
+        .allMatch(
+            param ->
+                parent.getParameter(param).getType().is(child.getParameter(param).getType()))) {
       return false;
     }
+
+    if (!parent.isClassInvocable()) {
+      // A class invocable cannot inherit from an instance invocable
+      return !child.isClassInvocable();
+    }
+
+    if (parent.getRoutineKind() == RoutineKind.CONSTRUCTOR
+        || parent.getRoutineKind() == RoutineKind.DESTRUCTOR) {
+      // An instance constructor or destructor cannot inherit from a class constructor or
+      // destructor
+      return child.isClassInvocable();
+    }
+
+    // Any other type of invocable can inherit from a class invocable
+    return true;
   }
 
   public static List<RoutineNameDeclaration> findParentMethodDeclarations(

diff --git a/delphi-checks/src/test/java/au/com/integradev/delphi/checks/RedundantInheritedCheckTest.java b/delphi-checks/src/test/java/au/com/integradev/delphi/checks/RedundantInheritedCheckTest.java
@@ -123,6 +123,64 @@ void testOverridingGrandparentShouldNotAddIssue() {
         .verifyNoIssues();
   }
 
+  @Test
+  void testOverridingMethodWithDifferentParameterNamesShouldNotAddIssues() {
+    CheckVerifier.newVerifier()
+        .withCheck(new RedundantInheritedCheck())
+        .onFile(
+            new DelphiTestUnitBuilder()
+                .appendDecl("type")
+                .appendDecl("  TBase = class(TObject)")
+                .appendDecl("    procedure MyProcedure(A: TObject);")
+                .appendDecl("  end;")
+                .appendDecl("  TChild = class(TBase)")
+                .appendDecl("    procedure MyProcedure(B: TObject);")
+                .appendDecl("  end;")
+                .appendImpl("procedure TChild.MyProcedure;")
+                .appendImpl("begin")
+                .appendImpl("  inherited; // Compliant")
+                .appendImpl("end;"))
+        .verifyNoIssues();
+  }
+
+  @Test
+  void testOverridingMethodWithDifferentArgsShouldAddIssues() {
+    CheckVerifier.newVerifier()
+        .withCheck(new RedundantInheritedCheck())
+        .onFile(
+            new DelphiTestUnitBuilder()
+                .appendDecl("type")
+                .appendDecl("  TBase = class(TObject)")
+                .appendDecl("    procedure MyProcedure(A: TObject); overload;")
+                .appendDecl("    procedure MyProcedure(A: TObject; B: String); overload;")
+                .appendDecl("  end;")
+                .appendDecl("  TChild = class(TBase)")
+                .appendDecl("    procedure MyProcedure;")
+                .appendDecl("  end;")
+                .appendImpl("procedure TChild.MyProcedure;")
+                .appendImpl("begin")
+                .appendImpl("  inherited; // Noncompliant")
+                .appendImpl("end;"))
+        .verifyIssues();
+  }
+
+  @Test
+  void testMessageHandlerShouldNotAddIssues() {
+    CheckVerifier.newVerifier()
+        .withCheck(new RedundantInheritedCheck())
+        .onFile(
+            new DelphiTestUnitBuilder()
+                .appendDecl("type")
+                .appendDecl("  TFoo = class(TObject)")
+                .appendDecl("    procedure Handler(A: TObject); message 1;")
+                .appendDecl("  end;")
+                .appendImpl("procedure TFoo.Handler;")
+                .appendImpl("begin")
+                .appendImpl("  inherited; // Compliant")
+                .appendImpl("end;"))
+        .verifyNoIssues();
+  }
+
   @Test
   void testNotOverridingMethodShouldAddIssues() {
     CheckVerifier.newVerifier()