C++: Cover more cases of returning *this

Author: jbj

cpp/ql/src/jsf/4.10 Classes/AV Rule 82.ql

@@ -25,7 +25,7 @@ predicate pointerThis(Expr e) {
   // `f(...)`
   // (includes `this = ...`, where `=` is overloaded so a `FunctionCall`)
   exists(FunctionCall fc | fc = e and callOnThis(fc) |
-    exists(fc.getTarget().getBlock()) implies returnsPointerThis(fc.getTarget())
+    returnsPointerThis(fc.getTarget())
   ) or
 
   // `this = ...` (where `=` is not overloaded, so an `AssignExpr`) 
@@ -38,22 +38,33 @@ predicate dereferenceThis(Expr e) {
   // `f(...)`
   // (includes `*this = ...`, where `=` is overloaded so a `FunctionCall`)
   exists(FunctionCall fc | fc = e and callOnThis(fc) |
-    exists(fc.getTarget().getBlock()) implies returnsDereferenceThis(fc.getTarget())
+    returnsDereferenceThis(fc.getTarget())
   ) or
 
   // `*this = ...` (where `=` is not overloaded, so an `AssignExpr`) 
   dereferenceThis(e.(AssignExpr).getLValue())
 }
 
+/**
+ * Holds if all `return` statements in `f` return `this`, possibly indirectly.
+ * This includes functions whose body is not in the database.
+ */
 predicate returnsPointerThis(Function f) {
-  forex(ReturnStmt s | s.getEnclosingFunction() = f |
+  f.getType().getUnspecifiedType() instanceof PointerType and
+  forall(ReturnStmt s | s.getEnclosingFunction() = f and reachable(s) |
     // `return this`
     pointerThis(s.getExpr())
   )
 }
 
+/**
+ * Holds if all `return` statements in `f` return a reference to `*this`,
+ * possibly indirectly. This includes functions whose body is not in the
+ * database.
+ */
 predicate returnsDereferenceThis(Function f) {
-  forex(ReturnStmt s | s.getEnclosingFunction() = f |
+  f.getType().getUnspecifiedType() instanceof ReferenceType and
+  forall(ReturnStmt s | s.getEnclosingFunction() = f and reachable(s) |
     // `return *this`
     dereferenceThis(s.getExpr())
   )
@@ -72,10 +83,6 @@ predicate assignOperatorWithWrongType(Operator op, string msg) {
 predicate assignOperatorWithWrongResult(Operator op, string msg) {
   op.hasName("operator=")
   and not returnsDereferenceThis(op)
-  // If a function does not have a reachable `ReturnStmt` then either its body
-  // was not in the snapshot or it was established by the extractor or the CFG
-  // pruning that the function never returns.
-  and exists(ReturnStmt ret | ret.getEnclosingFunction() = op and reachable(ret))
   and not op.getType() instanceof VoidType
   and not assignOperatorWithWrongType(op, _)
   and msg = "Assignment operator in class " + op.getDeclaringType().getName() + " does not return a reference to *this."

cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.cpp

@@ -134,7 +134,7 @@ class AlwaysThrows {
 };
 
 class Reachability {
-  Reachability &operator=(Reachability &that) { // GOOD [FALSE POSITIVE]
+  Reachability &operator=(Reachability &that) { // GOOD
     int one = 1;
     if (one)
       return *this;
@@ -160,11 +160,11 @@ class Reachability {
       return &staticInstance; // unreachable
   }
 
-  Reachability &operator=(int _val) { // GOOD [FALSE POSITIVE]
+  Reachability &operator=(int _val) { // GOOD
     return returnThisReference();
   }
 
-  Reachability &operator=(short _val) { // GOOD [FALSE POSITIVE]
+  Reachability &operator=(short _val) { // GOOD
     return *returnThisPointer();
   }
 

cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected

@@ -2,6 +2,3 @@
 | AV Rule 82.cpp:24:8:24:16 | operator= | Assignment operator in class Bad2 should have return type Bad2&. Otherwise a copy is created at each call. |
 | AV Rule 82.cpp:63:29:63:37 | operator= | Assignment operator in class TemplateReturnAssignment<T> does not return a reference to *this. |
 | AV Rule 82.cpp:63:29:63:37 | operator= | Assignment operator in class TemplateReturnAssignment<int> does not return a reference to *this. |
-| AV Rule 82.cpp:137:17:137:25 | operator= | Assignment operator in class Reachability does not return a reference to *this. |
-| AV Rule 82.cpp:163:17:163:25 | operator= | Assignment operator in class Reachability does not return a reference to *this. |
-| AV Rule 82.cpp:167:17:167:25 | operator= | Assignment operator in class Reachability does not return a reference to *this. |