Java: Refactor VisibleForTestingAbuse query to reduce complexity

Author: Napalys

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

@@ -42,56 +42,9 @@ predicate isWithinVisibleForTestingContext(Callable c) {
   isWithinVisibleForTestingContext(c.getEnclosingCallable())
 }
 
-/**
- * Holds if a nested class is within a static context
- */
-predicate withinStaticContext(NestedClass c) {
-  c.isStatic() or
-  c.(AnonymousClass).getClassInstanceExpr().getEnclosingCallable().isStatic() // JLS 15.9.2
-}
-
-/**
- * Gets the enclosing instance type for a non-static inner class
- */
-RefType enclosingInstanceType(Class inner) {
-  not withinStaticContext(inner) and
-  result = inner.(NestedClass).getEnclosingType()
-}
-
-/**
- * A class that encloses one or more inner classes
- */
-class OuterClass extends Class {
-  OuterClass() { this = enclosingInstanceType+(_) }
-}
-
-/**
- * Holds if 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
+from Annotatable annotated, Expr e
 where
-  annotation.getType().hasName("VisibleForTesting") and
-  annotated.getAnAnnotation() = annotation and
+  annotated.getAnAnnotation().getType().hasName("VisibleForTesting") and
   (
     // field access
     exists(FieldAccess v |
@@ -109,24 +62,6 @@ where
       )
     )
     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
@@ -142,6 +77,19 @@ where
         not isWithinPackage(c, c.getMethod().getDeclaringType())
       )
     )
+    or
+    // Class instantiation - report if used outside appropriate scope
+    exists(ClassInstanceExpr c |
+      c = e and
+      c.getConstructedType() = annotated and
+      (
+        c.getConstructedType().isPublic() and not isWithinPackage(c, c.getConstructedType())
+        or
+        c.getConstructedType().hasNoModifier() and
+        c.getConstructedType() instanceof NestedClass and
+        not isWithinType(c.getEnclosingCallable(), c.getConstructedType())
+      )
+    )
   ) and
   // not in a test where use is appropriate
   not e.getEnclosingCallable() instanceof LikelyTestMethod and