C++: Fix false positive by adding another allow-list pattern in AssignWhereCompareMeant.

Author: MathiasVP

cpp/ql/src/Likely Bugs/Likely Typos/AssignWhereCompareMeant.ql

@@ -54,14 +54,29 @@ class BooleanControllingAssignmentInExpr extends BooleanControllingAssignment {
   override predicate isWhitelisted() {
     this.getConversion().(ParenthesisExpr).isParenthesised()
     or
-    // whitelist this assignment if all comparison operations in the expression that this
+    // Allow this assignment if all comparison operations in the expression that this
     // assignment is part of, are not parenthesized. In that case it seems like programmer
     // is fine with unparenthesized comparison operands to binary logical operators, and
     // the parenthesis around this assignment was used to call it out as an assignment.
     this.isParenthesised() and
     forex(ComparisonOperation op | op = getComparisonOperand*(this.getParent+()) |
       not op.isParenthesised()
     )
+    or
+    // Match a pattern like:
+    // ```
+    // if((a = b) && use_value(a)) { ... }
+    // ```
+    // where the assignment is meant to update the value of `a` before it's used in some other boolean
+    // subexpression that is guarenteed to be evaluate _after_ the assignment.
+    this.isParenthesised() and
+    exists(LogicalAndExpr parent, Variable var, VariableAccess access |
+      var = this.getLValue().(VariableAccess).getTarget() and
+      access = var.getAnAccess() and
+      not access.isUsedAsLValue() and
+      parent.getRightOperand() = access.getParent*() and
+      parent.getLeftOperand() = this.getParent*()
+    )
   }
 }
 

cpp/ql/test/query-tests/Likely Bugs/Likely Typos/AssignWhereCompareMeant/AssignWhereCompareMeant.expected

@@ -19,10 +19,6 @@
 | test.cpp:144:32:144:36 | ... = ... | Use of '=' where '==' may have been intended. |
 | test.cpp:150:32:150:36 | ... = ... | Use of '=' where '==' may have been intended. |
 | test.cpp:153:46:153:50 | ... = ... | Use of '=' where '==' may have been intended. |
-| test.cpp:160:7:160:12 | ... = ... | Use of '=' where '==' may have been intended. |
-| test.cpp:162:7:162:12 | ... = ... | Use of '=' where '==' may have been intended. |
-| test.cpp:163:7:163:12 | ... = ... | Use of '=' where '==' may have been intended. |
-| test.cpp:164:7:164:12 | ... = ... | Use of '=' where '==' may have been intended. |
 | test.cpp:166:22:166:27 | ... = ... | Use of '=' where '==' may have been intended. |
 | test.cpp:168:24:168:29 | ... = ... | Use of '=' where '==' may have been intended. |
 | test.cpp:169:23:169:28 | ... = ... | Use of '=' where '==' may have been intended. |

cpp/ql/test/query-tests/Likely Bugs/Likely Typos/AssignWhereCompareMeant/test.cpp

@@ -157,11 +157,11 @@ void f3(int x, int y) {
 bool use(int);
 
 void f4(int x, bool b) {
-  if((x = 10) && use(x)) {} // GOOD [FALSE POSITIVE]: This is likely just a short-hand way of writing an assignment
+  if((x = 10) && use(x)) {} // GOOD: This is likely just a short-hand way of writing an assignment
                             // followed by a boolean check.
-  if((x = 10) && b && use(x)) {} // GOOD [FALSE POSITIVE]: Same reason as above
-  if((x = 10) && use(x) && b) {} // GOOD [FALSE POSITIVE]: Same reason as above
-  if((x = 10) && (use(x) && b)) {} // GOOD [FALSE POSITIVE]: Same reason as above
+  if((x = 10) && b && use(x)) {} // GOOD: Same reason as above
+  if((x = 10) && use(x) && b) {} // GOOD: Same reason as above
+  if((x = 10) && (use(x) && b)) {} // GOOD: Same reason as above
 
   if(use(x) && b && (x = 10)) {} // BAD: The assignment is the last thing that happens in the comparison.
                                  // This doesn't match the usual pattern.