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,102 @@ 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 Object helper() {
+      return new Object();
+    }
+
+    static Builder builder() {
+      return new Builder();
+    }
+
+    static class Builder {
+    }
+  }
+
+  static class ImmutableSetLike extends ImmutableCollectionLike {
+    static ImmutableSetLike of() { // Compliant - covariant static factory
+      return new ImmutableSetLike();
+    }
+
+    static List<String> copyOf(Collection<String> elements) { // Noncompliant {{Rename this method; it hides "copyOf" in "ImmutableCollectionLike".}}
+      return new ArrayList<>(elements);
+    }
+
+    static Object create() { // Noncompliant {{Rename this method; it hides "create" in "ImmutableCollectionLike".}}
+      return new Object();
+    }
+
+    static String helper() { // Noncompliant {{Rename this method; it hides "helper" in "ImmutableCollectionLike".}}
+      return "";
+    }
+
+    static Builder builder() { // Compliant - covariant static factory returning a nested type
+      return new Builder();
+    }
+
+    static class Builder extends ImmutableCollectionLike.Builder {
+    }
+  }
+
+  static class SameReturnParent {
+    static Integer value() {
+      return 1;
+    }
+  }
+
+  static class SameReturnChild extends SameReturnParent {
+    static Integer value() { // Noncompliant {{Rename this method; it hides "value" in "SameReturnParent".}}
+      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);
+    }
+  }
+
+  public static class ParentTestSuiteCase extends TestCase {
+    public static TestSuite suite() {
+      return new TestSuite(ParentTestSuiteCase.class);
+    }
+  }
+
+  public static class ChildTestSuiteCase extends ParentTestSuiteCase {
+    public static TestSuite suite() { // Compliant - JUnit 3 suite
+      return new TestSuite(ChildTestSuiteCase.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,45 @@ 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, this type being the declaring class, a subtype of it or a type nested in it
+   *   (e.g. Guava's {@code ImmutableBiMap.of()} hiding {@code ImmutableMap.of()}, or {@code ImmutableBiMap.builder()} hiding {@code ImmutableMap.builder()})</li>
+   *   <li>JUnit 3 {@code suite()} methods returning a {@code Test}, looked up by the test runner on each test class</li>
+   * </ul>
+   */
+  private static boolean isRequiredByDesign(Symbol.MethodSymbol methodSymbol, Symbol.MethodSymbol hiddenMethod) {
+    return isCovariantFactory(methodSymbol, hiddenMethod) || isJUnit3Suite(methodSymbol);
+  }
+
+  private static boolean isCovariantFactory(Symbol.MethodSymbol methodSymbol, Symbol.MethodSymbol hiddenMethod) {
+    Type returnType = methodSymbol.returnType().type().erasure();
+    Type hiddenReturnType = hiddenMethod.returnType().type().erasure();
+    if (returnType.equals(hiddenReturnType) || !returnType.isSubtypeOf(hiddenReturnType)) {
+      return false;
+    }
+    Symbol owner = methodSymbol.owner();
+    return returnType.isSubtypeOf(owner.type().erasure()) || isNestedIn(returnType.symbol(), owner);
+  }
+
+  private static boolean isNestedIn(Symbol symbol, Symbol enclosingType) {
+    Symbol current = symbol.owner();
+    while (current != null && current.isTypeSymbol()) {
+      if (current.equals(enclosingType)) {
+        return true;
+      }
+      current = current.owner();
+    }
+    return false;
+  }
+
+  private static boolean isJUnit3Suite(Symbol.MethodSymbol methodSymbol) {
+    return "suite".equals(methodSymbol.name())
+      && methodSymbol.parameterTypes().isEmpty()
+      && methodSymbol.returnType().type().isSubtypeOf("junit.framework.Test");
+  }
+
   private static boolean hasSameParameterTypes(Symbol.MethodSymbol method, Symbol.MethodSymbol candidate) {
     List<Type> methodParams = method.parameterTypes();
     List<Type> candidateParams = candidate.parameterTypes();