Fix FP on S1149 on overriding methods body (#4400)

its/ruling/src/test/java/org/sonar/java/it/AutoScanTest.java

@@ -188,7 +188,7 @@ public void javaCheckTestSources() throws Exception {
      * No differences would mean that we find the same issues with and without the bytecode and libraries
      */
     String differences = Files.readString(pathFor(TARGET_ACTUAL + PROJECT_KEY + "-no-binaries_differences"));
-    assertThat(differences).isEqualTo("Issues differences: 3268");
+    assertThat(differences).isEqualTo("Issues differences: 3269");
   }
 
   private static Path pathFor(String path) {

its/ruling/src/test/resources/autoscan/autoscan-diff-by-rules.json

@@ -224,7 +224,7 @@
   {
     "ruleKey": "S1128",
     "hasTruePositives": true,
-    "falseNegatives": 31,
+    "falseNegatives": 32,
     "falsePositives": 0
   },
   {

its/ruling/src/test/resources/eclipse-jetty-similar-to-main/java-S1149.json

@@ -6,7 +6,4 @@
 298,
 310,
 ],
-'org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/Request.java':[
-1283,
-],
 }

its/ruling/src/test/resources/eclipse-jetty/java-S1149.json

@@ -6,9 +6,6 @@
 298,
 310,
 ],
