Merge pull request github#11770 from atorralba/atorralba/ql/omittable-exists

QL: Add OmittableExists query

Author: atorralba

ql/ql/src/codeql_ql/style/ConjunctionParent.qll

@@ -0,0 +1,46 @@
+import ql
+
+signature predicate checkAstNodeSig(AstNode p);
+
+module ConjunctionParent<checkAstNodeSig/1 checkAstNode> {
+  /**
+   * Gets the top-most parent of `p` that is not a disjunction.
+   */
+  AstNode getConjunctionParent(AstNode p) {
+    result =
+      min(int level, AstNode parent |
+        parent = getConjunctionParentRec(p) and level = level(parent)
+      |
+        parent order by level
+      )
+  }
+
+  /**
+   * Gets a (transitive) parent of `p`, where the parent is not a negative edge, and `checkAstNode(p)` holds.
+   */
+  private AstNode getConjunctionParentRec(AstNode p) {
+    checkAstNode(p) and
+    result = p
+    or
+    result = getConjunctionParentRec(p).getParent() and
+    not result instanceof Disjunction and
+    not result instanceof IfFormula and
+    not result instanceof Implication and
+    not result instanceof Negation and
+    not result instanceof Predicate and
+    not result instanceof ExprAggregate and
+    not result instanceof FullAggregate and
+    not result instanceof Forex and
+    not result instanceof Forall
+  }
+
+  /**
+   * Gets which level in the AST `p` is at.
+   * E.g. the top-level is 0, the next level is 1, etc.
+   */
+  private int level(AstNode p) {
+    p instanceof TopLevel and result = 0
+    or
+    result = level(p.getParent()) + 1
+  }
+}

ql/ql/src/queries/style/OmittableExists.ql

@@ -0,0 +1,39 @@
+/**
+ * @name Omittable 'exists' variable
+ * @description Writing 'exists(x | pred(x))' is bad practice and can be omitted.
+ * @kind problem
+ * @problem.severity warning
+ * @precision high
+ * @id ql/omittable-exists
+ * @tags maintainability
+ */
+
+import ql
+import codeql_ql.style.ConjunctionParent
+
+/**
+ * Holds if `existsArgument` is the declaration of a variable in an `exists` formula,
+ * and `use` is both its only use and an argument of a predicate that doesn't restrict
+ * the corresponding parameter type.
+ */
+predicate omittableExists(VarDecl existsArgument, VarAccess use) {
+  existsArgument = any(Exists e).getAnArgument() and
+  use = unique( | | existsArgument.getAnAccess()) and
+  exists(Call c, int argPos, Type paramType |
+    c.getArgument(argPos) = use and paramType = c.getTarget().getParameterType(argPos)
+  |
+    existsArgument.getType() = paramType.getASuperType*() and
+    not paramType instanceof DatabaseType
+  )
+}
+
+/** Holds if `p` is an exists variable (either declaration or use) that can be omitted. */
+predicate omittableExistsNode(AstNode p) { omittableExists(p, _) or omittableExists(_, p) }
+
+from VarDecl existsArgument, VarAccess use
+where
+  omittableExists(existsArgument, use) and
+  ConjunctionParent<omittableExistsNode/1>::getConjunctionParent(existsArgument) =
+    ConjunctionParent<omittableExistsNode/1>::getConjunctionParent(use)
+select existsArgument, "This exists variable can be omitted by using a don't-care expression $@.",
+  use, "in this argument"

ql/ql/src/queries/style/RedundantAssignment.ql

@@ -9,6 +9,8 @@
  */
 
 import ql
