diff --git a/javascript/ql/lib/Expressions/ExprHasNoEffect.qll b/javascript/ql/lib/Expressions/ExprHasNoEffect.qll index eff5fa7fc989..9813d9b32ed9 100644 --- a/javascript/ql/lib/Expressions/ExprHasNoEffect.qll +++ b/javascript/ql/lib/Expressions/ExprHasNoEffect.qll @@ -22,6 +22,9 @@ predicate inVoidContext(Expr e) { ) ) or + // propagate void context through parenthesized expressions + inVoidContext(e.getParent().(ParExpr)) + or exists(SeqExpr seq, int i, int n | e = seq.getOperand(i) and n = seq.getNumOperands() @@ -129,6 +132,19 @@ predicate noSideEffects(Expr e) { ) } +/** + * Holds if `e` is a compound expression that may contain sub-expressions with side effects. + * We should not flag these directly as useless since we want to flag only the innermost + * expressions that actually have no effect. + */ +predicate isCompoundExpression(Expr e) { + e instanceof LogicalBinaryExpr + or + e instanceof SeqExpr + or + e instanceof ParExpr +} + /** * Holds if the expression `e` should be reported as having no effect. */ @@ -145,6 +161,7 @@ predicate hasNoEffect(Expr e) { not isDeclaration(e) and // exclude DOM properties, which sometimes have magical auto-update properties not isDomProperty(e.(PropAccess).getPropertyName()) and + not isCompoundExpression(e) and // exclude xUnit.js annotations not e instanceof XUnitAnnotation and // exclude common patterns that are most likely intentional @@ -157,7 +174,17 @@ predicate hasNoEffect(Expr e) { not exists(fe.getName()) ) and // exclude block-level flow type annotations. For example: `(name: empty)`. - not e.(ParExpr).getExpression().getLastToken().getNextToken().getValue() = ":" and + not exists(ParExpr parent | + e.getParent() = parent and + e.getLastToken().getNextToken().getValue() = ":" + ) and + // exclude expressions that are part of a conditional expression + not exists(ConditionalExpr cond | e = cond.getABranch() | + e instanceof NullLiteral or + e.(GlobalVarAccess).getName() = "undefined" or + e.(NumberLiteral).getIntValue() = 0 or + e instanceof VoidExpr + ) and // exclude the first statement of a try block not e = any(TryStmt stmt).getBody().getStmt(0).(ExprStmt).getExpr() and // exclude expressions that are alone in a file, and file doesn't contain a function. diff --git a/javascript/ql/src/change-notes/2025-05-30-dom-property-access.md b/javascript/ql/src/change-notes/2025-05-30-dom-property-access.md new file mode 100644 index 000000000000..2dcb16a8327b --- /dev/null +++ b/javascript/ql/src/change-notes/2025-05-30-dom-property-access.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The `js/useless-expression` query now correctly flags only the innermost expressions with no effect, avoiding duplicate alerts on compound expressions. diff --git a/javascript/ql/test/query-tests/Expressions/ExprHasNoEffect/ExprHasNoEffect.expected b/javascript/ql/test/query-tests/Expressions/ExprHasNoEffect/ExprHasNoEffect.expected index 853e781c88e2..f1beafe0037a 100644 --- a/javascript/ql/test/query-tests/Expressions/ExprHasNoEffect/ExprHasNoEffect.expected +++ b/javascript/ql/test/query-tests/Expressions/ExprHasNoEffect/ExprHasNoEffect.expected @@ -1,3 +1,4 @@ +| dom.js:2:33:2:50 | a.clientTop === !0 | This expression has no effect. | | try.js:22:9:22:26 | x.ordinaryProperty | This expression has no effect. | | tst2.js:2:4:2:4 | 0 | This expression has no effect. | | tst.js:3:1:3:2 | 23 | This expression has no effect. | @@ -11,4 +12,4 @@ | tst.js:50:3:50:36 | new Err ... age(e)) | This expression has no effect. | | tst.js:61:2:61:20 | o.trivialNonGetter1 | This expression has no effect. | | tst.js:77:24:77:24 | o | This expression has no effect. | -| uselessfn.js:1:1:1:26 | (functi ... .");\\n}) | This expression has no effect. | +| uselessfn.js:1:2:1:26 | functio ... d.");\\n} | This expression has no effect. | diff --git a/javascript/ql/test/query-tests/Expressions/ExprHasNoEffect/dom.js b/javascript/ql/test/query-tests/Expressions/ExprHasNoEffect/dom.js new file mode 100644 index 000000000000..5d22e4a0bed2 --- /dev/null +++ b/javascript/ql/test/query-tests/Expressions/ExprHasNoEffect/dom.js @@ -0,0 +1,7 @@ +function f(){ + a.clientTop && a.clientTop, a.clientTop === !0; //$Alert + a && a.clientTop; + a.clientTop, a.clientTop; + if(a) return a.clientTop && a.clientTop, a.clientTop === !0; + if(b) return b && (b.clientTop, b.clientTop && b.clientTop), null; +} diff --git a/javascript/ql/test/query-tests/Expressions/ExprHasNoEffect/tst.js b/javascript/ql/test/query-tests/Expressions/ExprHasNoEffect/tst.js index a91759e553f1..6de5ac9a236a 100644 --- a/javascript/ql/test/query-tests/Expressions/ExprHasNoEffect/tst.js +++ b/javascript/ql/test/query-tests/Expressions/ExprHasNoEffect/tst.js @@ -79,4 +79,9 @@ function g() { consume(testSomeCondition() ? o : doSomethingDangerous()); + + ("release" === isRelease() ? warning() : null); + "release" === isRelease() ? warning() : null; + "release" === isRelease() ? warning() : 0; + "release" === isRelease() ? warning() : undefined; }; diff --git a/javascript/ql/test/query-tests/Expressions/ExprHasNoEffect/uselessfn.js b/javascript/ql/test/query-tests/Expressions/ExprHasNoEffect/uselessfn.js index 341644bf6498..e47a25458d44 100644 --- a/javascript/ql/test/query-tests/Expressions/ExprHasNoEffect/uselessfn.js +++ b/javascript/ql/test/query-tests/Expressions/ExprHasNoEffect/uselessfn.js @@ -1,3 +1,3 @@ (function f() { // $ Alert console.log("I'm never called."); -}) \ No newline at end of file +})