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

java/ql/integration-tests/java/query-suite/java-code-quality-extended.qls.expected

@@ -73,6 +73,7 @@ ql/java/ql/src/Violations of Best Practice/Exception Handling/IgnoreExceptionalR
 ql/java/ql/src/Violations of Best Practice/Exception Handling/NumberFormatException.ql
 ql/java/ql/src/Violations of Best Practice/Implementation Hiding/AbstractToConcreteCollection.ql
 ql/java/ql/src/Violations of Best Practice/Implementation Hiding/ExposeRepresentation.ql
+ql/java/ql/src/Violations of Best Practice/Implementation Hiding/VisibleForTestingAbuse.ql
 ql/java/ql/src/Violations of Best Practice/Naming Conventions/AmbiguousOuterSuper.ql
 ql/java/ql/src/Violations of Best Practice/Naming Conventions/ConfusingMethodNames.ql
 ql/java/ql/src/Violations of Best Practice/Naming Conventions/ConfusingOverloading.ql

java/ql/integration-tests/java/query-suite/java-code-quality.qls.expected

@@ -71,6 +71,7 @@ ql/java/ql/src/Violations of Best Practice/Exception Handling/IgnoreExceptionalR
 ql/java/ql/src/Violations of Best Practice/Exception Handling/NumberFormatException.ql
 ql/java/ql/src/Violations of Best Practice/Implementation Hiding/AbstractToConcreteCollection.ql
 ql/java/ql/src/Violations of Best Practice/Implementation Hiding/ExposeRepresentation.ql
