SONARJAVA-4835: FP on S3242 forcing user to add unnecessary logic (#4708

)
SonarSource · Mar 13, 2024 · 23337de · 23337de
1 parent 4e949c9
commit 23337de
Show file tree

Hide file tree

Showing 7 changed files with 113 additions and 44 deletions.
diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S2095.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S2095.json
@@ -3,4 +3,4 @@
   "hasTruePositives": true,
   "falseNegatives": 67,
   "falsePositives": 0
-}
+}
diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S2699.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S2699.json
@@ -3,4 +3,4 @@
   "hasTruePositives": true,
   "falseNegatives": 147,
   "falsePositives": 1
-}
+}
diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S6913.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S6913.json
@@ -3,4 +3,4 @@
   "hasTruePositives": false,
   "falseNegatives": 0,
   "falsePositives": 0
-}
+}
diff --git a/its/ruling/src/test/resources/eclipse-jetty/java-S3242.json b/its/ruling/src/test/resources/eclipse-jetty/java-S3242.json
@@ -8,10 +8,6 @@
 "org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionData.java": [
 73
 ],
-"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java": [
-251,
-277
-],
 "org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/FutureCallback.java": [
 176
 ],
@@ -21,9 +17,6 @@
 "org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/IntrospectionUtil.java": [
 215
 ],
-"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/LazyList.java": [
-147
-],
 "org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/PathWatcher.java": [
 211,
 271

diff --git a/its/ruling/src/test/resources/sonar-server/java-S3242.json b/its/ruling/src/test/resources/sonar-server/java-S3242.json
@@ -2,10 +2,6 @@
 "org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/component/index/ComponentIndexer.java": [
 113
 ],
-"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/es/EsUtils.java": [
-69,
-78
-],
 "org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/issue/index/IssueIndexer.java": [
 137
 ],

diff --git a/java-checks-test-sources/default/src/main/java/checks/LeastSpecificTypeCheck.java b/java-checks-test-sources/default/src/main/java/checks/LeastSpecificTypeCheck.java
@@ -6,8 +6,8 @@
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.List;
-import java.util.function.UnaryOperator;
 import java.util.function.Function;
+import java.util.function.UnaryOperator;
 import javax.annotation.Resource;
 import javax.inject.Inject;
 import org.springframework.beans.factory.annotation.Autowired;
@@ -103,6 +103,7 @@ public void autowiredMethod3(List<Object> list) { // Compliant - List interface
       o.toString();
     }
   }
