diff --git a/javascript/ql/src/Statements/UselessConditional.ql b/javascript/ql/src/Statements/UselessConditional.ql index 83a1c9a3b5b6..94e44c556429 100644 --- a/javascript/ql/src/Statements/UselessConditional.ql +++ b/javascript/ql/src/Statements/UselessConditional.ql @@ -120,9 +120,11 @@ predicate whitelist(Expr e) { */ predicate isConditional(ASTNode cond, Expr e) { e = cond.(IfStmt).getCondition() or + e = cond.(LoopStmt).getTest() or e = cond.(ConditionalExpr).getCondition() or - e = cond.(LogAndExpr).getLeftOperand() or - e = cond.(LogOrExpr).getLeftOperand() + e = cond.(LogicalBinaryExpr).getLeftOperand() or + // Include `z` in `if (x && z)`. + isConditional(_, cond) and e = cond.(Expr).getUnderlyingValue().(LogicalBinaryExpr).getRightOperand() } from ASTNode cond, DataFlow::AnalyzedNode op, boolean cv, ASTNode sel, string msg @@ -132,7 +134,7 @@ where isConditional(cond, op.asExpr()) and // if `cond` is of the form ` && `, // we suggest replacing it with `, ` - if cond instanceof LogAndExpr and cv = true and not op.asExpr().isPure() then + if cond.(LogAndExpr).getLeftOperand() = op.asExpr() and cv = true and not op.asExpr().isPure() then (sel = cond and msg = "This logical 'and' expression can be replaced with a comma expression.") // otherwise we just report that `op` always evaluates to `cv` diff --git a/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.expected b/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.expected index ae363adfdeab..0790b9006b7c 100644 --- a/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.expected +++ b/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.expected @@ -16,6 +16,11 @@ | UselessConditional.js:76:13:76:13 | x | This use of variable 'x' always evaluates to true. | | UselessConditional.js:82:13:82:13 | x | This use of variable 'x' always evaluates to true. | | UselessConditional.js:89:10:89:16 | x, true | This expression always evaluates to true. | +| UselessConditional.js:94:16:94:16 | x | This use of variable 'x' always evaluates to false. | +| UselessConditional.js:100:13:100:24 | true && true | This expression always evaluates to true. | +| 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. | | 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. | diff --git a/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.js b/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.js index 179014e5c770..6b5c6b63c9a3 100644 --- a/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.js +++ b/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.js @@ -89,4 +89,19 @@ async function awaitFlow(){ if ((x, true)); }); +(function (x, y) { + if (!x) { + while (x) { // NOT OK + f(); + } + while (true) { // OK + break; + } + if (true && true) {} // NOT OK + if (y && x) {} // NOT OK + if (y && (x)) {} // NOT OK + do { } while (x); // NOT OK + } +}); + // semmle-extractor-options: --experimental