SONARJAVA-4263 Fix FP on S2325 when method requires type parameter from parent class. (#4954)

Co-authored-by: Dorian Burihabwa <75226315+dorian-burihabwa-sonarsource@users.noreply.github.com>

Author: tomasz-tylenda-sonarsource

its/ruling/src/test/resources/guava/java-S2325.json

@@ -1,17 +1,13 @@
 {
 "com.google.guava:guava:src/com/google/common/cache/LocalCache.java": [
-1964,
-3515
+1964
 ],
 "com.google.guava:guava:src/com/google/common/collect/HashBiMap.java": [
 358
 ],
 "com.google.guava:guava:src/com/google/common/collect/MapMakerInternalMap.java": [
 1975
 ],
-"com.google.guava:guava:src/com/google/common/collect/TreeRangeMap.java": [
-263
-],
 "com.google.guava:guava:src/com/google/common/io/ByteSource.java": [
 213,
 236

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

@@ -3,6 +3,7 @@
 import java.io.IOException;
 import java.io.ObjectStreamException;
 import java.io.Serializable;
+import java.util.List;
 import java.util.Map;
 import java.util.HashMap;
 import javax.annotation.Nullable;
@@ -363,3 +364,55 @@ synchronized private String magicWordSynchronized2() { // Noncompliant [[quickfi
     return magicWord;
   }
 }
+
+class ReturnTypeCheck {
+  static class SingleTypeVar<T> {
+    private List<T> requiresTypeVar() { // Compliant
+      return List.of();
+    }
+
+    private List<String> doesNotRequireTypeVar() { // Noncompliant
+      return List.of();
+    }
+
+    private static List<String> staticDoesNotRequireTypeVar() { // Compliant
+      return List.of();
+    }
+
+    private <U> List<U> requiresAnotherTypeVar() { // Noncompliant
+      return List.of();
+    }
+
+    private <T> List<T> requiresAnotherTypeVarShadowed() { // Noncompliant
+      return List.of();
+    }
+  }
+
+  static class TwoTypeVar<T1, T2> {
+    private List<T1> oneUsed1() { // Compliant
+      return List.of();
+    }
+
+    private <U> Map<T1, U> oneUsed2() { // Compliant
+      return Map.of();
+    }
+
+    private static <T1, U> Map<T1, U> bothShadowed() { // Compliant
+      return Map.of();
+    }
+
+    private List<String> noneUsed() {  // Noncompliant
+      return List.of();
+    }
+
+    private static List<String> staticNoneUsed() {  // Compliant
+      return List.of();
+    }
+  }
+
+  static class Nested<T> {
+    private List<List<T>> requiresNestedTypeVar() { // Compliant
+      return List.of();
+    }
+  }
+}

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

@@ -16,9 +16,13 @@
  */
 package org.sonar.java.checks;
 
+import java.util.ArrayList;
 import java.util.Deque;
 import java.util.LinkedList;
+import java.util.List;
 import java.util.Objects;
+import java.util.Set;
+import java.util.stream.Collectors;
 import javax.annotation.CheckForNull;
 import org.sonar.check.Rule;
 import org.sonar.java.checks.helpers.QuickFixHelper;
@@ -37,6 +41,7 @@
 import org.sonar.plugins.java.api.tree.Modifier;
 import org.sonar.plugins.java.api.tree.ModifierKeywordTree;
 import org.sonar.plugins.java.api.tree.Tree;
+import org.sonar.plugins.java.api.tree.TypeParameterTree;
 
 @Rule(key = "S2325")
 public class StaticMethodCheck extends BaseTreeVisitor implements JavaFileScanner {
@@ -85,7 +90,10 @@ public void visitMethod(MethodTree tree) {
       // In case it cannot be determined (isOverriding returns null), consider as overriding to avoid FP.
       return;
     }
-    if ((symbol.isPrivate() || symbol.isFinal() || classTree.symbol().isFinal()) && !symbol.isStatic() && !reference.hasNonStaticReference()) {
+    if ((symbol.isPrivate() || symbol.isFinal() || classTree.symbol().isFinal())
+      && !symbol.isStatic()
+      && !reference.hasNonStaticReference()
+      && !returnRequiresParentTypeParameter(symbol)) {
       QuickFixHelper.newIssue(context)
         .forRule(this)
         .onTree(tree.simpleName())
@@ -95,6 +103,43 @@ public void visitMethod(MethodTree tree) {
     }
   }
 
+  /**
+   * Method's return type requires type parameter and therefore cannot be made static.
+   */
+  private static boolean returnRequiresParentTypeParameter(Symbol.MethodSymbol symbol) {
+    List<Type> returnTypeVars = new ArrayList<>();
+    collectTypeVars(returnTypeVars, symbol.returnType().type());
+    if (returnTypeVars.isEmpty()) {
+      return false;
+    }
+    // called from visitMethod, so we have declaration (NPE not possible)
+    ClassTree parent = (ClassTree) symbol.declaration().parent();
+    Set<Symbol> parentTypeParam =
+      parent
+        .typeParameters()
+        .stream()
+        .map(TypeParameterTree::symbol)
+        .collect(Collectors.toUnmodifiableSet());
+    return returnTypeVars
+      .stream()
+      .map(Type::symbol)
+      .anyMatch(parentTypeParam::contains);
+  }
+
+  private static void collectTypeVars(List<Type> accumulator, Type t) {
+    // Consider a type such as `Map<T,Map<String,U>>`, it has two type
+    // arguments: `T` and `Map<String,U>`. We collect variables in the `accumulator`,
+    // and descend recursively otherwise (which is fine for simple types, such as String,
+    // which do not have variables).
+    for(Type tt: t.typeArguments()) {
+      if (tt.isTypeVar()) {
+        accumulator.add(tt);
+      } else {
+        collectTypeVars(accumulator, tt);
+      }
+    }
+  }
+
   private static JavaQuickFix getQuickFix(MethodTree tree) {
     Tree insertPosition = QuickFixHelper.nextToken(tree.modifiers());