Merge pull request #129 from github/erik-krogh/cartesian

various new improvements and queries

Author: erik-krogh

ql/src/codeql_ql/ast/Ast.qll

@@ -49,6 +49,7 @@ class AstNode extends TAstNode {
   /**
    * Gets the parent in the AST for this node.
    */
+  cached
   AstNode getParent() { result.getAChild(_) = this }
 
   /**
@@ -74,12 +75,14 @@ class AstNode extends TAstNode {
   predicate hasAnnotation(string name) { this.getAnAnnotation().getName() = name }
 
   /** Gets an annotation of this AST node. */
-  Annotation getAnAnnotation() { toQL(this).getParent() = toQL(result).getParent() }
+  Annotation getAnAnnotation() {
+    toQL(this).getParent() = pragma[only_bind_out](toQL(result)).getParent()
+  }
 
   /**
    * Gets the predicate that contains this AST node.
    */
-  pragma[noinline]
+  cached
   Predicate getEnclosingPredicate() { this = getANodeInPredicate(result) }
 }
 
@@ -563,8 +566,10 @@ class VarDecl extends TVarDecl, VarDef, Declaration {
     )
   }
 
-  /** If this is a field, returns the class type that declares it. */
-  ClassType getDeclaringType() { result.getDeclaration().getAField() = this }
+  /** If this is declared in a field, returns the class type that declares it. */
+  ClassType getDeclaringType() {
+    exists(FieldDecl f | f.getVarDecl() = this and result = f.getParent().(Class).getType())
+  }
 
   /**
    * Holds if this is a class field that overrides the field `other`.
@@ -580,6 +585,32 @@ class VarDecl extends TVarDecl, VarDef, Declaration {
   override string toString() { result = this.getName() }
 }
 
+/**
+ * A field declaration;
+ */
+class FieldDecl extends TFieldDecl, AstNode {
+  QL::Field f;
+
+  FieldDecl() { this = TFieldDecl(f) }
+
+  VarDecl getVarDecl() { toQL(result) = f.getChild() }
+
+  override AstNode getAChild(string pred) {
+    result = super.getAChild(pred)
+    or
+    pred = directMember("getVarDecl") and result = this.getVarDecl()
+  }
+
+  override string getAPrimaryQlClass() { result = "FieldDecl" }
+
+  /** Holds if this field is annotated as overriding another field. */
+  predicate isOverride() { this.hasAnnotation("override") }
+
+  string getName() { result = getVarDecl().getName() }
+
+  override QLDoc getQLDoc() { result = any(Class c).getQLDocFor(this) }
+}
+
 /**
  * A type reference, such as `DataFlow::Node`.
  */
