Merge pull request #87 from github/mathiasvp/superfluous-exists

New query: Unnecessary 'exists'

ql/src/codeql/GlobalValueNumbering.qll

@@ -0,0 +1,244 @@
+private import ql
+private import codeql_ql.ast.internal.Predicate
+private import codeql_ql.ast.internal.Type
+private import codeql_ql.ast.internal.Builtins
+
+private newtype TValueNumber =
+  TVariableValueNumber(VarDecl var) { variableAccessValueNumber(_, var) } or
+  TFieldValueNumber(VarDecl var) { fieldAccessValueNumber(_, var) } or
+  TThisValueNumber(Predicate pred) { thisAccessValueNumber(_, pred) } or
+  TPredicateValueNumber(PredicateOrBuiltin pred, ValueNumberArgumentList args) {
+    predicateCallValueNumber(_, pred, args)
+  } or
+  TClassPredicateValueNumber(PredicateOrBuiltin pred, ValueNumber base, ValueNumberArgumentList args) {
+    classPredicateCallValueNumber(_, pred, base, args)
+  } or
+  TLiteralValueNumber(string value, Type t) { literalValueNumber(_, value, t) } or
+  TBinaryOpValueNumber(FunctionSymbol symbol, ValueNumber leftOperand, ValueNumber rightOperand) {
+    binaryOperandValueNumber(_, symbol, leftOperand, rightOperand)
+  } or
+  TUnaryOpValueNumber(FunctionSymbol symbol, ValueNumber operand) {
+    unaryOperandValueNumber(_, symbol, operand)
+  } or
+  TInlineCastValueNumber(ValueNumber operand, Type t) { inlineCastValueNumber(_, operand, t) } or
+  TDontCareValueNumber() or
+  TRangeValueNumber(ValueNumber lower, ValueNumber high) { rangeValueNumber(_, lower, high) } or
+  TSetValueNumber(ValueNumberElementList elements) { setValueNumber(_, elements) } or
+  TUniqueValueNumber(Expr e) { uniqueValueNumber(e) }
+
+private newtype ValueNumberArgumentList =
+  MkArgsNil() or
+  MkArgsCons(ValueNumber head, ValueNumberArgumentList tail) {
+    argumentValueNumbers(_, _, head, tail)
+  }
+
+private newtype ValueNumberElementList =
+  MkElementsNil() or
+  MkElementsCons(ValueNumber head, ValueNumberElementList tail) {
+    setValueNumbers(_, _, head, tail)
+  }
+
+private ValueNumberArgumentList argumentValueNumbers(Call call, int start) {
+  start = call.getNumberOfArguments() and
+  result = MkArgsNil()
+  or
+  exists(ValueNumber head, ValueNumberArgumentList tail |
+    argumentValueNumbers(call, start, head, tail) and
+    result = MkArgsCons(head, tail)
+  )
+}
+
+private predicate argumentValueNumbers(
+  Call call, int start, ValueNumber head, ValueNumberArgumentList tail
+) {
+  head = valueNumber(call.getArgument(start)) and
+  tail = argumentValueNumbers(call, start + 1)
+}
+
+private ValueNumberElementList setValueNumbers(Set set, int start) {
+  start = set.getNumberOfElements() and
+  result = MkElementsNil()
+  or
+  exists(ValueNumber head, ValueNumberElementList tail |
+    setValueNumbers(set, start, head, tail) and
+    result = MkElementsCons(head, tail)
+  )
+}
+
+private predicate setValueNumbers(Set set, int start, ValueNumber head, ValueNumberElementList tail) {
+  head = valueNumber(set.getElement(start)) and
+  tail = setValueNumbers(set, start + 1)
+}
+
+/**
+ * A value number. A value number represents a collection of expressions that compute to the same value
+ * at runtime.
+ */
+class ValueNumber extends TValueNumber {
+  string toString() { result = "GVN" }
+
+  /** Gets an expression that has this value number. */
+  final Expr getAnExpr() { this = valueNumber(result) }
+}
+
+private predicate uniqueValueNumber(Expr e) { not numberable(e) }
+
+private predicate numberable(Expr e) {
+  e instanceof VarAccess or
+  e instanceof FieldAccess or
+  e instanceof ThisAccess or
+  e instanceof Call or
+  e instanceof Literal or
+  e instanceof BinOpExpr or
+  e instanceof UnaryExpr or
+  e instanceof InlineCast or
+  e instanceof ExprAnnotation or
+  e instanceof DontCare or
+  e instanceof Range or
+  e instanceof Set or
+  e instanceof AsExpr
+}
+
+private predicate variableAccessValueNumber(VarAccess access, VarDef var) {
+  access.getDeclaration() = var
+}
+
+private predicate fieldAccessValueNumber(FieldAccess access, VarDef var) {
+  access.getDeclaration() = var
+}
+
+private predicate thisAccessValueNumber(ThisAccess access, Predicate pred) {
+  access.getEnclosingPredicate() = pred
+}
+
+private predicate predicateCallValueNumber(
+  Call call, PredicateOrBuiltin pred, ValueNumberArgumentList args
+) {
+  call.getTarget() = pred and
+  not exists(call.(MemberCall).getBase()) and
+  args = argumentValueNumbers(call, 0)
+}
+
+private predicate classPredicateCallValueNumber(
+  MemberCall call, PredicateOrBuiltin pred, ValueNumber base, ValueNumberArgumentList args
+) {
+  call.getTarget() = pred and
+  valueNumber(call.getBase()) = base and
+  args = argumentValueNumbers(call, 0)
+}
+
+private predicate literalValueNumber(Literal lit, string value, Type t) {
+  lit.(String).getValue() = value and
+  t instanceof StringClass
+  or
+  lit.(Integer).getValue().toString() = value and
+  t instanceof IntClass
+  or
+  lit.(Float).getValue().toString() = value and
+  t instanceof FloatClass
+  or
+  lit.(Boolean).isFalse() and
+  value = "false" and
+  t instanceof BooleanClass
+  or
+  lit.(Boolean).isTrue() and
+  value = "true" and
+  t instanceof BooleanClass
+}
+
+private predicate binaryOperandValueNumber(
+  BinOpExpr e, FunctionSymbol symbol, ValueNumber leftOperand, ValueNumber rightOperand
+) {
+  e.getOperator() = symbol and
+  valueNumber(e.getLeftOperand()) = leftOperand and
+  valueNumber(e.getRightOperand()) = rightOperand
+}
+
+private predicate unaryOperandValueNumber(UnaryExpr e, FunctionSymbol symbol, ValueNumber operand) {
+  e.getOperator() = symbol and
+  valueNumber(e.getOperand()) = operand
+}
+
+private predicate inlineCastValueNumber(InlineCast cast, ValueNumber operand, Type t) {
+  valueNumber(cast.getBase()) = operand and
+  cast.getTypeExpr().getResolvedType() = t
+}
+
+private predicate rangeValueNumber(Range range, ValueNumber lower, ValueNumber high) {
+  valueNumber(range.getLowEndpoint()) = lower and
+  valueNumber(range.getHighEndpoint()) = high
+}
+
+private predicate setValueNumber(Set set, ValueNumberElementList elements) {
+  elements = setValueNumbers(set, 0)
+}
+
+private TValueNumber nonUniqueValueNumber(Expr e) {
+  exists(VarDecl var |
+    variableAccessValueNumber(e, var) and
+    result = TVariableValueNumber(var)
+  )
+  or
+  exists(VarDecl var |
+    fieldAccessValueNumber(e, var) and
+    result = TFieldValueNumber(var)
+  )
+  or
+  exists(Predicate pred |
+    thisAccessValueNumber(e, pred) and
+    result = TThisValueNumber(pred)
+  )
+  or
+  exists(PredicateOrBuiltin pred, ValueNumberArgumentList args |
+    predicateCallValueNumber(e, pred, args) and
+    result = TPredicateValueNumber(pred, args)
+  )
+  or
+  exists(PredicateOrBuiltin pred, ValueNumber base, ValueNumberArgumentList args |
+    classPredicateCallValueNumber(e, pred, base, args) and
+    result = TClassPredicateValueNumber(pred, base, args)
+  )
+  or
+  exists(string value, Type t |
+    literalValueNumber(e, value, t) and
+    result = TLiteralValueNumber(value, t)
+  )
+  or
+  exists(FunctionSymbol symbol, ValueNumber leftOperand, ValueNumber rightOperand |
+    binaryOperandValueNumber(e, symbol, leftOperand, rightOperand) and
+    result = TBinaryOpValueNumber(symbol, leftOperand, rightOperand)
+  )
+  or
+  exists(FunctionSymbol symbol, ValueNumber operand |
+    unaryOperandValueNumber(e, symbol, operand) and
+    result = TUnaryOpValueNumber(symbol, operand)
+  )
+  or
+  exists(ValueNumber operand, Type t |
+    inlineCastValueNumber(e, operand, t) and
+    result = TInlineCastValueNumber(operand, t)
+  )
+  or
+  result = valueNumber([e.(ExprAnnotation).getExpression(), e.(AsExpr).getInnerExpr()])
+  or
+  e instanceof DontCare and result = TDontCareValueNumber()
+  or
+  exists(ValueNumber lower, ValueNumber high |
+    rangeValueNumber(e, lower, high) and
+    result = TRangeValueNumber(lower, high)
+  )
+  or
+  exists(ValueNumberElementList elements |
+    setValueNumber(e, elements) and
+    result = TSetValueNumber(elements)
+  )
+}
+
+/** Gets the value number of an expression `e`. */
+cached
+TValueNumber valueNumber(Expr e) {
+  result = nonUniqueValueNumber(e)
+  or
+  uniqueValueNumber(e) and
+  result = TUniqueValueNumber(e)
+}

