Merge pull request #685 from asger-semmle/useless-conditional-as-value

JS: fix FPs in UselessConditional

Author: Max Schaefer

javascript/ql/src/Statements/UselessConditional.ql

@@ -109,15 +109,26 @@ predicate whitelist(Expr e) {
 
 /**
  * Holds if `e` is part of a conditional node `cond` that evaluates
- * `e` and checks its value for truthiness.
+ * `e` and checks its value for truthiness, and the return value of `e`
+ * is not used for anything other than this truthiness check.
  */
-predicate isConditional(ASTNode cond, Expr e) {
+predicate isExplicitConditional(ASTNode cond, Expr e) {
   e = cond.(IfStmt).getCondition() or
   e = cond.(LoopStmt).getTest() or
   e = cond.(ConditionalExpr).getCondition() or
-  e = cond.(LogicalBinaryExpr).getLeftOperand() or
-  // Include `z` in `if (x && z)`.
-  isConditional(_, cond) and e = cond.(Expr).getUnderlyingValue().(LogicalBinaryExpr).getRightOperand()
+  isExplicitConditional(_, cond) and e = cond.(Expr).getUnderlyingValue().(LogicalBinaryExpr).getAnOperand()
+}
+
+/**
+ * Holds if `e` is part of a conditional node `cond` that evaluates
+ * `e` and checks its value for truthiness.
+ *
+ * The return value of `e` may have other uses besides the truthiness check,
+ * but if the truthiness check always goes one way, it still indicates an error.
+ */
+predicate isConditional(ASTNode cond, Expr e) {
+  isExplicitConditional(cond, e) or
+  e = cond.(LogicalBinaryExpr).getLeftOperand()
 }
 
 from ASTNode cond, DataFlow::AnalyzedNode op, boolean cv, ASTNode sel, string msg

javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.expected

@@ -21,6 +21,7 @@
 | UselessConditional.js:101:18:101:18 | x | This use of variable 'x' always evaluates to false. |
 | UselessConditional.js:102:19:102:19 | x | This use of variable 'x' always evaluates to false. |
 | UselessConditional.js:103:23:103:23 | x | This use of variable 'x' always evaluates to false. |
+| UselessConditional.js:109:15:109:16 | {} | This expression always evaluates to true. |
 | UselessConditionalGood.js:58:12:58:13 | x2 | This use of variable 'x2' always evaluates to false. |
 | UselessConditionalGood.js:69:12:69:13 | xy | This use of variable 'xy' always evaluates to false. |
 | UselessConditionalGood.js:85:12:85:13 | xy | This use of variable 'xy' always evaluates to false. |

javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.js

@@ -104,4 +104,9 @@ async function awaitFlow(){
     }
 });
 
+(function(x,y) {
+    let obj = (x && {}) || y; // OK
+    if ((x && {}) || y) {} // NOT OK
+});
+
 // semmle-extractor-options: --experimental