+
   @jakarta.annotation.Resource
   public void jakartaRAnnotatedMethod4(Collection<Object> list) { // Compliant - since Spring annotated methods cannot take 'Iterable' as argument
     for (Object o : list) {
@@ -176,10 +177,12 @@ abstract static class Base implements IBase {
 
   protected static class ProtectedBase extends Base {
     @Override
-    public void b2() { }
+    public void b2() {
+    }
 
     @Override
-    public void b() { }
+    public void b() {
+    }
   }
 
   public static class Visibility extends ProtectedBase {
@@ -191,7 +194,8 @@ public static void visibility(Visibility vis) { // Compliant - Base has package
     vis.b();
   }
 
-  protected static class Visibility2 extends ProtectedBase { }
+  protected static class Visibility2 extends ProtectedBase {
+  }
 
   public static void visibility(Visibility2 vis) { // False-Negative
     vis.b();
@@ -224,9 +228,12 @@ public static void fieldsAreIgnored(E e) { // Compliant
   }
 
   private static class PrivateClass implements IBase {
-    void m2() {}
+    void m2() {
+    }
+
     @Override
-    public void b2() { }
+    public void b2() {
+    }
   }
 
   protected static class ProtectedClass extends PrivateClass {
@@ -237,15 +244,16 @@ static class PackageClass extends ProtectedClass {
 
   }
 
-  public static void coverage(PrivateClass c) { // Noncompliant  {{Use 'checks.LeastSpecificTypeCheck.IBase' here; it is a more general type than 'PrivateClass'.}}
+  public static void coverage(PrivateClass c) { // Noncompliant {{Use 'checks.LeastSpecificTypeCheck.IBase' here; it is a more general type than 'PrivateClass'.}}
     c.b2();
   }
 
   public static void coverage2(PackageClass c) {
     c.m2();
   }
 
-  public static void primitiveTypesAreIgnored(int i, long l, double d, float f, byte b, short s, char c, boolean boo) { }
+  public static void primitiveTypesAreIgnored(int i, long l, double d, float f, byte b, short s, char c, boolean boo) {
+  }
 
   public BigDecimal getUnaryOperator(UnaryOperator<BigDecimal> func) { // Compliant
     // issue exception because UnaryOperator<BigDecimal> is a better functional interface usage than Function<BigDecimal, BigDecimal>
@@ -256,4 +264,72 @@ public BigDecimal getFunction(Function<BigDecimal, BigDecimal> func) { // S4276
     return func.apply(BigDecimal.ONE);
   }
 
+  public static class A {
+    public A getMe() {
+      return this;
+    }
+  }
+
+  public static class B extends A {
+    @Override
+    public B getMe() {
+      return this;
+    }
+
+    public void doSomethingOnlyOnB() {
+    }
+  }
+
+  public B getB(B b) { // Compliant, don't raise issue when the return type is the same as the non-least specific parameter type
+    return b.getMe();
+  }
+
+  public A getA(A b) { // Compliant
+    return b.getMe();
+  }
+
+  public A getAFromB(B b) { // Complaint
+    b.doSomethingOnlyOnB();
+    return b.getMe();
+  }
+
+  public static class X {
+    public Number getOtherThanOwnerType() {
+      return 23;
+    }
+  }
+
+  public static class Y extends X {
+    @Override
+    public Integer getOtherThanOwnerType() {
+      return 42;
+    }
+  }
+
+  public Integer getNonRelated(Y y) { // Compliant
+    return y.getOtherThanOwnerType();
+  }
+
+  interface A1 {
+    default void a(){}
+  }
+  interface A2 {
+    default void a(String a){}
+  }
+  interface A3 {
+    default int a(String a, String b){ return 0;}
+  }
+
+  public static class OverloadedA1A2 implements A1, A2 {
+
+  }
+
+  public static class OverloadedA1A2A3 extends OverloadedA1A2 implements A3 {
+  }
+
+  public OverloadedA1A2 overloaded(OverloadedA1A2A3 a) { // FN it should suggest to use "OverloadedA1A2" as parameter type
+    a.a();
+    a.a("a");
+    return a;
+  }
 }
diff --git a/java-checks/src/main/java/org/sonar/java/checks/LeastSpecificTypeCheck.java b/java-checks/src/main/java/org/sonar/java/checks/LeastSpecificTypeCheck.java
@@ -48,8 +48,7 @@ public class LeastSpecificTypeCheck extends IssuableSubscriptionVisitor {
     "javax.inject.Inject",
     "jakarta.inject.Inject",
     "javax.annotation.Resource",
-    "jakarta.annotation.Resource"
-  );
+    "jakarta.annotation.Resource");
 
   @Override
   public List<Tree.Kind> nodesToVisit() {
@@ -90,11 +89,14 @@ private void handleParameter(Symbol parameter, boolean springInjectionAnnotated)
       // Exclude functional interface, it's wrong to have issues on UnaryOperator<T> and ask the user to use Function<T,T> instead
       return;
     }
+
     Type leastSpecificType = findLeastSpecificType(parameter);
     if (parameterType != leastSpecificType
       && !leastSpecificType.is("java.lang.Object")) {
+
       String suggestedType = getSuggestedType(springInjectionAnnotated, leastSpecificType);
-      reportIssue(parameter.declaration(), String.format("Use '%s' here; it is a more general type than '%s'.", suggestedType, parameterType.erasure().name()));
+      String message = String.format("Use '%s' here; it is a more general type than '%s'.", suggestedType, parameterType.erasure().name());
+      reportIssue(parameter.declaration(), message);
     }
   }
 
@@ -127,10 +129,11 @@ private static Type findLeastSpecificType(Symbol parameter) {
     return inheritanceGraph.leastSpecificType();
   }
 
-  private static Optional<Symbol> findIteratorMethod(Symbol parameter) {
+  private static Optional<Symbol.MethodSymbol> findIteratorMethod(Symbol parameter) {
     return parameter.type().symbol().lookupSymbols("iterator").stream()
       .filter(Symbol::isMethodSymbol)
-      .filter(s -> ((Symbol.MethodSymbol) s).parameterTypes().isEmpty())
+      .map(Symbol.MethodSymbol.class::cast)
+      .filter(methodSymbol -> methodSymbol.parameterTypes().isEmpty())
       .findFirst();
   }
 
@@ -143,15 +146,15 @@ private InheritanceGraph(Type type) {
       startType = type;
     }
 
-    private void update(Symbol m) {
+    private void update(Symbol.MethodSymbol m) {
       if (chains == null) {
         chains = computeChains(m, startType);
       } else {
         refineChains(m);
       }
     }
 
-    private List<List<Type>> computeChains(Symbol m, Type type) {
+    private List<List<Type>> computeChains(Symbol.MethodSymbol m, Type type) {
       List<List<Type>> result = new ArrayList<>();
       Symbol.TypeSymbol typeSymbol = type.symbol();
       Type superClass = typeSymbol.superClass();
@@ -172,23 +175,27 @@ private List<List<Type>> computeChains(Symbol m, Type type) {
       return result;
     }
 
-    private void computeChainsForSuperType(List<List<Type>> result, Symbol m, Type type, Type superType) {
-      for (List<Type> chain : computeChains(m, superType)) {
+    private void computeChainsForSuperType(List<List<Type>> result, Symbol.MethodSymbol methodSymbol, Type type, Type superType) {
+      for (List<Type> chain : computeChains(methodSymbol, superType)) {
         chain.add(type);
         result.add(chain);
       }
     }
 
-    private static boolean definesOrInheritsSymbol(Symbol symbol, Symbol.TypeSymbol typeSymbol) {
-      return definesSymbol(symbol, typeSymbol)
-        || typeSymbol.superTypes().stream().anyMatch(superType -> definesSymbol(symbol, superType.symbol()));
+    private static boolean definesOrInheritsSymbol(Symbol.MethodSymbol methodSymbol, Symbol.TypeSymbol typeSymbol) {
+      return definesSymbol(methodSymbol, typeSymbol)
+        || typeSymbol.superTypes().stream().anyMatch(superType -> definesSymbol(methodSymbol, superType.symbol()));
     }
 
-    private static boolean definesSymbol(Symbol m, Symbol.TypeSymbol typeSymbol) {
-      return typeSymbol.memberSymbols().stream().anyMatch(s -> isOverriding(m, s));
+    private static boolean definesSymbol(Symbol.MethodSymbol methodSymbol, Symbol.TypeSymbol typeSymbol) {
+      return typeSymbol.memberSymbols()
+        .stream()
+        .filter(Symbol::isMethodSymbol)
+        .map(Symbol.MethodSymbol.class::cast)
+        .anyMatch(memberMethodSymbol -> isOverridingWithSameReturnType(methodSymbol, memberMethodSymbol));
     }
 
-    private void refineChains(Symbol m) {
+    private void refineChains(Symbol.MethodSymbol m) {
       for (List<Type> chain : chains) {
         Iterator<Type> chainIterator = chain.iterator();
         while (chainIterator.hasNext()) {
@@ -219,13 +226,11 @@ private Type leastSpecificType() {
       return longestChain.get(0);
     }
 
-    private static boolean isOverriding(Symbol s1, Symbol s2) {
-      return s1.isMethodSymbol()
-        && s2.isMethodSymbol()
-        && s1.name().equals(s2.name())
-        && ConfusingOverloadCheck.isPotentialOverride((Symbol.MethodSymbol) s1, (Symbol.MethodSymbol) s2);
+    private static boolean isOverridingWithSameReturnType(Symbol.MethodSymbol m1, Symbol.MethodSymbol m2) {
+      return m1.name().equals(m2.name())
+        && m1.returnType() == m2.returnType()
+        && ConfusingOverloadCheck.isPotentialOverride(m1, m2);
     }
-
   }
 
   private static boolean isMethodInvocationOnParameter(Symbol parameter, MethodInvocationTree mit) {
@@ -242,5 +247,4 @@ private static boolean isMethodInvocationOnParameter(Symbol parameter, MethodInv
   private static boolean isSpringInjectionAnnotated(SymbolMetadata metadata) {
     return SPRING_INJECT_ANNOTATIONS.stream().anyMatch(metadata::isAnnotatedWith);
   }
-
 }