+ql/java/ql/src/Violations of Best Practice/Implementation Hiding/VisibleForTestingAbuse.ql
 ql/java/ql/src/Violations of Best Practice/Naming Conventions/AmbiguousOuterSuper.ql
 ql/java/ql/src/Violations of Best Practice/Naming Conventions/ConfusingMethodNames.ql
 ql/java/ql/src/Violations of Best Practice/Naming Conventions/ConfusingOverloading.ql

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,129 @@
+/**
+ * @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 quality
+ *       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(Expr e, RefType t) {
+  e.getCompilationUnit().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, 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, 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, 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,17 @@
+| packageone/SourcePackage.java:9:21:9:32 | Annotated.m1 | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:9:29:9:30 | m1 | element |
+| packageone/SourcePackage.java:10:21:10:32 | Annotated.m2 | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:11:26:11:27 | m2 | element |
+| packageone/SourcePackage.java:16:18:16:36 | fPublic(...) | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:26:23:26:29 | fPublic | element |
+| packageone/SourcePackage.java:17:18:17:39 | fProtected(...) | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:31:26:31:35 | fProtected | element |
+| packageone/SourcePackage.java:25:31:25:42 | Annotated.m1 | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:9:29:9:30 | m1 | element |
+| packageone/SourcePackage.java:26:31:26:42 | Annotated.m2 | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:11:26:11:27 | m2 | element |
+| packageone/SourcePackage.java:29:28:29:46 | fPublic(...) | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:26:23:26:29 | fPublic | element |
+| packageone/SourcePackage.java:30:28:30:49 | fProtected(...) | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:31:26:31:35 | fProtected | element |
+| packagetwo/Annotated.java:49:31:49:31 | m | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:7:19:7:19 | m | element |
+| packagetwo/Annotated.java:54:26:54:28 | f(...) | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:16:16:16:16 | f | element |
+| packagetwo/Annotated.java:64:28:64:28 | m | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:7:19:7:19 | m | element |
+| packagetwo/Annotated.java:69:26:69:28 | f(...) | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:16:16:16: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:14:17:14:29 | f(...) | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:16:16:16:16 | f | element |
+| packagetwo/Source.java:20:28:20:47 | new AnnotatedClass(...) | Access of $@ annotated with VisibleForTesting found in production code. | packageone/AnnotatedClass.java:4:14:4:27 | AnnotatedClass | element |
+| packagetwo/Source.java:24:30:24:40 | Annotated.m | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:7:19:7:19 | m | element |
+| packagetwo/Source.java:28:27:28:39 | f(...) | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:16:16:16:16 | f | element |

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

@@ -0,0 +1,2 @@
+query: Violations of Best Practice/Implementation Hiding/VisibleForTestingAbuse.ql
+postprocess: utils/test/InlineExpectationsTestQuery.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,34 @@
+package packageone;
+
+import packagetwo.Annotated;
+
+public class SourcePackage extends Annotated {
+    void f() {
+        // Fields - cross-package access (only accessible ones)
+        // String s = Annotated.m; // Cannot access package-private from different package
+        String s1 = Annotated.m1; // $ Alert
+        String s2 = Annotated.m2; // $ Alert
+        // String s3 = Annotated.m3; // Cannot access private field
+
+        // Methods - cross-package access (only accessible ones)
+        // int i = Annotated.f(); // Cannot access package-private from different package
+        // int i1 = Annotated.fPrivate(); // Cannot access private method
+        int i2 = Annotated.fPublic(); // $ Alert
+        int i3 = Annotated.fProtected(); // $ Alert
+
+        // Same package class
+        AnnotatedClass a = new AnnotatedClass(); // COMPLIANT - same package
+
+        // Lambda usage - cross-package (only accessible members)
+        Runnable lambda = () -> {
+            // String lambdaS = Annotated.m; // Cannot access package-private
+            String lambdaS1 = Annotated.m1; // $ Alert
+            String lambdaS2 = Annotated.m2; // $ Alert
+
+            // int lambdaI = Annotated.f(); // Cannot access package-private
+            int lambdaI2 = Annotated.fPublic(); // $ Alert
+            int lambdaI3 = Annotated.fProtected(); // $ Alert
+        };
+        lambda.run();
+    }
+}

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,75 @@
+package packagetwo;
+
+import packageone.*;
+
+public class Annotated {
+    @VisibleForTesting
+    static String m;
+    @VisibleForTesting
+    static protected String m1;
+    @VisibleForTesting
+    static public String m2;
+    @VisibleForTesting
+    static private String m3;
+
+    @VisibleForTesting
+    static int f() {
+        return 1;
+    }
+
+    @VisibleForTesting
+    static private int fPrivate() {
+        return 1;
+    }
+
+    @VisibleForTesting
+    static public int fPublic() {
+        return 1;
+    }
+
+    @VisibleForTesting
+    static protected int fProtected() {
+        return 1;
+    }
+
+    private static void resetPriorities() {
+        String priority = m;
+        String priority1 = m1;
+        String priority2 = m2;
+        String priority3 = m3;
+
+        int result = f();
+        int resultPrivate = fPrivate();
+        int resultPublic = fPublic();
+        int resultProtected = fProtected();
+    }
+
+    private static void resetPriorities2() {
+        Runnable task = () -> {
+            String priority = m; // $ SPURIOUS: Alert
+            String priority1 = m1;
+            String priority2 = m2;
+            String priority3 = m3;
+
+            int result = f(); // $ SPURIOUS: Alert
+            int resultPrivate = fPrivate();
+            int resultPublic = fPublic();
+            int resultProtected = fProtected();
+        };
+        task.run();
+    }
+
+    private static class InnerClass {
+        void useVisibleForMembers() {
+            String field = m; // $ SPURIOUS: Alert
+            String field1 = m1;
+            String field2 = m2;
+            String field3 = m3;
+
+            int method = f(); // $ SPURIOUS: Alert
+            int methodPrivate = fPrivate();
+            int methodPublic = fPublic();
+            int methodProtected = fProtected();
+        }
+    }
+}

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

@@ -0,0 +1,34 @@
+package packagetwo;
+
+import packageone.*;
+
+public class Source {
+    void f() {
+        // Fields
+        String s = Annotated.m; // $ Alert
+        String s1 = Annotated.m1; // COMPLIANT - same package
+        String s2 = Annotated.m2;
+        // String s3 = Annotated.m3; // Cannot access private field
+
+        // Methods
+        int i = Annotated.f(); // $ Alert
+        // int i1 = Annotated.fPrivate(); // Cannot access private method
+        int i2 = Annotated.fPublic();
+        int i3 = Annotated.fProtected();
+
+        // Other class
+        AnnotatedClass a = new AnnotatedClass(); // $ Alert
+
+        // Lambda usage
+        Runnable lambda = () -> {
+            String lambdaS = Annotated.m; // $ Alert
+            String lambdaS1 = Annotated.m1;
+            String lambdaS2 = Annotated.m2;
+
+            int lambdaI = Annotated.f(); // $ Alert
+            int lambdaI2 = Annotated.fPublic();
+            int lambdaI3 = Annotated.fProtected();
+        };
+        lambda.run();
+    }
+}