Merge pull request #7472 from erik-krogh/redundant-aggregate

QL-for-QL: Add a could-be-cast query

Author: MathiasVP

ql/ql/src/codeql-suites/ql-code-scanning.qls

@@ -12,6 +12,7 @@
     problem.severity:
     - error
     - warning
+    - recommendation
 - exclude:
     deprecated: //
 - exclude:

ql/ql/src/codeql_ql/ast/Ast.qll

@@ -63,6 +63,9 @@ class AstNode extends TAstNode {
     pred = directMember("getQLDoc") and result = this.getQLDoc()
   }
 
+  /** Gets any child of this node. */
+  AstNode getAChild() { result = this.getAChild(_) }
+
   /**
    * Gets the primary QL class for the ast node.
    */
@@ -1238,14 +1241,7 @@ class Boolean extends Literal {
 
 /** A comparison symbol, such as `"<"` or `"="`. */
 class ComparisonSymbol extends string {
-  ComparisonSymbol() {
-    this = "=" or
-    this = "!=" or
-    this = "<" or
-    this = ">" or
-    this = "<=" or
-    this = ">="
-  }
+  ComparisonSymbol() { this = ["=", "!=", "<", ">", "<=", ">="] }
 }
 
 /** A comparison formula, such as `x < 3` or `y = true`. */
@@ -1287,10 +1283,7 @@ class Quantifier extends TQuantifier, Formula {
   }
 
   /** Gets the ith variable declaration of this quantifier. */
-  VarDecl getArgument(int i) {
-    i >= 1 and
-    toQL(result) = quant.getChild(i - 1)
-  }
+  VarDecl getArgument(int i) { toQL(result) = quant.getChild(i + 1) }
 
   /** Gets an argument of this quantifier. */
   VarDecl getAnArgument() { result = this.getArgument(_) }
@@ -1661,6 +1654,15 @@ class FullAggregate extends TFullAggregate, Aggregate {
   }
 }
 
+/**
+ * A "any" expression, such as `any(int i | i > 0).toString()`.
+ */
+class Any extends FullAggregate {
+  Any() { this.getKind() = "any" }
+
+  override string getAPrimaryQlClass() { result = "Any" }
+}
+
 /**
  * A "rank" expression, such as `rank[4](int i | i = [5 .. 15] | i)`.
  */
