SONARJAVA-4238 S2924 should not report on non-private rules declared inside of abstract classes (#4886)

Author: pauloreilly-sonarsource

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

@@ -1046,7 +1046,7 @@
   {
     "ruleKey": "S2187",
     "hasTruePositives": true,
-    "falseNegatives": 11,
+    "falseNegatives": 12,
     "falsePositives": 1
   },
   {
@@ -1400,7 +1400,7 @@
   {
     "ruleKey": "S2699",
     "hasTruePositives": true,
-    "falseNegatives": 143,
+    "falseNegatives": 151,
     "falsePositives": 1
   },
   {
@@ -1466,7 +1466,7 @@
   {
     "ruleKey": "S2924",
     "hasTruePositives": false,
-    "falseNegatives": 8,
+    "falseNegatives": 11,
     "falsePositives": 0
   },
   {
@@ -1682,7 +1682,7 @@
   {
     "ruleKey": "S3577",
     "hasTruePositives": true,
-    "falseNegatives": 44,
+    "falseNegatives": 46,
     "falsePositives": 0
   },
   {

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

@@ -1,6 +1,6 @@
 {
   "ruleKey": "S2187",
   "hasTruePositives": true,
-  "falseNegatives": 11,
+  "falseNegatives": 12,
   "falsePositives": 1
-}
+}

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

@@ -1,6 +1,6 @@
 {
   "ruleKey": "S2699",
   "hasTruePositives": true,
-  "falseNegatives": 150,
+  "falseNegatives": 151,
   "falsePositives": 1
-}
+}

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

@@ -1,6 +1,6 @@
 {
   "ruleKey": "S2924",
   "hasTruePositives": false,
-  "falseNegatives": 8,
+  "falseNegatives": 11,
   "falsePositives": 0
-}
+}

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

@@ -1,6 +1,6 @@
 {
   "ruleKey": "S3577",
   "hasTruePositives": true,
-  "falseNegatives": 45,
+  "falseNegatives": 46,
   "falsePositives": 0
-}
+}

java-checks-test-sources/default/src/test/java/checks/tests/UnusedTestRuleCheck_Protected.java

@@ -0,0 +1,47 @@
+package checks.tests;
+
+import java.nio.file.Path;
+import org.junit.jupiter.api.io.TempDir;
+
+// https://community.sonarsource.com/t/possible-fp-for-java-tempdir-declared-in-super-class/60836
+// https://sonarsource.atlassian.net/browse/SONARJAVA-4238
+public abstract class UnusedTestRuleCheck_Protected {
+  @TempDir
+  protected Path tempDirOldFP; // Compliant FP as used in a subclass - compliant because not private
+}
+
+// Test abstract private
+abstract class AbstractTestCase {
+
+  @TempDir
+  private Path tempDir; // Noncompliant {{Remove this unused "TempDir".}}
+  // increases AutoScan FNs
+
+  void test() {
+  }
+
+}
+
+// Test non-abstract private
+class ClassTestCase { // increases AutoScan FNs
+
+  @TempDir
+  private Path tempDir; // Noncompliant {{Remove this unused "TempDir".}}
+  // increases AutoScan FNs
+
+  void test() {
+  }
+
+}
+
+// Test non-abstract protected
+class ClassTestCase2 {
+
+  @TempDir
+  protected Path tempDir; // Noncompliant {{Remove this unused "TempDir".}}
+  // increases AutoScan FNs
+
+  void test() {
+  }
+
+}

java-checks-test-sources/default/src/test/java/checks/tests/UnusedTestRuleCheck_UseProtected.java

@@ -0,0 +1,17 @@
+package checks.tests;
+
+import java.nio.file.Files;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+// AutoScan S3577 Increases FN to 46
+public class UnusedTestRuleCheck_UseProtected extends UnusedTestRuleCheck_Protected {
+  @BeforeEach
+  void setup() throws Exception {
+    Files.createTempFile(tempDirOldFP, "test", "");
+  }
+  @Test
+  void test() {
+    // AutoScan S2699 Increases FN to 151
+  }
+}

java-checks/src/main/java/org/sonar/java/checks/unused/UnusedTestRuleCheck.java

@@ -23,13 +23,15 @@
 import java.util.List;
 import java.util.Set;
 import org.sonar.check.Rule;
-import org.sonarsource.analyzer.commons.collections.SetUtils;
+import org.sonar.java.model.ModifiersUtils;
 import org.sonar.plugins.java.api.IssuableSubscriptionVisitor;
 import org.sonar.plugins.java.api.semantic.Symbol;
 import org.sonar.plugins.java.api.tree.ClassTree;
 import org.sonar.plugins.java.api.tree.MethodTree;
+import org.sonar.plugins.java.api.tree.Modifier;
 import org.sonar.plugins.java.api.tree.Tree;
 import org.sonar.plugins.java.api.tree.VariableTree;
+import org.sonarsource.analyzer.commons.collections.SetUtils;
 
 @Rule(key = "S2924")
 public class UnusedTestRuleCheck extends IssuableSubscriptionVisitor {
@@ -47,11 +49,16 @@ public List<Tree.Kind> nodesToVisit() {
   @Override
   public void visitNode(Tree tree) {
     ClassTree classTree = (ClassTree) tree;
+    boolean isAbstract = ModifiersUtils.hasModifier(classTree.modifiers(), Modifier.ABSTRACT);
     for (Tree member : classTree.members()) {
       if (member.is(Tree.Kind.VARIABLE)) {
         VariableTree variableTree = (VariableTree) member;
         Symbol symbol = variableTree.symbol();
         if ((isTestNameOrTemporaryFolderRule(symbol) || hasTempDirAnnotation(symbol)) && symbol.usages().isEmpty()) {
+          // if class is abstract, then we need to check modifier - if not private, then it's okay
+          if (isAbstract && !ModifiersUtils.hasModifier(variableTree.modifiers(), Modifier.PRIVATE)) {
+            continue;
+          }
           reportIssue(variableTree.simpleName(), "Remove this unused \"" + getSymbolType(symbol) + "\".");
         }
       } else if (member.is(Tree.Kind.METHOD, Tree.Kind.CONSTRUCTOR)) {

java-checks/src/test/java/org/sonar/java/checks/unused/UnusedTestRuleCheckTest.java

@@ -42,6 +42,14 @@ void test_JUnit5() {
       .verifyIssues();
   }
 
+  @Test
+  void test_UseProtected() {
+    CheckVerifier.newVerifier()
+      .onFile(testCodeSourcesPath("checks/tests/UnusedTestRuleCheck_Protected.java"))
+      .withCheck(new UnusedTestRuleCheck())
+      .verifyIssues();
+  }
+
   @Test
   void test_no_issues_without_semantic() {
     CheckVerifier.newVerifier()