Java: Added new query java/visible-for-testing-abuse

Author: Napalys

java/ql/src/Violations of Best Practice/Implementation Hiding/VisibleForTestingAbuse.md

@@ -0,0 +1,39 @@
+# J-T-003: Accessing any method, field or class annotated with `@VisibleForTesting` from production code is discouraged
+
+Accessing class members annotated with `@VisibleForTesting` from production code goes against the intention of the annotation and may indicate programmer error.
+
+## Overview
+
+The `@VisibleForTesting` serves to increase visibility of methods, fields or classes for the purposes of testing. Accessing methods, fields or classes that are annotated with `@VisibleForTesting` in production code (not test code) abuses the intention of the annotation.
+
+## Recommendation
+
+Only access methods, fields or classes annotated with `@VisibleForTesting` from test code. If the visibility of the methods, fields or classes should generally be relaxed, use Java language access modifiers.
+
+## Example
+
+```java
+public class Annotated {
+@VisibleForTesting static int f(){}
+}
+
+/* src/test/java/Test.java */
+int i = Annotated.f(); // COMPLIANT
+
+/* src/main/Source.java */
+ int i = Annotated.f(); // NON_COMPLIANT
+
+```
+
+## Implementation notes
+
+This rule alerts on any implementation of the annotation `VisibleForTesting`, regardless of where it is provided from.
+
+The rule also uses the following logic to determine what an abuse of the annotation is:
+
+  1) If public or protected member/type is annotated with `VisibleForTesting`, it's assumed that package-private access is enough for production code. Therefore the rule alerts when a public or protected member/type annotated with `VisibleForTesting` is used outside of its declaring package.
+  2) If package-private member/type is annotated with `VisibleForTesting`, it's assumed that private access is enough for production code. Therefore the rule alerts when a package-private member/type annotated with `VisibleForTesting` is used outside its declaring class.
+
+## References
+- Example Specific Implementation of a VisibleForTesting Annotation: [AssertJ VisibleForTesting](https://javadoc.io/doc/org.assertj/assertj-core/latest/org/assertj/core/util/VisibleForTesting.html)
+- Assumptions of what level of access is permittable for each access modifier and the annotation: [JetBrains VisibleForTesting](https://javadoc.io/doc/org.jetbrains/annotations/22.0.0/org/jetbrains/annotations/VisibleForTesting.html)

java/ql/src/Violations of Best Practice/Implementation Hiding/VisibleForTestingAbuse.ql

@@ -0,0 +1,128 @@
+/**
+ * @id java/visible-for-testing-abuse
+ * @name Accessing any method, field or class annotated with `@VisibleForTesting` from production code is discouraged
+ * @description Accessing any method, field or class annotated with `@VisibleForTesting` from
+ *              production code goes against the intention of the annotation and may indicate
+ *              programmer error.
+ * @kind problem
+ * @precision high
+ * @problem.severity warning
+ * @tags maintainability
+ *       readability
+ */
+
+import java
+
+/**
+ * A `Callable` is within some `RefType`
+ */
+predicate isWithinType(Callable c, RefType t) { c.getDeclaringType() = t }
+
+/**
+ * A `Callable` is within same package as the `RefType`
+ */
+predicate isWithinPackage(Callable c, RefType t) {
+  c.getDeclaringType().getPackage() = t.getPackage()
+}
+
+predicate withinStaticContext(NestedClass c) {
+  c.isStatic() or
+  c.(AnonymousClass).getClassInstanceExpr().getEnclosingCallable().isStatic() // JLS 15.9.2
+}
+
+RefType enclosingInstanceType(Class inner) {
+  not withinStaticContext(inner) and
+  result = inner.(NestedClass).getEnclosingType()
+}
+
+class OuterClass extends Class {
+  OuterClass() { this = enclosingInstanceType+(_) }
+}
+
+/**
+ * An innerclass is accessed outside of its outerclass
+ * and also outside of its fellow inner parallel classes
+ */
+predicate isWithinDirectOuterClassOrSiblingInner(
+  Callable classInstanceEnclosing, RefType typeBeingConstructed
+) {
+  exists(NestedClass inner, OuterClass outer |
+    outer = enclosingInstanceType(inner) and
+    typeBeingConstructed = inner and
+    // where the inner is called from the outer class
+    classInstanceEnclosing.getDeclaringType() = outer
+  )
+  or
+  // and inner is called from the a parallel inner
+  exists(NestedClass inner, OuterClass outer, NestedClass otherinner |
+    typeBeingConstructed = inner and
+    outer = enclosingInstanceType(otherinner) and
+    outer = enclosingInstanceType(inner) and
+    classInstanceEnclosing.getDeclaringType() = otherinner
+  )
+}
+
+from Annotatable annotated, Annotation annotation, Expr e
+where
+  annotation.getType().hasName("VisibleForTesting") and
+  annotated.getAnAnnotation() = annotation and
+  (
+    // field access
+    exists(FieldAccess v |
+      v = e and
+      v.getField() = annotated and
+      // depending on the visiblity of the field, using the annotation to abuse the visibility may/may not be occurring
+      (
+        // if its package protected report when its used outside its class bc it should have been private (class only permitted)
+        v.getField().isPackageProtected() and
+        not isWithinType(v.getEnclosingCallable(), v.getField().getDeclaringType())
+        or
+        // if public or protected report when its used outside its package because package protected should have been enough (package only permitted)
+        (v.getField().isPublic() or v.getField().isProtected()) and
+        not isWithinPackage(v.getEnclosingCallable(), v.getField().getDeclaringType())
+      )
+    )
+    or
+    // class instantiation
+    exists(ClassInstanceExpr c |
+      c = e and
+      c.getConstructedType() = annotated and
+      // depending on the visiblity of the class, using the annotation to abuse the visibility may/may not be occurring
+      // if public report when its used outside its package because package protected should have been enough (package only permitted)
+      (
+        c.getConstructedType().isPublic() and
+        not isWithinPackage(c.getEnclosingCallable(), c.getConstructedType())
+        or
+        // if its package protected report when its used outside its outer class bc it should have been private (outer class only permitted)
+        c.getConstructedType().hasNoModifier() and
+        // and the class is an innerclass, because otherwise recommending a lower accessibility makes no sense (only inner classes can be private)
+        exists(enclosingInstanceType(c.getConstructedType())) and
+        not isWithinDirectOuterClassOrSiblingInner(c.getEnclosingCallable(), c.getConstructedType())
+      )
+    )
+    or
+    // method access
+    exists(MethodCall c |
+      c = e and
+      c.getMethod() = annotated and
+      // depending on the visiblity of the method, using the annotation to abuse the visibility may/may not be occurring
+      (
+        // if its package protected report when its used outside its class bc it should have been private (class only permitted)
+        c.getMethod().isPackageProtected() and
+        not isWithinType(c.getEnclosingCallable(), c.getMethod().getDeclaringType())
+        or
+        // if public or protected report when its used outside its package because package protected should have been enough (package only permitted)
+        (c.getMethod().isPublic() or c.getMethod().isProtected()) and
+        not isWithinPackage(c.getEnclosingCallable(), c.getMethod().getDeclaringType())
+      )
+    )
+  ) and
+  // not in a test where use is appropriate
+  not e.getEnclosingCallable() instanceof LikelyTestMethod and
+  // also omit our own ql unit test where it is acceptable
+  not e.getEnclosingCallable()
+      .getFile()
+      .getAbsolutePath()
+      .matches("%java/ql/test/query-tests/%Test.java")
+select e, "Access of $@ annotated with VisibleForTesting found in production code.", annotated,
+  "element"

java/ql/test/query-tests/VisibleForTestingAbuse/VisibleForTestingAbuse.expected

@@ -0,0 +1,4 @@
+| packageone/SourcePackage.java:8:21:8:32 | Annotated.m1 | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:9:29:9:30 | m1 | element |
+| packagetwo/Source.java:7:17:7:29 | f(...) | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:12:16:12:16 | f | element |
+| packagetwo/Source.java:8:20:8:30 | Annotated.m | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:7:19:7:19 | m | element |
+| packagetwo/Source.java:9:28:9:47 | new AnnotatedClass(...) | Access of $@ annotated with VisibleForTesting found in production code. | packageone/AnnotatedClass.java:4:14:4:27 | AnnotatedClass | element |

java/ql/test/query-tests/VisibleForTestingAbuse/VisibleForTestingAbuse.qlref

@@ -0,0 +1 @@
+Violations of Best Practice/Implementation Hiding/VisibleForTestingAbuse.ql

java/ql/test/query-tests/VisibleForTestingAbuse/packageone/AnnotatedClass.java

@@ -0,0 +1,6 @@
+package packageone;
+
+@VisibleForTesting
+public class AnnotatedClass {
+    public AnnotatedClass() {}
+}

java/ql/test/query-tests/VisibleForTestingAbuse/packageone/SourcePackage.java

@@ -0,0 +1,10 @@
+package packageone;
+
+import packagetwo.Annotated;
+
+public class SourcePackage extends Annotated {
+    void f() {
+        AnnotatedClass a = new AnnotatedClass(); // COMPLIANT - same package
+        String s1 = Annotated.m1; // NON_COMPLIANT
+    }
+}

java/ql/test/query-tests/VisibleForTestingAbuse/packageone/VisibleForTesting.java

@@ -0,0 +1,4 @@
+package packageone;
+
+public @interface VisibleForTesting {
+}

java/ql/test/query-tests/VisibleForTestingAbuse/packagetwo/Annotated.java

@@ -0,0 +1,15 @@
+package packagetwo;
+
+import packageone.*;
+
+public class Annotated {
+    @VisibleForTesting
+    static String m;
+    @VisibleForTesting
+    static protected String m1;
+
+    @VisibleForTesting
+    static int f() {
+        return 1;
+    }
+}

java/ql/test/query-tests/VisibleForTestingAbuse/packagetwo/Source.java

@@ -0,0 +1,12 @@
+package packagetwo;
+
+import packageone.*;
+
+public class Source {
+    void f() {
+        int i = Annotated.f(); // NON_COMPLIANT
+        String s = Annotated.m; // NON_COMPLIANT
+        AnnotatedClass a = new AnnotatedClass(); // NON_COMPLIANT
+        String s1 = Annotated.m1; // COMPLIANT - same package
+    }
+}

java/ql/test/query-tests/VisibleForTestingAbuse/packagetwo/Test.java

@@ -0,0 +1,12 @@
+package packagetwo;
+
+import packageone.*;
+
+public class Test {
+    void f() {
+        int i = Annotated.f(); // COMPLIANT
+        String s = Annotated.m; // COMPLIANT
+        AnnotatedClass a = new AnnotatedClass(); // COMPLIANT
+        String s1 = Annotated.m1; // COMPLIANT
+    }
+}