+import codeql_ql.style.ConjunctionParent
+import codeql.GlobalValueNumbering as GVN
 
 /**
  * A variable that is set equal to (assigned) a value one or more times.
@@ -40,8 +42,6 @@ class AssignedVariable extends VarDecl {
   }
 }
 
-import codeql.GlobalValueNumbering as GVN
-
 /**
  * Holds if `assigned1` and `assigned2` assigns the same value to `var`.
  * The assignments may be on different branches of a disjunction.
@@ -58,47 +58,14 @@ predicate candidateRedundantAssignment(AssignedVariable var, Expr assigned1, Exp
   assigned1 != assigned2
 }
 
-/**
- * Gets a (transitive) parent of `p`, where the parent is not a disjunction, and `p` is a candidate assignment from `candidateRedundantAssignment`.
- */
-AstNode getConjunctionParentRec(AstNode p) {
-  candidateRedundantAssignment(_, p, _) and
-  result = p
-  or
-  result = getConjunctionParentRec(p).getParent() and
-  not result instanceof Disjunction and
-  not result instanceof IfFormula and
-  not result instanceof Implication and
-  not result instanceof Negation and
-  not result instanceof Predicate
-}
-
-/**
- * Gets which level in the AST `p` is at.
- * E.g. the top-level is 0, the next level is 1, etc.
- */
-int level(AstNode p) {
-  p instanceof TopLevel and result = 0
-  or
-  result = level(p.getParent()) + 1
-}
-
-/**
- * Gets the top-most parent of `p` that is not a disjunction.
- */
-AstNode getConjunctionParent(AstNode p) {
-  result =
-    min(int level, AstNode parent |
-      parent = getConjunctionParentRec(p) and level = level(parent)
-    |
-      parent order by level
-    )
-}
+/** Holds if `p` is a candidate node for redundant assignment. */
+predicate candidateNode(AstNode p) { candidateRedundantAssignment(_, p, _) }
 
 from AssignedVariable var, Expr assigned1, Expr assigned2
 where
   candidateRedundantAssignment(var, assigned1, assigned2) and
-  getConjunctionParent(assigned1) = getConjunctionParent(assigned2) and
+  ConjunctionParent<candidateNode/1>::getConjunctionParent(assigned1) =
+    ConjunctionParent<candidateNode/1>::getConjunctionParent(assigned2) and
   // de-duplcation:
   (
     assigned1.getLocation().getStartLine() < assigned2.getLocation().getStartLine()

ql/ql/test/queries/style/OmittableExists/OmittableExists.expected

@@ -0,0 +1 @@
+| Test.qll:20:10:20:14 | i | This exists variable can be omitted by using a don't-care expression $@. | Test.qll:20:29:20:29 | i | in this argument |

ql/ql/test/queries/style/OmittableExists/OmittableExists.qlref

@@ -0,0 +1 @@
+queries/style/OmittableExists.ql

ql/ql/test/queries/style/OmittableExists/Test.qll

@@ -0,0 +1,39 @@
+predicate aPredicate(int i) { none() }
+
+predicate anotherPredicate(int i) { none() }
+
+predicate yetAnotherPredicate(int i, int y) { none() }
+
+predicate dbTypePredicate(@location l) { none() }
+
+string predicateWithResult(int i) { none() }
+
+class SmallInt extends int {
+  SmallInt() { this = [0 .. 10] }
+}
+
+class Location extends @location {
+  string toString() { result = "" }
+}
+
+predicate test() {
+  exists(int i | aPredicate(i)) // BAD
+  or
+  exists(int i | aPredicate(i) or anotherPredicate(i)) // BAD [NOT DETECTED]
+  or
+  exists(int i | aPredicate(i) and i.abs() = 0) // GOOD
+  or
+  exists(int i | aPredicate(i) and exists(int i2 | i = i2)) // GOOD
+  or
+  exists(int i | count(predicateWithResult(i)) = 0) // GOOD
+  or
+  exists(int i | count(int y | yetAnotherPredicate(i, y)) > 0) // GOOD
+  or
+  exists(int i | forex(int y | yetAnotherPredicate(i, y))) // GOOD
+  or
+  exists(int i | forall(int y | yetAnotherPredicate(i, y))) // GOOD
+  or
+  exists(SmallInt i | aPredicate(i)) // GOOD
+  or
+  exists(Location l | dbTypePredicate(l)) // GOOD
+}