@@ -716,10 +747,7 @@ class Class extends TClass, TypeDeclaration, ModuleDeclaration {
    */
   CharPred getCharPred() { toQL(result) = cls.getChild(_).(QL::ClassMember).getChild(_) }
 
-  AstNode getMember(int i) {
-    toQL(result) = cls.getChild(i).(QL::ClassMember).getChild(_) or
-    toQL(result) = cls.getChild(i).(QL::ClassMember).getChild(_).(QL::Field).getChild()
-  }
+  AstNode getMember(int i) { toQL(result) = cls.getChild(i).(QL::ClassMember).getChild(_) }
 
   QLDoc getQLDocFor(AstNode m) {
     exists(int i | result = this.getMember(i) and m = this.getMember(i + 1))
@@ -743,9 +771,7 @@ class Class extends TClass, TypeDeclaration, ModuleDeclaration {
   /**
    * Gets a field in this class.
    */
-  VarDecl getAField() {
-    toQL(result) = cls.getChild(_).(QL::ClassMember).getChild(_).(QL::Field).getChild()
-  }
+  FieldDecl getAField() { result = getMember(_) }
 
   /**
    * Gets a super-type referenced in the `extends` part of the class declaration.
@@ -1102,9 +1128,15 @@ class Conjunction extends TConjunction, AstNode, Formula {
 
   override string getAPrimaryQlClass() { result = "Conjunction" }
 
-  /** Gets an operand to this formula. */
+  /** Gets an operand to this conjunction. */
   Formula getAnOperand() { toQL(result) in [conj.getLeft(), conj.getRight()] }
 
+  /** Gets the left operand to this conjunction. */
+  Formula getLeft() { toQL(result) = conj.getLeft() }
+
+  /** Gets the right operand to this conjunction. */
+  Formula getRight() { toQL(result) = conj.getRight() }
+
   override AstNode getAChild(string pred) {
     result = super.getAChild(pred)
     or
@@ -1120,33 +1152,22 @@ class Disjunction extends TDisjunction, AstNode, Formula {
 
   override string getAPrimaryQlClass() { result = "Disjunction" }
 
-  /** Gets an operand to this formula. */
+  /** Gets an operand to this disjunction. */
   Formula getAnOperand() { toQL(result) in [disj.getLeft(), disj.getRight()] }
 
+  /** Gets the left operand to this disjunction */
+  Formula getLeft() { toQL(result) = disj.getLeft() }
+
+  /** Gets the right operand to this disjunction */
+  Formula getRight() { toQL(result) = disj.getRight() }
+
   override AstNode getAChild(string pred) {
     result = super.getAChild(pred)
     or
     pred = directMember("getAnOperand") and result = this.getAnOperand()
   }
 }
 
-/**
- * A comparison operator, such as `<` or `=`.
- */
-class ComparisonOp extends TComparisonOp, AstNode {
-  QL::Compop op;
-
-  ComparisonOp() { this = TComparisonOp(op) }
-
-  /**
-   * Gets a string representing the operator.
-   * E.g. "<" or "=".
-   */
-  ComparisonSymbol getSymbol() { result = op.getValue() }
-
-  override string getAPrimaryQlClass() { result = "ComparisonOp" }
-}
-
 /**
  * A literal expression, such as `6` or `true` or `"foo"`.
  */
@@ -1243,10 +1264,7 @@ class ComparisonFormula extends TComparisonFormula, Formula {
   Expr getAnOperand() { result in [this.getLeftOperand(), this.getRightOperand()] }
 
   /** Gets the operator of this comparison. */
-  ComparisonOp getOperator() { toQL(result) = comp.getChild() }
-
-  /** Gets the symbol of this comparison (as a string). */
-  ComparisonSymbol getSymbol() { result = this.getOperator().getSymbol() }
+  ComparisonSymbol getOperator() { result = comp.getChild().getValue() }
 
   override string getAPrimaryQlClass() { result = "ComparisonFormula" }
 
@@ -1256,8 +1274,6 @@ class ComparisonFormula extends TComparisonFormula, Formula {
     pred = directMember("getLeftOperand") and result = this.getLeftOperand()
     or
     pred = directMember("getRightOperand") and result = this.getRightOperand()
-    or
-    pred = directMember("getOperator") and result = this.getOperator()
   }
 }
 
@@ -2218,6 +2234,8 @@ class Annotation extends TAnnotation, AstNode {
   /** Gets the node corresponding to the field `name`. */
   string getName() { result = annot.getName().getValue() }
 
+  override AstNode getParent() { result = AstNode.super.getParent() }
+
   override AstNode getAChild(string pred) {
     result = super.getAChild(pred)
     or

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

@@ -8,6 +8,7 @@ newtype TAstNode =
   TQLDoc(QL::Qldoc qldoc) or
   TClasslessPredicate(QL::ClasslessPredicate pred) or
   TVarDecl(QL::VarDecl decl) or
+  TFieldDecl(QL::Field field) or
   TClass(QL::Dataclass dc) or
   TCharPred(QL::Charpred pred) or
   TClassPredicate(QL::MemberPredicate pred) or
@@ -21,7 +22,6 @@ newtype TAstNode =
   TDisjunction(QL::Disjunction disj) or
   TConjunction(QL::Conjunction conj) or
   TComparisonFormula(QL::CompTerm comp) or
-  TComparisonOp(QL::Compop op) or
   TQuantifier(QL::Quantified quant) or
   TFullAggregate(QL::Aggregate agg) { agg.getChild(_) instanceof QL::FullAggregateBody } or
   TExprAggregate(QL::Aggregate agg) { agg.getChild(_) instanceof QL::ExprAggregateBody } or
@@ -90,7 +90,6 @@ private QL::AstNode toQLFormula(AST::AstNode n) {
   n = TConjunction(result) or
   n = TDisjunction(result) or
   n = TComparisonFormula(result) or
-  n = TComparisonOp(result) or
   n = TQuantifier(result) or
   n = TFullAggregate(result) or
   n = TIdentifier(result) or
@@ -127,6 +126,7 @@ private QL::AstNode toGenerateYAML(AST::AstNode n) {
 /**
  * Gets the underlying TreeSitter entity for a given AST node.
  */
+cached
 QL::AstNode toQL(AST::AstNode n) {
   result = toQLExpr(n)
   or
@@ -150,6 +150,8 @@ QL::AstNode toQL(AST::AstNode n) {
   or
   n = TVarDecl(result)
   or
+  n = TFieldDecl(result)
+  or
   n = TClass(result)
   or
   n = TCharPred(result)

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

@@ -134,7 +134,7 @@ predicate predOverrides(ClassPredicate sub, ClassPredicate sup) {
 }
 
 private VarDecl declaredField(ClassType ty, string name) {
-  result = ty.getDeclaration().getAField() and
+  result = ty.getDeclaration().getAField().getVarDecl() and
   result.getName() = name
 }
 

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

@@ -37,7 +37,7 @@ class VariableScope extends TScope, AstNode {
   predicate containsField(VarDef decl, string name) {
     name = decl.getName() and
     (
-      decl = this.(Class).getAField()
+      decl = this.(Class).getAField().getVarDecl()
       or
       this.getOuterScope().containsField(decl, name) and
       not exists(this.getADefinition(name))

…ries/performance/PrefixSuffixEquality.ql …nce/InefficientStringComparisonQuery.qllql/src/queries/performance/PrefixSuffixEquality.ql renamed to ql/src/codeql_ql/performance/InefficientStringComparisonQuery.qll ql/src/queries/performance/PrefixSuffixEquality.ql renamed to ql/src/codeql_ql/performance/InefficientStringComparisonQuery.qll

@@ -1,13 +1,3 @@
-/**
- * @name Prefix or suffix predicate calls when comparing with literal
- * @description Using 'myString.prefix(n) = "..."' instead of 'myString.matches("...%")'
- * @kind problem
- * @problem.severity error
- * @id ql/prefix-or-suffix-equality-check
- * @tags performance
- * @precision high
- */
-
 import ql
 import codeql_ql.ast.internal.Predicate
 import codeql_ql.ast.internal.Builtins
@@ -29,7 +19,7 @@ class SuffixPredicateCall extends Call {
 }
 
 class EqFormula extends ComparisonFormula {
-  EqFormula() { this.getSymbol() = "=" }
+  EqFormula() { this.getOperator() = "=" }
 }
 
 bindingset[s]
@@ -48,6 +38,17 @@ class FixPredicateCall extends Call {
   FixPredicateCall() { this instanceof PrefixPredicateCall or this instanceof SuffixPredicateCall }
 }
 
-from EqFormula eq, FixPredicateCall call, String literal
-where eq.getAnOperand() = call and eq.getAnOperand() = literal
-select eq, "Use " + getMessage(call, literal) + " instead."
+class RegexpMatchPredicate extends BuiltinPredicate {
+  RegexpMatchPredicate() { this = any(StringClass sc).getClassPredicate("regexpMatch", 1) }
+}
+
+predicate canUseMatchInsteadOfRegexpMatch(Call c, string matchesStr) {
+  c.getTarget() instanceof RegexpMatchPredicate and
+  exists(string raw | raw = c.getArgument(0).(String).getValue() |
+    matchesStr = "%" + raw.regexpCapture("^\\.\\*(\\w+)$", _)
+    or
+    matchesStr = raw.regexpCapture("^(\\w+)\\.\\*$", _) + "%"
+    or
+    matchesStr = "%" + raw.regexpCapture("^\\.\\*(\\w+)\\.\\*$", _) + "%"
+  )
+}

ql/src/codeql_ql/style/ImplicitThisQuery.qll

@@ -0,0 +1,20 @@
+import ql
+
+MemberCall explicitThisCallInFile(File f) {
+  result.getLocation().getFile() = f and
+  result.getBase() instanceof ThisAccess and
+  // Exclude `this.(Type).whatever(...)`, as some files have that as their only instance of `this`.
+  not result = any(InlineCast c).getBase()
+}
+
+PredicateCall implicitThisCallInFile(File f) {
+  result.getLocation().getFile() = f and
+  exists(result.getTarget().getDeclaringType().getASuperType()) and
+  // Exclude `SomeModule::whatever(...)`
+  not exists(result.getQualifier())
+}
+
+PredicateCall confusingImplicitThisCall(File f) {
+  result = implicitThisCallInFile(f) and
+  exists(explicitThisCallInFile(f))
+}

ql/src/codeql_ql/style/RedundantInlineCastQuery.qll

@@ -0,0 +1,35 @@
+import ql
+
+class RedundantInlineCast extends AstNode instanceof InlineCast {
+  Type t;
+
+  RedundantInlineCast() {
+    t = unique( | | super.getType()) and
+    (
+      // The cast is to the type the base expression already has
+      t = unique( | | super.getBase().getType())
+      or
+      // The cast is to the same type as the other expression in an equality comparison
+      exists(ComparisonFormula comp, Expr other | comp.getOperator() = "=" |
+        this = comp.getAnOperand() and
+        other = comp.getAnOperand() and
+        this != other and
+        t = unique( | | other.getType()) and
+        not other instanceof InlineCast // we don't want to risk both sides being "redundant"
+      )
+      or
+      exists(Call call, int i, Predicate target |
+        this = call.getArgument(i) and
+        target = unique( | | call.getTarget()) and
+        t = unique( | | target.getParameterType(i))
+      )
+    ) and
+    // noopt can require explicit casts
+    not exists(Annotation annon | annon = this.getEnclosingPredicate().getAnAnnotation() |
+      annon.getName() = "pragma" and
+      annon.getArgs(0).getValue() = "noopt"
+    )
+  }
+
+  TypeExpr getTypeExpr() { result = super.getTypeExpr() }
+}