SONARJAVA-7242: Improve S9149 to not raise when the parent class design requires redeclaring static methods

its/autoscan/src/test/resources/autoscan/diffs/diff_S9149.json

@@ -2,5 +2,5 @@
   "ruleKey": "S9149",
   "hasTruePositives": true,
   "falseNegatives": 0,
-  "falsePositives": 0
+  "falsePositives": 1
 }

its/ruling/src/test/resources/expected/commons-beanutils/java-S9149.json

@@ -8,11 +8,5 @@
 432,
 487,
 512
-],
-"commons-beanutils:src/test/java/org/apache/commons/beanutils2/BeanUtils2TestCase.java": [
-57
-],
-"commons-beanutils:src/test/java/org/apache/commons/beanutils2/WrapDynaBeanTestCase.java": [
-75
 ]
 }

java-checks-test-sources/default/src/main/files/non-compiling/checks/StaticMethodHidingCheckSample.java

@@ -27,4 +27,18 @@ static void process(Unknown param) { // Compliant - parameter type is unknown
     }
   }
 
+  // --- Noncompliant: unknown return types are not considered covariant ---
+
+  static class ParentWithUnknownReturn {
+    static Unknown create() {
+      return null;
+    }
+  }
+
+  static class ChildWithUnknownReturn extends ParentWithUnknownReturn {
+    static Unknown create() { // Noncompliant {{Rename this method; it hides "create" in "ParentWithUnknownReturn".}}
+      return null;
+    }
+  }
+
 }

java-checks-test-sources/default/src/main/java/checks/StaticMethodHidingCheckSample.java

@@ -2,7 +2,12 @@
 
 import com.google.errorprone.annotations.DoNotCall;
 import com.google.errorprone.annotations.InlineMe;
+import java.util.ArrayList;
+import java.util.Collection;
 import java.util.List;
+import junit.framework.Test;
+import junit.framework.TestCase;
+import junit.framework.TestSuite;
 import org.jetbrains.annotations.ApiStatus;
 
 class StaticMethodHidingCheckSample {
@@ -331,4 +336,68 @@ static class PlainChild extends PlainParent {
     static void compute() { // Noncompliant {{Rename this method; it hides "compute" in "PlainParent".}}
     }
   }
+
+  // --- Compliant: static factory returning a more specific type ---
+
+  static class ImmutableCollectionLike {
+    static ImmutableCollectionLike of() {
+      return new ImmutableCollectionLike();
+    }
+
+    static Collection<String> copyOf(Collection<String> elements) {
+      return elements;
+    }
+
+    static Object create() {
+      return new Object();
+    }
+  }
+
+  static class ImmutableSetLike extends ImmutableCollectionLike {
+    static ImmutableSetLike of() { // Compliant - covariant static factory
+      return new ImmutableSetLike();
+    }
+
+    static List<String> copyOf(Collection<String> elements) { // Compliant - covariant static factory
+      return new ArrayList<>(elements);
+    }
+
+    static Object create() { // Noncompliant {{Rename this method; it hides "create" in "ImmutableCollectionLike".}}
+      return new Object();
+    }
+  }
+
+  static class UnrelatedReturnParent {
+    static Integer value() {
+      return 1;
+    }
+  }
+
+  static class UnrelatedReturnChild extends UnrelatedReturnParent {
+    static Integer value() { // Noncompliant {{Rename this method; it hides "value" in "UnrelatedReturnParent".}}
+      return 2;
+    }
+  }
+
+  // --- Compliant: JUnit 3 suite() methods have to be redeclared in each test class ---
+
+  public static class ParentTestCase extends TestCase {
+    public static Test suite() {
+      return new TestSuite(ParentTestCase.class);
+    }
+
+    public static Test other() {
+      return new TestSuite(ParentTestCase.class);
+    }
+  }
+
+  public static class ChildTestCase extends ParentTestCase {
+    public static Test suite() { // Compliant - JUnit 3 suite
+      return new TestSuite(ChildTestCase.class);
+    }
+
+    public static Test other() { // Noncompliant {{Rename this method; it hides "other" in "ParentTestCase".}}
+      return new TestSuite(ChildTestCase.class);
+    }
+  }
 }

java-checks/src/main/java/org/sonar/java/checks/StaticMethodHidingCheck.java

@@ -58,7 +58,9 @@ private boolean checkHiding(MethodTree methodTree, Symbol.MethodSymbol methodSym
     for (Symbol symbol : superClass.symbol().lookupSymbols(methodSymbol.name())) {
       if (symbol.isMethodSymbol() && symbol.isStatic() && !symbol.isPrivate()
         && hasSameParameterTypes(methodSymbol, (Symbol.MethodSymbol) symbol)) {
-        reportHidingIssue(methodTree, methodSymbol, (Symbol.MethodSymbol) symbol);
+        if (!isRequiredByDesign(methodSymbol, (Symbol.MethodSymbol) symbol)) {
+          reportHidingIssue(methodTree, methodSymbol, (Symbol.MethodSymbol) symbol);
+        }
         return true;
       }
     }
@@ -88,6 +90,26 @@ private static boolean isIntentionalHiding(Symbol.MethodSymbol methodSymbol) {
       || methodSymbol.metadata().isAnnotatedWith("org.jetbrains.annotations.ApiStatus$ScheduledForRemoval");
   }
 
+  /**
+   * Some hierarchies are designed so that subclasses have to redeclare static methods of their parent:
+   * <ul>
+   *   <li>static factories returning a more specific type than the hidden one (e.g. Guava's {@code ImmutableBiMap.of()} hiding {@code ImmutableMap.of()})</li>
+   *   <li>JUnit 3 {@code public static Test suite()} methods, looked up by the test runner on each test class</li>
+   * </ul>
+   */
+  private static boolean isRequiredByDesign(Symbol.MethodSymbol methodSymbol, Symbol.MethodSymbol hiddenMethod) {
+    Type returnType = methodSymbol.returnType().type().erasure();
+    Type hiddenReturnType = hiddenMethod.returnType().type().erasure();
+    boolean isCovariantFactory = !returnType.equals(hiddenReturnType) && returnType.isSubtypeOf(hiddenReturnType);
+    return isCovariantFactory || isJUnit3Suite(methodSymbol);
+  }
+
+  private static boolean isJUnit3Suite(Symbol.MethodSymbol methodSymbol) {
+    return "suite".equals(methodSymbol.name())
+      && methodSymbol.parameterTypes().isEmpty()
+      && methodSymbol.returnType().type().is("junit.framework.Test");
+  }
+
   private static boolean hasSameParameterTypes(Symbol.MethodSymbol method, Symbol.MethodSymbol candidate) {
     List<Type> methodParams = method.parameterTypes();
     List<Type> candidateParams = candidate.parameterTypes();