diff --git a/change-notes/1.19/analysis-javascript.md b/change-notes/1.19/analysis-javascript.md index d92f9c7db6f2..635771eea8dd 100644 --- a/change-notes/1.19/analysis-javascript.md +++ b/change-notes/1.19/analysis-javascript.md @@ -39,7 +39,6 @@ | 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 3f6e55cb20b4..a740e8628d8b 100644 --- a/javascript/ql/src/Statements/UselessConditional.ql +++ b/javascript/ql/src/Statements/UselessConditional.ql @@ -115,21 +115,25 @@ predicate whitelist(Expr e) { } /** - * Gets the `&&` expression that defines this guard node, if any. + * Holds if `e` is part of a conditional node `cond` that evaluates + * `e` and checks its value for truthiness. */ -LogAndExpr getLogAndExpr(ConditionGuardNode guard) { - result.getLeftOperand().stripParens() = guard.getTest() +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() } -from ConditionGuardNode guard, DataFlow::AnalyzedNode op, boolean cv, ASTNode sel, string msg -where guard.getTest() = op.asExpr() and +from ASTNode cond, DataFlow::AnalyzedNode op, boolean cv, ASTNode sel, string msg +where isConditional(cond, op.asExpr()) and cv = op.getTheBooleanValue()and not whitelist(op.asExpr()) and // if `cond` is of the form ` && `, // we suggest replacing it with `, ` - 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.") + 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.") // 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 5562d3a7c70b..de351ec51b9d 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:8:5:12 | lines | Variable 'lines' always evaluates to true here. | +| UselessConditional.js:5:7:5:12 | !lines | This expression always evaluates to false. | | 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,8 +15,7 @@ | 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: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. | +| UselessConditional.js:89:10:89:16 | x, true | This expression always evaluates to true. | | 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 7898b1a3c0f2..179014e5c770 100644 --- a/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.js +++ b/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.js @@ -89,17 +89,4 @@ 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