ql/src/codeql_ql/ast/Ast.qll

@@ -816,6 +816,9 @@ class Call extends TCall, Expr, Formula {
     none() // overriden in sublcasses.
   }
 
+  /** Gets an argument of this call, if any. */
+  final Expr getAnArgument() { result = getArgument(_) }
+
   PredicateOrBuiltin getTarget() { resolveCall(this, result) }
 
   override Type getType() { result = this.getTarget().getReturnType() }
@@ -1791,7 +1794,19 @@ class FunctionSymbol extends string {
 /**
  * A binary operation expression, such as `x + 3` or `y / 2`.
  */
-class BinOpExpr extends TBinOpExpr, Expr { }
+class BinOpExpr extends TBinOpExpr, Expr {
+  /** Gets the left operand of the binary expression. */
+  Expr getLeftOperand() { none() } // overriden in subclasses
+
+  /* Gets the right operand of the binary expression. */
+  Expr getRightOperand() { none() } // overriden in subclasses
+
+  /** Gets the operator of the binary expression. */
+  FunctionSymbol getOperator() { none() } // overriden in subclasses
+
+  /* Gets an operand of the binary expression. */
+  final Expr getAnOperand() { result = getLeftOperand() or result = getRightOperand() }
+}
 
 /**
  * An addition or subtraction expression.
@@ -1802,17 +1817,11 @@ class AddSubExpr extends TAddSubExpr, BinOpExpr {
 
   AddSubExpr() { this = TAddSubExpr(expr) and operator = expr.getChild().getValue() }
 
-  /** Gets the left operand of the binary expression. */
-  Expr getLeftOperand() { toGenerated(result) = expr.getLeft() }
+  override Expr getLeftOperand() { toGenerated(result) = expr.getLeft() }
 
-  /* Gets the right operand of the binary expression. */
-  Expr getRightOperand() { toGenerated(result) = expr.getRight() }
+  override Expr getRightOperand() { toGenerated(result) = expr.getRight() }
 
-  /* Gets an operand of the binary expression. */
-  Expr getAnOperand() { result = getLeftOperand() or result = getRightOperand() }
-
-  /** Gets the operator of the binary expression. */
-  FunctionSymbol getOperator() { result = operator }
+  override FunctionSymbol getOperator() { result = operator }
 
   override PrimitiveType getType() {
     // Both operands are the same type
@@ -1872,16 +1881,12 @@ class MulDivModExpr extends TMulDivModExpr, BinOpExpr {
   MulDivModExpr() { this = TMulDivModExpr(expr) and operator = expr.getChild().getValue() }
 
   /** Gets the left operand of the binary expression. */
-  Expr getLeftOperand() { toGenerated(result) = expr.getLeft() }
+  override Expr getLeftOperand() { toGenerated(result) = expr.getLeft() }
 
   /** Gets the right operand of the binary expression. */
-  Expr getRightOperand() { toGenerated(result) = expr.getRight() }
+  override Expr getRightOperand() { toGenerated(result) = expr.getRight() }
 
-  /** Gets an operand of the binary expression. */
-  Expr getAnOperand() { result = getLeftOperand() or result = getRightOperand() }
-
-  /** Gets the operator of the binary expression. */
-  FunctionSymbol getOperator() { result = operator }
+  override FunctionSymbol getOperator() { result = operator }
 
   override PrimitiveType getType() {
     // Both operands are of the same type
@@ -1954,6 +1959,11 @@ class Range extends TRange, Expr {
    */
   Expr getHighEndpoint() { toGenerated(result) = range.getUpper() }
 
+  /**
+   * Gets the lower and upper bounds of the range.
+   */
+  Expr getAnEndpoint() { result = [getLowEndpoint(), getHighEndpoint()] }
+
   override PrimitiveType getType() { result.getName() = "int" }
 
   override string getAPrimaryQlClass() { result = "Range" }
@@ -1980,6 +1990,16 @@ class Set extends TSet, Expr {
    */
   Expr getElement(int i) { toGenerated(result) = set.getChild(i) }
 
+  /**
+   * Gets an element in this set literal expression, if any.
+   */
+  Expr getAnElement() { result = getElement(_) }
+
+  /**
+   * Gets the number of elements in this set literal expression.
+   */
+  int getNumberOfElements() { result = count(getAnElement()) }
+
   override Type getType() { result = this.getElement(0).getType() }
 
   override string getAPrimaryQlClass() { result = "Set" }

ql/src/codeql_ql/ast/internal/Builtins.qll

@@ -65,3 +65,18 @@ string getArgType(string args, int i) { result = args.splitAt(",", i).trim() }
 class StringClass extends PrimitiveType {
   StringClass() { this.getName() = "string" }
 }
+
+/** The primitive 'int' class. */
+class IntClass extends PrimitiveType {
+  IntClass() { this.getName() = "int" }
+}
+
+/** The primitive 'float' class. */
+class FloatClass extends PrimitiveType {
+  FloatClass() { this.getName() = "float" }
+}
+
+/** The primitive 'boolean' class. */
+class BooleanClass extends PrimitiveType {
+  BooleanClass() { this.getName() = "boolean" }
+}

ql/src/queries/style/SuperfluousExists.ql

@@ -0,0 +1,99 @@
+/**
+ * @name Superfluous 'exists' conjunct.
+ * @description Writing 'exists(x)' when the existence of X is implied by another conjunct is bad practice.
+ * @kind problem
+ * @problem.severity warning
+ * @precision high
+ * @id ql/superfluous-exists
+ * @tags maintainability
+ */
+
+import ql
+import codeql.GlobalValueNumbering
+
+/**
+ * Gets an operand of this conjunction (we need the restriction
+ * to `Conjunction` to get the correct transitive closure).
+ */
+Formula getAConjOperand(Conjunction conj) { result = conj.getAnOperand() }
+
+/** A conjunction that is not a operand of another conjunction. */
+class TopLevelConjunction extends Conjunction {
+  TopLevelConjunction() { not this = getAConjOperand(_) }
+
+  /** Gets a formula within this conjunction that is not itself a conjunction. */
+  Formula getAnAtom() {
+    not result instanceof Conjunction and
+    result = getAConjOperand*(this)
+  }
+}
+
+/**
+ * Holds if the existence of `e` implies the existence of `vn`. For instance, the existence of
+ * `1 + x` implies the existence of a value number `vn` such that `vn.getAnExpr() = x`.
+ */
+predicate exprImpliesExists(ValueNumber vn, Expr e) {
+  vn.getAnExpr() = e
+  or
+  exprImpliesExists(vn, e.(BinOpExpr).getAnOperand())
+  or
+  exprImpliesExists(vn, e.(InlineCast).getBase())
+  or
+  exprImpliesExists(vn, e.(PredicateCall).getAnArgument())
+  or
+  exprImpliesExists(vn, [e.(MemberCall).getAnArgument(), e.(MemberCall).getBase()])
+  or
+  exprImpliesExists(vn, e.(UnaryExpr).getOperand())
+  or
+  exprImpliesExists(vn, e.(ExprAnnotation).getExpression())
+  or
+  forex(Formula child | child = e.(Set).getAnElement() | exprImpliesExists(vn, child))
+  or
+  exprImpliesExists(vn, e.(AsExpr).getInnerExpr())
+  or
+  exprImpliesExists(vn, e.(Range).getAnEndpoint())
+  or
+  exists(ExprAggregate agg |
+    agg = e and
+    agg.getKind().matches(["strict%", "unique"]) and
+    exprImpliesExists(vn, agg.getExpr(0))
+  )
+}
+
+/**
+ * Holds if the satisfiability of `f` implies the existence of `vn`. For instance, if `x.foo()` is
+ * satisfied, the value number `vn` such that `vn.getAnExpr() = x` exists.
+ */
+predicate formulaImpliesExists(ValueNumber vn, Formula f) {
+  forex(Formula child | child = f.(Disjunction).getAnOperand() | formulaImpliesExists(vn, child))
+  or
+  formulaImpliesExists(vn, f.(Conjunction).getAnOperand())
+  or
+  exprImpliesExists(vn, f.(ComparisonFormula).getAnOperand())
+  or
+  exists(IfFormula ifFormula | ifFormula = f |
+    exprImpliesExists(vn, ifFormula.getCondition())
+    or
+    formulaImpliesExists(vn, ifFormula.getThenPart()) and
+    formulaImpliesExists(vn, ifFormula.getElsePart())
+  )
+  or
+  exprImpliesExists(vn, f.(InstanceOf).getExpr())
+  or
+  exprImpliesExists(vn, f.(PredicateCall).getAnArgument())
+  or
+  exprImpliesExists(vn, [f.(MemberCall).getAnArgument(), f.(MemberCall).getBase()])
+  or
+  exists(InFormula inFormula | inFormula = f |
+    exprImpliesExists(vn, [inFormula.getExpr(), inFormula.getRange()])
+  )
+}
+
+from TopLevelConjunction toplevel, Exists existsFormula, ValueNumber vn, Formula conjunct
+where
+  existsFormula = toplevel.getAnAtom() and
+  vn.getAnExpr() = existsFormula.getExpr() and
+  conjunct = toplevel.getAnAtom() and
+  formulaImpliesExists(vn, conjunct)
+select existsFormula, "This conjunct is superfluous as the existence is implied by $@.", conjunct,
+  "this conjunct"