-'org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/Request.java':[
-1283,
-],
 'org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/QuotedStringTokenizer.java':[
 45,
 286,

java-checks-test-sources/src/main/java/checks/SynchronizedClassUsageCheck.java

@@ -10,6 +10,9 @@
 import java.util.Vector; // Compliant
 
 public class SynchronizedClassUsageCheck {
+
+  public SynchronizedClassUsageCheck() {}
+
   interface IA {
     // Noncompliant@+2 {{Replace the synchronized class "Vector" by an unsynchronized one such as "ArrayList" or "LinkedList".}}
     // Noncompliant@+1 {{Replace the synchronized class "Vector" by an unsynchronized one such as "ArrayList" or "LinkedList".}}
@@ -40,6 +43,7 @@ class A implements IA {
 
     A(Vector v) { // Noncompliant {{Replace the synchronized class "Vector" by an unsynchronized one such as "ArrayList" or "LinkedList".}}
       a = v;
+      new antlr.collections.impl.Vector().capacity();
     }
 
     private static Hashtable foo() { // Noncompliant
@@ -68,8 +72,20 @@ public java.util.Stack f2() { // Noncompliant {{Replace the synchronized class "
 
     @Override
     public Vector f3(Vector a) { // Compliant
-      Vector b; // Noncompliant {{Replace the synchronized class "Vector" by an unsynchronized one such as "ArrayList" or "LinkedList".}}
-      return null;
+      Vector b = new Vector(); // Compliant: since Vector is part of the overriding signature, it is allowed to use inside the method body
+      Hashtable ht; // Noncompliant
+
+      class Anonymous implements IA{
+        @Override
+        public Vector f3(Vector a) {
+          Vector b = new Vector();
+          Hashtable ht; // Noncompliant
+          return null;
+        }
+      }
+
+      Vector b2 = new Vector();
+      return b;
     }
     public void f(Integer i) { // Compliant
     }

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

@@ -22,6 +22,7 @@
 import java.util.ArrayDeque;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.Deque;
 import java.util.HashSet;
 import java.util.List;
@@ -30,7 +31,6 @@
 import java.util.stream.Collectors;
 import org.sonar.check.Rule;
 import org.sonar.java.checks.helpers.ExpressionsHelper;
-import org.sonarsource.analyzer.commons.collections.MapBuilder;
 import org.sonar.plugins.java.api.IssuableSubscriptionVisitor;
 import org.sonar.plugins.java.api.semantic.Symbol;
 import org.sonar.plugins.java.api.semantic.Type;
@@ -43,6 +43,7 @@
 import org.sonar.plugins.java.api.tree.Tree;
 import org.sonar.plugins.java.api.tree.TypeTree;
 import org.sonar.plugins.java.api.tree.VariableTree;
+import org.sonarsource.analyzer.commons.collections.MapBuilder;
 
 @Rule(key = "S1149")
 public class SynchronizedClassUsageCheck extends IssuableSubscriptionVisitor {
@@ -81,7 +82,6 @@ public void visitNode(Tree tree) {
     tree.accept(new DeprecatedTypeVisitor());
   }
 
-
   @Override
   public void leaveNode(Tree tree) {
     if (tree.is(Tree.Kind.COMPILATION_UNIT)) {
@@ -117,6 +117,8 @@ private static boolean isMethodFromJavaPackage(String fqn) {
 
   private class DeprecatedTypeVisitor extends BaseTreeVisitor {
 
+    private Deque<Set<String>> overridingMethodTypes = new ArrayDeque<>();
+
     @Override
     public void visitClass(ClassTree tree) {
       TypeTree superClass = tree.superClass();
@@ -130,18 +132,32 @@ public void visitClass(ClassTree tree) {
     @Override
     public void visitMethod(MethodTree tree) {
       TypeTree returnTypeTree = tree.returnType();
-      if (isNotOverriding(tree) || returnTypeTree == null) {
-        if (returnTypeTree != null) {
+      boolean isConstructor = tree.is(Tree.Kind.CONSTRUCTOR);
+      if (isOverriding(tree) && !isConstructor) {
+        overridingMethodTypes.push(collectOverridingMethodExclusions(tree, returnTypeTree));
+      } else {
+        overridingMethodTypes.push(Collections.emptySet());
+        if (!isConstructor) {
           reportIssueOnDeprecatedType(returnTypeTree, returnTypeTree.symbolType());
         }
         scan(tree.parameters());
       }
       scan(tree.block());
+      overridingMethodTypes.pop();
     }
 
-    private boolean isNotOverriding(MethodTree tree) {
+    private Set<String> collectOverridingMethodExclusions(MethodTree methodTree, TypeTree returnType) {
+      var methodTypes = new HashSet<String>();
+      methodTypes.add(returnType.symbolType().fullyQualifiedName());
+      methodTree.parameters().stream()
+        .map(param -> param.type().symbolType().fullyQualifiedName())
+        .forEach(methodTypes::add);
+      return methodTypes;
+    }
+
+    private boolean isOverriding(MethodTree tree) {
       // In case it can not be determined (isOverriding returns null), return false to avoid FP.
-      return Boolean.FALSE.equals(tree.isOverriding());
+      return Boolean.TRUE.equals(tree.isOverriding());
     }
 
     @Override
@@ -159,7 +175,7 @@ public void visitLambdaExpression(LambdaExpressionTree lambdaExpressionTree) {
     }
 
     private boolean reportIssueOnDeprecatedType(Tree tree, Type type) {
-      if (visited.contains(tree)) {
+      if (visited.contains(tree) || isAllowedByOverridingSignature(type)) {
         return false;
       }
       visited.add(tree);
@@ -170,6 +186,10 @@ private boolean reportIssueOnDeprecatedType(Tree tree, Type type) {
       return false;
     }
 
+    private boolean isAllowedByOverridingSignature(Type type) {
+      return !overridingMethodTypes.isEmpty() && overridingMethodTypes.peek().contains(type.fullyQualifiedName());
+    }
+
     private boolean isDeprecatedType(Type symbolType) {
       if (symbolType.isClass()) {
         for (String deprecatedType : REPLACEMENTS.keySet()) {
@@ -180,5 +200,6 @@ private boolean isDeprecatedType(Type symbolType) {
       }
       return false;
     }
+
   }
 }

java-checks/src/test/java/org/sonar/java/checks/SynchronizedClassUsageCheckTest.java

@@ -22,32 +22,33 @@
 import java.util.Collections;
 import org.junit.jupiter.api.Test;
 import org.sonar.java.checks.verifier.CheckVerifier;
+import org.sonar.java.checks.verifier.internal.InternalCheckVerifier;
 
-import static org.sonar.java.checks.verifier.TestUtils.testSourcesPath;
+import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath;
 
 class SynchronizedClassUsageCheckTest {
-
+  
   @Test
   void detected() {
     CheckVerifier.newVerifier()
-      .onFile(testSourcesPath("checks/SynchronizedClassUsageCheck.java"))
+      .onFile(mainCodeSourcesPath("checks/SynchronizedClassUsageCheck.java"))
       .withCheck(new SynchronizedClassUsageCheck())
       .verifyIssues();
     CheckVerifier.newVerifier()
-      .onFile(testSourcesPath("checks/SynchronizedClassUsageByAPICheck.java"))
+      .onFile(mainCodeSourcesPath("checks/SynchronizedClassUsageByAPICheck.java"))
       .withCheck(new SynchronizedClassUsageCheck())
       .verifyIssues();
   }
 
   @Test
   void test_without_semantic() {
     CheckVerifier.newVerifier()
-      .onFile(testSourcesPath("checks/SynchronizedClassUsageCheck.java"))
+      .onFile(mainCodeSourcesPath("checks/SynchronizedClassUsageCheck.java"))
       .withCheck(new SynchronizedClassUsageCheck())
       .withClassPath(Collections.emptyList())
       .verifyIssues();
     CheckVerifier.newVerifier()
-      .onFile(testSourcesPath("checks/SynchronizedClassUsageByAPICheck.java"))
+      .onFile(mainCodeSourcesPath("checks/SynchronizedClassUsageByAPICheck.java"))
       .withCheck(new SynchronizedClassUsageCheck())
       .withClassPath(Collections.emptyList())
       .verifyIssues();