diff --git a/change-notes/1.19/analysis-javascript.md b/change-notes/1.19/analysis-javascript.md index 635771eea8dd..d92f9c7db6f2 100644 --- a/change-notes/1.19/analysis-javascript.md +++ b/change-notes/1.19/analysis-javascript.md @@ -39,6 +39,7 @@ | User-controlled bypass of security check | Fewer results | This rule no longer flags conditions that guard early returns. The precision of this rule has been revised to "medium". Results are no longer shown on LGTM by default. | | Whitespace contradicts operator precedence | Fewer false-positive results | This rule no longer flags operators with asymmetric whitespace. | | Unused import | Fewer false-positive results | This rule no longer flags imports used by the `transform-react-jsx` Babel plugin. | +| Useless conditional | More true-positive results | This rule now flags useless conditions in more cases. | ## Changes to QL libraries diff --git a/javascript/ql/src/Statements/UselessConditional.ql b/javascript/ql/src/Statements/UselessConditional.ql index a740e8628d8b..3f6e55cb20b4 100644 --- a/javascript/ql/src/Statements/UselessConditional.ql +++ b/javascript/ql/src/Statements/UselessConditional.ql @@ -115,25 +115,21 @@ predicate whitelist(Expr e) { } /** - * Holds if `e` is part of a conditional node `cond` that evaluates - * `e` and checks its value for truthiness. + * Gets the `&&` expression that defines this guard node, if any. */ -predicate isConditional(ASTNode cond, Expr e) { - e = cond.(IfStmt).getCondition() or - e = cond.(ConditionalExpr).getCondition() or - e = cond.(LogAndExpr).getLeftOperand() or - e = cond.(LogOrExpr).getLeftOperand() +LogAndExpr getLogAndExpr(ConditionGuardNode guard) { + result.getLeftOperand().stripParens() = guard.getTest() } -from ASTNode cond, DataFlow::AnalyzedNode op, boolean cv, ASTNode sel, string msg -where isConditional(cond, op.asExpr()) and +from ConditionGuardNode guard, DataFlow::AnalyzedNode op, boolean cv, ASTNode sel, string msg +where guard.getTest() = op.asExpr() and cv = op.getTheBooleanValue()and not whitelist(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 - (sel = cond and msg = "This logical 'and' expression can be replaced with a comma expression.") + if exists(getLogAndExpr(guard)) and cv = true and not op.asExpr().isPure() then + (sel = getLogAndExpr(guard) and msg = "This logical 'and' expression can be replaced with a comma expression.") // otherwise we just report that `op` always evaluates to `cv` else ( diff --git a/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.expected b/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.expected index de351ec51b9d..5562d3a7c70b 100644 --- a/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.expected +++ b/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.expected @@ -1,4 +1,4 @@ -| UselessConditional.js:5:7:5:12 | !lines | This expression always evaluates to false. | +| UselessConditional.js:5:8:5:12 | lines | Variable 'lines' always evaluates to true here. | | UselessConditional.js:12:34:12:79 | (v = ne ... k] = v) | This logical 'and' expression can be replaced with a comma expression. | | UselessConditional.js:17:9:17:9 | a | Variable 'a' always evaluates to false here. | | UselessConditional.js:18:9:18:9 | b | Variable 'b' always evaluates to false here. | @@ -15,7 +15,8 @@ | UselessConditional.js:65:5:65:5 | x | Variable 'x' always evaluates to true here. | | UselessConditional.js:76:13:76:13 | x | Variable 'x' always evaluates to true here. | | UselessConditional.js:82:13:82:13 | x | Variable 'x' always evaluates to true here. | -| UselessConditional.js:89:10:89:16 | x, true | This expression always evaluates to true. | +| UselessConditional.js:94:16:94:16 | x | Variable 'x' always evaluates to false here. | +| UselessConditional.js:101:18:101:18 | x | Variable 'x' always evaluates to false here. | | UselessConditionalGood.js:58:12:58:13 | x2 | Variable 'x2' always evaluates to false here. | | UselessConditionalGood.js:69:12:69:13 | xy | Variable 'xy' always evaluates to false here. | | UselessConditionalGood.js:85:12:85:13 | xy | Variable 'xy' always evaluates to false here. | diff --git a/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.js b/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.js index 179014e5c770..7898b1a3c0f2 100644 --- a/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.js +++ b/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.js @@ -89,4 +89,17 @@ async function awaitFlow(){ if ((x, true)); }); +(function (x, y) { + if (!x) { + while (x) { // NOT OK + f(); + } + while (true) { // OK + break; + } + if (true && true) {} // OK + if (y && x) {} // NOT OK + } +}); + // semmle-extractor-options: --experimental