Merge pull request github#11630 from erik-krogh/useInstanceOf

QL4QL: enable medium precision queries, and make the "suggest instanceof" query louder

Author: erik-krogh

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

@@ -7,6 +7,7 @@
     - alert
     - path-alert
     precision:
+    - medium
     - high
     - very-high
     problem.severity:

ql/ql/src/codeql_ql/style/UseInstanceofExtensionQuery.qll

@@ -14,20 +14,9 @@ predicate instanceofThisInCharPred(Class c, Type type) {
   |
     instanceOf.getExpr() instanceof ThisAccess and
     type = instanceOf.getType().getResolvedType()
-  )
-}
-
-/**
- * Holds if `c` uses the casting based range pattern, which could be replaced with `instanceof type`.
- */
-predicate usesCastingBasedInstanceof(Class c, Type type) {
-  instanceofThisInCharPred(c, type) and
-  // require that there is a call to the range class that matches the name of the enclosing predicate
-  exists(InlineCast cast, MemberCall call |
-    cast = getAThisCast(c, type) and
-    call.getBase() = cast and
-    cast.getEnclosingPredicate().getName() = call.getMemberName()
-  )
+  ) and
+  // no existing super-type corresponds to the instanceof type, that is benign.
+  not c.getType().getASuperType+() = type
 }
 
 /** Gets an inline cast that cases `this` to `type` inside a class predicate for `c`. */
@@ -40,7 +29,7 @@ InlineCast getAThisCast(Class c, Type type) {
   )
 }
 