@@ -1855,7 +1857,7 @@ class ExprAnnotation extends TExprAnnotation, Expr {
 
 /** A function symbol, such as `+` or `*`. */
 class FunctionSymbol extends string {
-  FunctionSymbol() { this = "+" or this = "-" or this = "*" or this = "/" or this = "%" }
+  FunctionSymbol() { this = ["+", "-", "*", "/", "%"] }
 }
 
 /**

ql/ql/src/codeql_ql/style/CouldBeCastQuery.qll

@@ -0,0 +1,53 @@
+import ql
+
+/**
+ * Holds if `aggr` is of one of the following forms:
+ * `exists(var | range)` or `any(var | range)` or `any(var | | range)` or `any(type x | .. | x)`
+ */
+private predicate castAggregate(AstNode aggr, Formula range, VarDecl var, string kind) {
+  kind = "exists" and
+  exists(Exists ex | aggr = ex |
+    ex.getRange() = range and
+    not exists(ex.getFormula()) and
+    count(ex.getArgument(_)) = 1 and
+    ex.getArgument(0) = var
+  )
+  or
+  kind = "any" and
+  exists(Any anyy | aggr = anyy |
+    not exists(anyy.getRange()) and
+    anyy.getExpr(0) = range and
+    count(anyy.getExpr(_)) = 1 and
+    count(anyy.getArgument(_)) = 1 and
+    anyy.getArgument(0) = var
+  )
+  or
+  kind = "any" and
+  exists(Any anyy | aggr = anyy |
+    range = anyy.getRange() and
+    count(anyy.getArgument(_)) = 1 and
+    anyy.getArgument(0) = var and
+    not exists(anyy.getExpr(0))
+  )
+  or
+  kind = "any" and
+  exists(Any anyy | aggr = anyy |
+    range = anyy.getRange() and
+    count(anyy.getExpr(_)) = 1 and
+    count(anyy.getArgument(_)) = 1 and
+    anyy.getExpr(_).(VarAccess).getDeclaration() = var and
+    anyy.getArgument(0) = var
+  )
+}
+
+/** Holds if `aggr` is an aggregate that could be replaced with an instanceof expression or an inline cast. */
+predicate aggregateCouldBeCast(
+  AstNode aggr, ComparisonFormula comp, string kind, VarDecl var, AstNode operand
+) {
+  castAggregate(aggr, comp, var, kind) and
+  comp.getOperator() = "=" and
+  count(VarAccess access | access.getDeclaration() = var and access.getParent() != aggr) = 1 and
+  comp.getAnOperand().(VarAccess).getDeclaration() = var and
+  operand = comp.getAnOperand() and
+  not operand.(VarAccess).getDeclaration() = var
+}

ql/ql/src/queries/style/CouldBeCast.ql

@@ -0,0 +1,27 @@
+/**
+ * @name Expression can be replaced with a cast
+ * @description An exists/any that is only used to cast a value to a type
+ *              can be replaced with a cast.
+ * @kind problem
+ * @problem.severity warning
+ * @id ql/could-be-cast
+ * @precision very-high
+ */
+
+import ql
+import codeql_ql.style.CouldBeCastQuery
+
+from AstNode aggr, VarDecl var, string msg, Expr operand
+where
+  exists(string kind | aggregateCouldBeCast(aggr, _, kind, var, operand) |
+    kind = "exists" and
+    if operand.getType().getASuperType*() = var.getType()
+    then msg = "The assignment in the exists(..) is redundant."
+    else msg = "The assignment to $@ in the exists(..) can replaced with an instanceof expression."
+    or
+    kind = "any" and
+    if operand.getType().getASuperType*() = var.getType()
+    then msg = "The assignment in the any(..) is redundant."
+    else msg = "The assignment to $@ in this any(..) can be replaced with an inline cast."
+  )
+select aggr, msg, var, var.getName()

ql/ql/src/queries/style/RedundantInlineCast.ql

@@ -2,7 +2,7 @@
  * @name Redundant inline cast
  * @description Redundant inline casts
  * @kind problem
- * @problem.severity error
+ * @problem.severity warning
  * @id ql/redundant-inline-cast
  * @tags maintainability
  * @precision high

ql/ql/src/queries/style/docs/MissingQLDoc.ql

@@ -121,8 +121,8 @@ nodes
 | Foo.qll:22:3:22:3 | f | semmle.order | 60 |
 | Foo.qll:22:3:22:16 | ComparisonFormula | semmle.label | [ComparisonFormula] ComparisonFormula |
 | Foo.qll:22:3:22:16 | ComparisonFormula | semmle.order | 60 |
-| Foo.qll:22:7:22:16 | FullAggregate[any] | semmle.label | [FullAggregate[any]] FullAggregate[any] |
-| Foo.qll:22:7:22:16 | FullAggregate[any] | semmle.order | 62 |
+| Foo.qll:22:7:22:16 | Any | semmle.label | [Any] Any |
+| Foo.qll:22:7:22:16 | Any | semmle.order | 62 |
 | Foo.qll:22:11:22:13 | TypeExpr | semmle.label | [TypeExpr] TypeExpr |
 | Foo.qll:22:11:22:13 | TypeExpr | semmle.order | 63 |
 | Foo.qll:22:11:22:15 | f | semmle.label | [VarDecl] f |
@@ -363,10 +363,10 @@ edges
 | Foo.qll:20:3:20:13 | ComparisonFormula | Foo.qll:20:13:20:13 | f | semmle.order | 59 |
 | Foo.qll:22:3:22:16 | ComparisonFormula | Foo.qll:22:3:22:3 | f | semmle.label | getLeftOperand() |
 | Foo.qll:22:3:22:16 | ComparisonFormula | Foo.qll:22:3:22:3 | f | semmle.order | 60 |
-| Foo.qll:22:3:22:16 | ComparisonFormula | Foo.qll:22:7:22:16 | FullAggregate[any] | semmle.label | getRightOperand() |
-| Foo.qll:22:3:22:16 | ComparisonFormula | Foo.qll:22:7:22:16 | FullAggregate[any] | semmle.order | 62 |
-| Foo.qll:22:7:22:16 | FullAggregate[any] | Foo.qll:22:11:22:15 | f | semmle.label | getArgument(_) |
-| Foo.qll:22:7:22:16 | FullAggregate[any] | Foo.qll:22:11:22:15 | f | semmle.order | 63 |
+| Foo.qll:22:3:22:16 | ComparisonFormula | Foo.qll:22:7:22:16 | Any | semmle.label | getRightOperand() |
+| Foo.qll:22:3:22:16 | ComparisonFormula | Foo.qll:22:7:22:16 | Any | semmle.order | 62 |
+| Foo.qll:22:7:22:16 | Any | Foo.qll:22:11:22:15 | f | semmle.label | getArgument(_) |
+| Foo.qll:22:7:22:16 | Any | Foo.qll:22:11:22:15 | f | semmle.order | 63 |
 | Foo.qll:22:11:22:15 | f | Foo.qll:22:11:22:13 | TypeExpr | semmle.label | getTypeExpr() |
 | Foo.qll:22:11:22:15 | f | Foo.qll:22:11:22:13 | TypeExpr | semmle.order | 63 |
 | Foo.qll:24:3:24:23 | ComparisonFormula | Foo.qll:24:3:24:3 | Integer | semmle.label | getLeftOperand() |

ql/ql/test/printAst/printAst.expected

@@ -0,0 +1,5 @@
+| Foo.qll:3:3:3:24 | Exists | The assignment to $@ in the exists(..) can replaced with an instanceof expression. | Foo.qll:3:10:3:15 | j | j |
+| Foo.qll:7:3:7:21 | Any | The assignment to $@ in this any(..) can be replaced with an inline cast. | Foo.qll:7:7:7:12 | j | j |
+| Foo.qll:9:3:9:25 | Any | The assignment to $@ in this any(..) can be replaced with an inline cast. | Foo.qll:9:7:9:12 | j | j |
+| Foo.qll:15:3:15:20 | Any | The assignment in the any(..) is redundant. | Foo.qll:15:7:15:11 | j | j |
+| Foo.qll:17:3:17:23 | Exists | The assignment in the exists(..) is redundant. | Foo.qll:17:10:17:14 | j | j |

ql/ql/test/queries/style/CouldBeCast/CouldBeCast.expected

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

ql/ql/test/queries/style/CouldBeCast/CouldBeCast.qlref

@@ -0,0 +1,23 @@
+bindingset[i]
+predicate foo(int i) {
+  exists(Even j | j = i) // NOT OK
+  or
+  exists(Even j | j = i | j % 4 = 0) // OK
+  or
+  any(Even j | j = i) = 2 // NOT OK
+  or
+  any(Even j | j = i | j) = 2 // NOT OK
+  or
+  any(Even j | j = i | j * 2) = 4 // OK
+  or
+  any(Even j | j = i and j % 4 = 0 | j) = 4 // OK
+  or
+  any(int j | j = i) = 2 // NOT OK
+  or
+  exists(int j | j = i) // NOT OK
+}
+
+class Even extends int {
+  bindingset[this]
+  Even() { this % 2 = 0 }
+}