SONARJAVA-5115 FP in S5803: when using (VisibleForTesting.PROTECTED) (#…

…4861)

java-checks-test-sources/default/src/main/java/checks/VisibleForTestingProtectedUsageCheck/CheckOtherwiseString/IssueStringMyObject.java

@@ -0,0 +1,14 @@
+package checks.VisibleForTestingProtectedUsageCheck.CheckOtherwiseString;
+
+public class IssueStringMyObject {
+
+  IssueStringMyObject() {
+  }
+
+  @VisibleForTesting(otherwise = VisibleForTesting.PROTECTED)
+  String bar;
+
+  @VisibleForTesting(otherwise = "3")
+  String foo;
+
+}

java-checks-test-sources/default/src/main/java/checks/VisibleForTestingProtectedUsageCheck/CheckOtherwiseString/IssueStringService.java

@@ -0,0 +1,13 @@
+package checks.VisibleForTestingProtectedUsageCheck.CheckOtherwiseString;
+
+public class IssueStringService {
+
+  public String test() {
+    return new IssueStringMyObject().foo; // Noncompliant {{Remove this usage of "foo", it is annotated with @VisibleForTesting and should not be accessed from production code.}}
+  }
+
+  public String test2() {
+    return new IssueStringMyObject().bar; // Noncompliant {{Remove this usage of "bar", it is annotated with @VisibleForTesting and should not be accessed from production code.}}
+  }
+
+}

java-checks-test-sources/default/src/main/java/checks/VisibleForTestingProtectedUsageCheck/CheckOtherwiseString/VisibleForTesting.java

@@ -0,0 +1,16 @@
+package checks.VisibleForTestingProtectedUsageCheck.CheckOtherwiseString;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.Target;
+
+import static java.lang.annotation.ElementType.FIELD;
+import static java.lang.annotation.ElementType.METHOD;
+import static java.lang.annotation.ElementType.TYPE;
+import static java.lang.annotation.RetentionPolicy.CLASS;
+
+@Retention(value = CLASS)
+@Target(value = {TYPE, METHOD, FIELD})
+public @interface VisibleForTesting {
+  String otherwise();
+  String PROTECTED = "4";
+}

java-checks-test-sources/default/src/main/java/checks/VisibleForTestingProtectedUsageCheck/IssueMyObject.java

@@ -0,0 +1,17 @@
+package checks.VisibleForTestingProtectedUsageCheck;
+
+public class IssueMyObject {
+
+  IssueMyObject() {
+  }
+
+  @VisibleForTesting(otherwise = VisibleForTesting.PROTECTED, othertestcase=1, othertypecase="S")
+  String bar;
+
+  // androidx.annotation.VisibleForTesting
+  // @VisibleForTesting(otherwise = VisibleForTesting.PROTECTED)
+  // where VisibleForTesting.PROTECTED = 4
+  @VisibleForTesting(otherwise = 3, othertestcase=1, othertypecase="F")
+  String foo;
+
+}

java-checks-test-sources/default/src/main/java/checks/VisibleForTestingProtectedUsageCheck/IssueService.java

@@ -0,0 +1,13 @@
+package checks.VisibleForTestingProtectedUsageCheck;
+
+public class IssueService {
+
+  public String test() {
+    return new IssueMyObject().foo; // Noncompliant {{Remove this usage of "foo", it is annotated with @VisibleForTesting and should not be accessed from production code.}}
+  }
+
+  public String test2() {
+    return new IssueMyObject().bar; // Compliant {{bar has valid otherwise = VisibleForTesting.PROTECTED set so can be used here}}
+  }
+
+}

java-checks-test-sources/default/src/main/java/checks/VisibleForTestingProtectedUsageCheck/MyObject.java

@@ -0,0 +1,30 @@
+package checks.VisibleForTestingProtectedUsageCheck;
+
+public class MyObject {
+
+  MyObject() {}
+
+  // androidx.annotation.VisibleForTesting
+  // @VisibleForTesting(otherwise = VisibleForTesting.PROTECTED)
+  // where VisibleForTesting.PROTECTED = 4
+  @VisibleForTesting(otherwise = 4, othertestcase=1, othertypecase="F")
+  String foo;
+
+  @VisibleForTesting(otherwise = 4, othertestcase=1, othertypecase="F")
+  int answer() {
+    return 42;
+  }
+
+  int answer(int result) {
+    return result;
+  }
+
+  @VisibleForTesting(otherwise = 4, othertestcase=1, othertypecase="F")
+  class Nested {}
+}
+
+@VisibleForTesting(otherwise = 4, othertestcase=1, othertypecase="F")
+class Outer {}
+
+
+

java-checks-test-sources/default/src/main/java/checks/VisibleForTestingProtectedUsageCheck/Service.java