-predicate usesFieldBasedInstanceof(Class c, TypeExpr type, FieldDecl field, ComparisonFormula comp) {
+predicate usesFieldBasedInstanceof(Class c, Type type, FieldDecl field, ComparisonFormula comp) {
   exists(FieldAccess fieldAccess |
     c.getCharPred().getBody() = comp or
     c.getCharPred().getBody().(Conjunction).getAnOperand() = comp
@@ -50,14 +39,9 @@ predicate usesFieldBasedInstanceof(Class c, TypeExpr type, FieldDecl field, Comp
     comp.getAnOperand() instanceof ThisAccess and
     comp.getAnOperand() = fieldAccess and
     fieldAccess.getDeclaration() = field and
-    field.getVarDecl().getTypeExpr() = type
+    field.getVarDecl().getType() = type
   ) and
-  // require that there is a call to the range field that matches the name of the enclosing predicate
-  exists(FieldAccess access, MemberCall call |
-    access = getARangeFieldAccess(c, field, _) and
-    call.getBase() = access and
-    access.getEnclosingPredicate().getName() = call.getMemberName()
-  )
+  not c.getType().getASuperType+() = type
 }
 
 FieldAccess getARangeFieldAccess(Class c, FieldDecl field, string name) {

ql/ql/src/queries/bugs/PathProblemQuery.ql

@@ -14,12 +14,15 @@ import codeql_ql.bugs.PathProblemQueryQuery
 
 from Query query, string msg, AstNode pred
 where
-  query.isPathProblem() and
-  not query.hasEdgesRelation(_) and
-  pred = any(TopLevel top | top.getLocation().getFile() = query) and // <- dummy value
-  msg = "A path-problem query should have a edges relation."
-  or
-  query.isProblem() and
-  query.hasEdgesRelation(pred) and
-  msg = "A problem query should not have a $@."
+  (
+    query.isPathProblem() and
+    not query.hasEdgesRelation(_) and
+    pred = any(TopLevel top | top.getLocation().getFile() = query) and // <- dummy value
+    msg = "A path-problem query should have a edges relation."
+    or
+    query.isProblem() and
+    query.hasEdgesRelation(pred) and
+    msg = "A problem query should not have a $@."
+  ) and
+  not query.getAbsolutePath().matches("%/test/%")
 select query, msg, pred, "edges relation"

ql/ql/src/queries/performance/ClassPredicateDoesntMentionThis.ql

@@ -5,7 +5,7 @@
  * @problem.severity warning
  * @id ql/class-predicate-doesnt-use-this
  * @tags performance
- * @precision medium
+ * @precision low
  */
 
 import ql

ql/ql/src/queries/style/OverridingParameterName.ql

@@ -6,7 +6,7 @@
  * @id ql/override-parameter-name
  * @tags correctness
  *       maintainability
- * @precision medium
+ * @precision low
  */
 
 import ql
@@ -19,13 +19,18 @@ private predicate getAnOverridingParameter(
   parameter = pred.getParameter(index)
 }
 
+pragma[nomagic]
+string getParameterName(ClassPredicate pred, int index) {
+  pred.getParameter(index).getName() = result
+}
+
 from ClassPredicate pred, ClassPredicate sup, VarDecl parameter, int index
 where
   getAnOverridingParameter(pred, sup, parameter, index) and
-  sup.getParameter(index).getName() != pred.getParameter(index).getName() and
+  getParameterName(sup, index) != getParameterName(pred, index) and
   // avoid duplicated alerts with `ql/override-swapped-name`
   not exists(int other | other != index |
-    sup.getParameter(other).getName() = pred.getParameter(index).getName()
+    getParameterName(sup, other) = getParameterName(pred, index)
   )
 select parameter, pred.getParameter(index).getName() + " was $@ in the super class.",
   sup.getParameter(index), "named " + sup.getParameter(index).getName()

ql/ql/src/queries/style/ToStringInQueryLogic.ql

@@ -2,7 +2,7 @@
  * @name Using 'toString' in query logic
  * @description A query should not depend on the output of 'toString'.
  * @kind problem
- * @problem.severity error
+ * @problem.severity warning
  * @id ql/to-string-in-logic
  * @precision medium
  * @tags maintainability

ql/ql/src/queries/style/UseInstanceofExtension.ql

@@ -14,8 +14,8 @@ import codeql_ql.style.UseInstanceofExtensionQuery
 from Class c, Type type, string message
 where
   (
-    usesCastingBasedInstanceof(c, type) or
-    usesFieldBasedInstanceof(c, any(TypeExpr te | te.getResolvedType() = type), _, _)
+    instanceofThisInCharPred(c, type) or
+    usesFieldBasedInstanceof(c, type, _, _)
   ) and
   message = "Consider defining this class as non-extending subtype of $@."
 select c, message, type.getDeclaration(), type.getName()

ql/ql/test/queries/style/UseInstanceofExtension/Foo.qll

@@ -0,0 +1,29 @@
+class Range extends string {
+  Range() { this = "ql" }
+
+  string getAChild() { result = "test" }
+}
+
+class Inst extends string {
+  Range range;
+
+  Inst() { this = range }
+
+  string getAChild() { result = range.getAChild() }
+}
+
+class Inst2 extends string {
+  Inst2() { this instanceof Range }
+
+  string getAChild() { result = this.(Range).getAChild() }
+}
+
+class Inst3 extends string {
+  Range range;
+
+  Inst3() { this = range }
+}
+
+class Inst4 extends string {
+  Inst4() { this instanceof Range }
+}

ql/ql/test/queries/style/UseInstanceofExtension/UseInstanceofExtension.expected

@@ -0,0 +1,4 @@
+| Foo.qll:7:7:7:10 | Class Inst | Consider defining this class as non-extending subtype of $@. | Foo.qll:1:7:1:11 | Class Range | Range |
+| Foo.qll:15:7:15:11 | Class Inst2 | Consider defining this class as non-extending subtype of $@. | Foo.qll:1:7:1:11 | Class Range | Range |
+| Foo.qll:21:7:21:11 | Class Inst3 | Consider defining this class as non-extending subtype of $@. | Foo.qll:1:7:1:11 | Class Range | Range |
+| Foo.qll:27:7:27:11 | Class Inst4 | Consider defining this class as non-extending subtype of $@. | Foo.qll:1:7:1:11 | Class Range | Range |

ql/ql/test/queries/style/UseInstanceofExtension/UseInstanceofExtension.qlref

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