Merge pull request github#5604 from MathiasVP/fix-false-positive-in-assign-where-compare-meant

C++: Fix FP in cpp/assign-where-compare-meant

Author: jbj

cpp/change-notes/2021-04-06-assign-where-compare-meant.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* The 'Assignment where comparison was intended' (cpp/assign-where-compare-meant) query has been improved to flag fewer benign assignments in conditionals.

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,3 +19,7 @@
 | 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: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. |
+| test.cpp:171:7:171:12 | ... = ... | Use of '=' where '==' may have been intended. |

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

@@ -153,3 +153,21 @@ void f3(int x, int y) {
   if((x == 10) || ((z == z) && (x == 1)) && (y = 2)) { // BAD
   }
 }
+
+bool use(int);
+
+void f4(int x, bool b) {
+  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: 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.
+  if((use(x) && b) && (x = 10)) {} // BAD: Same reason as above
+  if(use(x) && (b && (x = 10))) {} // BAD: Same reason as above
+
+  if((x = 10) || use(x)) {} // BAD: This doesn't follow the usual style of writing an assignment in
+                            // a boolean check.
+}