From ec6a8b933c3639fa0eddba7feab88809d305978e Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Thu, 14 Oct 2021 14:36:14 +0100
Subject: [PATCH] Query for finding missing or unwanted bidirectional imports
 of abstract classes

---
 ql/src/codeql_ql/ast/Ast.qll                  |  3 +
 ql/src/codeql_ql/ast/internal/Module.qll      |  8 +-
 .../performance/AbstractClassImport.ql        | 82 +++++++++++++++++++
 3 files changed, 92 insertions(+), 1 deletion(-)
 create mode 100644 ql/src/queries/performance/AbstractClassImport.ql

diff --git a/ql/src/codeql_ql/ast/Ast.qll b/ql/src/codeql_ql/ast/Ast.qll
index 2c93da2af47..69b41b35188 100644
--- a/ql/src/codeql_ql/ast/Ast.qll
+++ b/ql/src/codeql_ql/ast/Ast.qll
@@ -804,6 +804,9 @@ class Class extends TClass, TypeDeclaration, ModuleDeclaration {
       result = this.getClassPredicate(name)
     )
   }
+
+  /** Holds if this class is abstract. */
+  predicate isAbstract() { hasAnnotation(this, "abstract") }
 }
 
 /**
diff --git a/ql/src/codeql_ql/ast/internal/Module.qll b/ql/src/codeql_ql/ast/internal/Module.qll
index a64ccfd0f2d..9cae791e410 100644
--- a/ql/src/codeql_ql/ast/internal/Module.qll
+++ b/ql/src/codeql_ql/ast/internal/Module.qll
@@ -19,7 +19,9 @@ private class ContainerOrModule extends TContainerOrModule {
 private class TFileOrModule = TFile or TModule;
 
 /** A file or a module. */
-class FileOrModule extends TFileOrModule, ContainerOrModule { }
+class FileOrModule extends TFileOrModule, ContainerOrModule {
+  abstract File getFile();
+}
 
 private class File_ extends FileOrModule, TFile {
   File f;
@@ -41,6 +43,8 @@ private class File_ extends FileOrModule, TFile {
     endline = 0 and
     endcolumn = 0
   }
+
+  override File getFile() { result = f }
 }
 
 private class Folder_ extends ContainerOrModule, TFolder {
@@ -90,6 +94,8 @@ class Module_ extends FileOrModule, TModule {
   ) {
     m.getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
   }
+
+  override File getFile() { result = m.getLocation().getFile() }
 }
 
 private predicate resolveQualifiedName(Import imp, ContainerOrModule m, int i) {
diff --git a/ql/src/queries/performance/AbstractClassImport.ql b/ql/src/queries/performance/AbstractClassImport.ql
new file mode 100644
index 00000000000..bccad253561
--- /dev/null
+++ b/ql/src/queries/performance/AbstractClassImport.ql
@@ -0,0 +1,82 @@
+/**
+ * @name Bidirectional imports for abstract classes
+ * @description An abstract class should import each of its subclasses, unless it is meant as an extension point, in which case it should import none of them.
+ * @kind problem
+ * @problem.severity error
+ * @id ql/abstract-class-import
+ * @tags correctness
+ *       performance
+ * @precision high
+ */
+
+import ql
+import codeql_ql.ast.internal.Module
+
+File imports(File file) {
+  exists(Import imp |
+    imp.getLocation().getFile() = file and
+    result = imp.getResolvedModule().getFile()
+  )
+}
+
+/** Gets a non-abstract subclass of `ab` that is defined in a different file */
+Class concreteExternalSubclass(Class ab) {
+  ab.isAbstract() and
+  not result.isAbstract() and
+  result.getType().getASuperType*() = ab.getType() and
+  result.getLocation().getFile() != ab.getLocation().getFile()
+}
+
+/** Holds if there is a bidirectional import between the abstract class `ab` and its subclass `sub` */
+predicate bidirectionalImport(Class ab, Class sub) {
+  sub = concreteExternalSubclass(ab) and
+  sub.getLocation().getFile() = imports*(ab.getLocation().getFile())
+}
+
+predicate stats(Class ab, int imports, int nonImports) {
+  ab.isAbstract() and
+  imports =
+    strictcount(Class sub | sub = concreteExternalSubclass(ab) and bidirectionalImport(ab, sub)) and
+  nonImports =
+    strictcount(Class sub | sub = concreteExternalSubclass(ab) and not bidirectionalImport(ab, sub))
+}
+
+predicate alert(Class ab, string msg, Class sub, Class sub2) {
+  sub = concreteExternalSubclass(ab) and
+  sub2 = concreteExternalSubclass(ab) and
+  exists(int imports, int nonImports | stats(ab, imports, nonImports) |
+    (imports < 10 or nonImports < 10) and // if this is not the case, it's likely intended
+    (
+      // report whichever of imports or nonimports there are more of; both if equal
+      imports >= nonImports and
+      not bidirectionalImport(ab, sub) and
+      sub2 =
+        min(Class other |
+          other = concreteExternalSubclass(ab) and
+          bidirectionalImport(ab, other)
+        |
+          other order by other.getLocation().toString()
+        ) and
+      msg =
+        "This abstract class doesn't import its subclass $@ but imports " + imports +
+          " other subclasses, such as $@."
+      or
+      nonImports >= imports and
+      bidirectionalImport(ab, sub) and
+      sub2 =
+        min(Class other |
+          other = concreteExternalSubclass(ab) and
+          not bidirectionalImport(ab, other)
+        |
+          other order by other.getLocation().toString()
+        ) and
+      msg =
+        "This abstract class imports its subclass $@ but doesn't import " + nonImports +
+          " other subclasses, such as $@."
+    )
+  )
+}
+
+from Class ab, string msg, Class sub, Class sub2
+where alert(ab, msg, sub, sub2)
+select ab, msg, sub, sub.getName(), sub2, sub2.getName()