@@ -0,0 +1,45 @@
+package checks.VisibleForTestingProtectedUsageCheck;
+
+import com.google.common.annotations.VisibleForTesting;
+
+public class Service {
+
+  public String f(int param) {
+
+    TestOnly testOnly = null; // Compliant, TestOnly class visible if it is private
+
+    MyObject.Nested nested = null; // Noncompliant {{Remove this usage of "Nested", it is annotated with @VisibleForTesting and should not be accessed from production code.}}
+
+    Outer outer = null; // Noncompliant {{Remove this usage of "Outer", it is annotated with @VisibleForTesting and should not be accessed from production code.}}
+
+    String foo = new MyObj().bar; // False negative MyObj and Service are in the same file but if 'bar' is private it wouldn't be visible here)
+
+    String bar = new MyObj().bar;
+
+    return new MyObject().foo; // Already reported in line 21
+
+  }
+
+  public int g(@Deprecated int param) {
+    MyObject myObject = new MyObject();
+
+    myObject.answer(123); // Compliant, no annotation
+    myObject.answer(param); // Compliant, no annotation
+
+    return myObject.answer(); // Noncompliant {{Remove this usage of "answer", it is annotated with @VisibleForTesting and should not be accessed from production code.}}
+  }
+
+}
+
+class MyObj {
+  @VisibleForTesting
+  String bar;
+}
+
+
+@VisibleForTesting
+class TestOnly {
+
+}
+
+

java-checks-test-sources/default/src/main/java/checks/VisibleForTestingProtectedUsageCheck/VisibleForTesting.java

@@ -0,0 +1,18 @@
+package checks.VisibleForTestingProtectedUsageCheck;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.Target;
+
+import static java.lang.annotation.ElementType.FIELD;
+import static java.lang.annotation.ElementType.METHOD;
+import static java.lang.annotation.ElementType.TYPE;
+import static java.lang.annotation.RetentionPolicy.CLASS;
+
+@Retention(value = CLASS)
+@Target(value = {TYPE, METHOD, FIELD})
+public @interface VisibleForTesting {
+  int otherwise();
+  int othertestcase();
+  String othertypecase();
+  int PROTECTED = 4;
+}

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

@@ -28,6 +28,7 @@
 import org.sonar.plugins.java.api.IssuableSubscriptionVisitor;
 import org.sonar.plugins.java.api.JavaFileScannerContext;
 import org.sonar.plugins.java.api.semantic.Symbol;
+import org.sonar.plugins.java.api.semantic.SymbolMetadata;
 import org.sonar.plugins.java.api.semantic.SymbolMetadata.AnnotationInstance;
 import org.sonar.plugins.java.api.tree.IdentifierTree;
 import org.sonar.plugins.java.api.tree.Tree;
@@ -70,7 +71,21 @@ private static boolean isMisusedVisibleForTesting(Symbol symbol) {
   }
 
   private static boolean isVisibleForTestingAnnotation(AnnotationInstance annotationInstance) {
-    return "VisibleForTesting".equals(annotationInstance.symbol().name());
+    return "VisibleForTesting".equals(annotationInstance.symbol().name())
+      && !isOtherwiseProtected(annotationInstance);
+  }
+
+  private static boolean isOtherwiseProtected(AnnotationInstance annotationInstance) {
+    List<SymbolMetadata.AnnotationValue> values = annotationInstance.values();
+    for (SymbolMetadata.AnnotationValue value : values) {
+      // Note: constant to support is androidx.annotation.VisibleForTesting.PROTECTED=4
+      if ("otherwise".equals(value.name()) &&
+        value.value() instanceof Integer &&
+        Integer.valueOf(4).equals(value.value())) {
+        return true;
+      }
+    }
+    return false;
   }
 
   private static boolean inTheSameFile(Symbol symbol) {

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

@@ -34,6 +34,29 @@ void test() {
       .verifyIssues();
   }
 
+  @Test
+  void test_protected() {
+    // no issues
+    CheckVerifier.newVerifier()
+      .onFile(mainCodeSourcesPath("checks/VisibleForTestingProtectedUsageCheck/Service.java"))
+      .withCheck(new VisibleForTestingUsageCheck())
+      .verifyNoIssues();
+    // issues
+    CheckVerifier.newVerifier()
+      .onFile(mainCodeSourcesPath("checks/VisibleForTestingProtectedUsageCheck/IssueService.java"))
+      .withCheck(new VisibleForTestingUsageCheck())
+      .verifyIssues();
+  }
+
+  @Test
+  void test_protected_string() {
+    // issues
+    CheckVerifier.newVerifier()
+      .onFile(mainCodeSourcesPath("checks/VisibleForTestingProtectedUsageCheck/CheckOtherwiseString/IssueStringService.java"))
+      .withCheck(new VisibleForTestingUsageCheck())
+      .verifyIssues();
+  }
+
   @Test
   void test_no_semantic() {
     CheckVerifier.newVerifier()