diff --git a/javascript/ql/src/Expressions/ExprHasNoEffect.ql b/javascript/ql/src/Expressions/ExprHasNoEffect.ql index 14024ec8f38..917ab81a3e7 100644 --- a/javascript/ql/src/Expressions/ExprHasNoEffect.ql +++ b/javascript/ql/src/Expressions/ExprHasNoEffect.ql @@ -13,122 +13,10 @@ */ import javascript -import DOMProperties -import semmle.javascript.frameworks.xUnit -import semmle.javascript.RestrictedLocations import ExprHasNoEffect +import semmle.javascript.RestrictedLocations -/** - * Holds if `e` is of the form `x;` or `e.p;` and has a JSDoc comment containing a tag. - * In that case, it is probably meant as a declaration and shouldn't be flagged by this query. - * - * This will still flag cases where the JSDoc comment contains no tag at all (and hence carries - * no semantic information), and expression statements with an ordinary (non-JSDoc) comment - * attached to them. - */ -predicate isDeclaration(Expr e) { - (e instanceof VarAccess or e instanceof PropAccess) and - exists(e.getParent().(ExprStmt).getDocumentation().getATag()) -} - -/** - * Holds if there exists a getter for a property called `name` anywhere in the program. - */ -predicate isGetterProperty(string name) { - // there is a call of the form `Object.defineProperty(..., name, descriptor)` ... - exists(CallToObjectDefineProperty defProp | name = defProp.getPropertyName() | - // ... where `descriptor` defines a getter - defProp.hasPropertyAttributeWrite("get", _) - or - // ... where `descriptor` may define a getter - exists(DataFlow::SourceNode descriptor | descriptor.flowsTo(defProp.getPropertyDescriptor()) | - descriptor.isIncomplete(_) - or - // minimal escape analysis for the descriptor - exists(DataFlow::InvokeNode invk | - not invk = defProp and - descriptor.flowsTo(invk.getAnArgument()) - ) - ) - ) - or - // there is an object expression with a getter property `name` - exists(ObjectExpr obj | obj.getPropertyByName(name) instanceof PropertyGetter) -} - -/** - * A property access that may invoke a getter. - */ -class GetterPropertyAccess extends PropAccess { - override predicate isImpure() { isGetterProperty(getPropertyName()) } -} - -/** - * Holds if `c` is an indirect eval call of the form `(dummy, eval)(...)`, where - * `dummy` is some expression whose value is discarded, and which simply - * exists to prevent the call from being interpreted as a direct eval. - */ -predicate isIndirectEval(CallExpr c, Expr dummy) { - exists(SeqExpr seq | seq = c.getCallee().stripParens() | - dummy = seq.getOperand(0) and - seq.getOperand(1).(GlobalVarAccess).getName() = "eval" and - seq.getNumOperands() = 2 - ) -} - -/** - * Holds if `c` is a call of the form `(dummy, e[p])(...)`, where `dummy` is - * some expression whose value is discarded, and which simply exists - * to prevent the call from being interpreted as a method call. - */ -predicate isReceiverSuppressingCall(CallExpr c, Expr dummy, PropAccess callee) { - exists(SeqExpr seq | seq = c.getCallee().stripParens() | - dummy = seq.getOperand(0) and - seq.getOperand(1) = callee and - seq.getNumOperands() = 2 - ) -} - -/** - * Holds if evaluating `e` has no side effects (except potentially allocating - * and initializing a new object). - * - * For calls, we do not check whether their arguments have any side effects: - * even if they do, the call itself is useless and should be flagged by this - * query. - */ -predicate noSideEffects(Expr e) { - e.isPure() - or - // `new Error(...)`, `new SyntaxError(...)`, etc. - forex(Function f | f = e.flow().(DataFlow::NewNode).getACallee() | - f.(ExternalType).getASupertype*().getName() = "Error" - ) -} from Expr e -where - noSideEffects(e) and - inVoidContext(e) and - // disregard pure expressions wrapped in a void(...) - not e instanceof VoidExpr and - // filter out directives (unknown directives are handled by UnknownDirective.ql) - not exists(Directive d | e = d.getExpr()) and - // or about externs - not e.inExternsFile() and - // don't complain about declarations - not isDeclaration(e) and - // exclude DOM properties, which sometimes have magical auto-update properties - not isDOMProperty(e.(PropAccess).getPropertyName()) and - // exclude xUnit.js annotations - not e instanceof XUnitAnnotation and - // exclude common patterns that are most likely intentional - not isIndirectEval(_, e) and - not isReceiverSuppressingCall(_, e, _) and - // exclude anonymous function expressions as statements; these can only arise - // from a syntax error we already flag - not exists(FunctionExpr fe, ExprStmt es | fe = e | - fe = es.getExpr() and - not exists(fe.getName()) - ) +where hasNoEffect(e) select e.(FirstLineOf), "This expression has no effect." diff --git a/javascript/ql/src/Expressions/ExprHasNoEffect.qll b/javascript/ql/src/Expressions/ExprHasNoEffect.qll index 858f719ba0a..bee980e5fd8 100644 --- a/javascript/ql/src/Expressions/ExprHasNoEffect.qll +++ b/javascript/ql/src/Expressions/ExprHasNoEffect.qll @@ -3,6 +3,8 @@ */ import javascript +import DOMProperties +import semmle.javascript.frameworks.xUnit /** * Holds if `e` appears in a syntactic context where its value is discarded. @@ -37,3 +39,121 @@ predicate inVoidContext(Expr e) { or exists(LogicalBinaryExpr logical | e = logical.getRightOperand() and inVoidContext(logical)) } + + +/** + * Holds if `e` is of the form `x;` or `e.p;` and has a JSDoc comment containing a tag. + * In that case, it is probably meant as a declaration and shouldn't be flagged by this query. + * + * This will still flag cases where the JSDoc comment contains no tag at all (and hence carries + * no semantic information), and expression statements with an ordinary (non-JSDoc) comment + * attached to them. + */ +predicate isDeclaration(Expr e) { + (e instanceof VarAccess or e instanceof PropAccess) and + exists(e.getParent().(ExprStmt).getDocumentation().getATag()) +} + +/** + * Holds if there exists a getter for a property called `name` anywhere in the program. + */ +predicate isGetterProperty(string name) { + // there is a call of the form `Object.defineProperty(..., name, descriptor)` ... + exists(CallToObjectDefineProperty defProp | name = defProp.getPropertyName() | + // ... where `descriptor` defines a getter + defProp.hasPropertyAttributeWrite("get", _) + or + // ... where `descriptor` may define a getter + exists(DataFlow::SourceNode descriptor | descriptor.flowsTo(defProp.getPropertyDescriptor()) | + descriptor.isIncomplete(_) + or + // minimal escape analysis for the descriptor + exists(DataFlow::InvokeNode invk | + not invk = defProp and + descriptor.flowsTo(invk.getAnArgument()) + ) + ) + ) + or + // there is an object expression with a getter property `name` + exists(ObjectExpr obj | obj.getPropertyByName(name) instanceof PropertyGetter) +} + +/** + * A property access that may invoke a getter. + */ +class GetterPropertyAccess extends PropAccess { + override predicate isImpure() { isGetterProperty(getPropertyName()) } +} + +/** + * Holds if `c` is an indirect eval call of the form `(dummy, eval)(...)`, where + * `dummy` is some expression whose value is discarded, and which simply + * exists to prevent the call from being interpreted as a direct eval. + */ +predicate isIndirectEval(CallExpr c, Expr dummy) { + exists(SeqExpr seq | seq = c.getCallee().stripParens() | + dummy = seq.getOperand(0) and + seq.getOperand(1).(GlobalVarAccess).getName() = "eval" and + seq.getNumOperands() = 2 + ) +} + +/** + * Holds if `c` is a call of the form `(dummy, e[p])(...)`, where `dummy` is + * some expression whose value is discarded, and which simply exists + * to prevent the call from being interpreted as a method call. + */ +predicate isReceiverSuppressingCall(CallExpr c, Expr dummy, PropAccess callee) { + exists(SeqExpr seq | seq = c.getCallee().stripParens() | + dummy = seq.getOperand(0) and + seq.getOperand(1) = callee and + seq.getNumOperands() = 2 + ) +} + +/** + * Holds if evaluating `e` has no side effects (except potentially allocating + * and initializing a new object). + * + * For calls, we do not check whether their arguments have any side effects: + * even if they do, the call itself is useless and should be flagged by this + * query. + */ +predicate noSideEffects(Expr e) { + e.isPure() + or + // `new Error(...)`, `new SyntaxError(...)`, etc. + forex(Function f | f = e.flow().(DataFlow::NewNode).getACallee() | + f.(ExternalType).getASupertype*().getName() = "Error" + ) +} + +/** + * Holds if the expression `e` should be reported as having no effect. + */ +predicate hasNoEffect(Expr e) { + noSideEffects(e) and + inVoidContext(e) and + // disregard pure expressions wrapped in a void(...) + not e instanceof VoidExpr and + // filter out directives (unknown directives are handled by UnknownDirective.ql) + not exists(Directive d | e = d.getExpr()) and + // or about externs + not e.inExternsFile() and + // don't complain about declarations + not isDeclaration(e) and + // exclude DOM properties, which sometimes have magical auto-update properties + not isDOMProperty(e.(PropAccess).getPropertyName()) and + // exclude xUnit.js annotations + not e instanceof XUnitAnnotation and + // exclude common patterns that are most likely intentional + not isIndirectEval(_, e) and + not isReceiverSuppressingCall(_, e, _) and + // exclude anonymous function expressions as statements; these can only arise + // from a syntax error we already flag + not exists(FunctionExpr fe, ExprStmt es | fe = e | + fe = es.getExpr() and + not exists(fe.getName()) + ) +} \ No newline at end of file diff --git a/javascript/ql/src/Statements/UseOfReturnlessFunction.ql b/javascript/ql/src/Statements/UseOfReturnlessFunction.ql index c586cfe2497..f8f459cc84d 100644 --- a/javascript/ql/src/Statements/UseOfReturnlessFunction.ql +++ b/javascript/ql/src/Statements/UseOfReturnlessFunction.ql @@ -82,17 +82,87 @@ predicate alwaysThrows(Function f) { ) } -from DataFlow::CallNode call -where - not call.isIndefinite(_) and - forex(Function f | f = call.getACallee() | +/** + * Holds if the last statement in the function is flagged by the js/useless-expression query. + */ +predicate lastStatementHasNoEffect(Function f) { + hasNoEffect(f.getExit().getAPredecessor()) +} + +/** + * Holds if `func` is a callee of `call`, and all possible callees of `call` never return a value. + */ +predicate callToVoidFunction(DataFlow::CallNode call, Function func) { + not call.isIncomplete() and + func = call.getACallee() and + forall(Function f | f = call.getACallee() | returnsVoid(f) and not isStub(f) and not alwaysThrows(f) + ) +} + +/** + * Holds if `name` is the name of a method from `Array.prototype` or Lodash, + * where that method takes a callback as parameter, + * and the callback is expected to return a value. + */ +predicate hasNonVoidCallbackMethod(string name) { + name = "every" or + name = "filter" or + name = "find" or + name = "findIndex" or + name = "flatMap" or + name = "map" or + name = "reduce" or + name = "reduceRight" or + name = "some" or + name = "sort" +} + +DataFlow::SourceNode array(DataFlow::TypeTracker t) { + t.start() and result instanceof DataFlow::ArrayCreationNode + or + exists (DataFlow::TypeTracker t2 | + result = array(t2).track(t2, t) + ) +} + +DataFlow::SourceNode array() { result = array(DataFlow::TypeTracker::end()) } + +/** + * Holds if `call` is an Array or Lodash method accepting a callback `func`, + * where the `call` expects a callback that returns an expression, + * but `func` does not return a value. + */ +predicate voidArrayCallback(DataFlow::CallNode call, Function func) { + hasNonVoidCallbackMethod(call.getCalleeName()) and + exists(int index | + index = min(int i | exists(call.getCallback(i))) and + func = call.getCallback(index).getFunction() + ) and + returnsVoid(func) and + not isStub(func) and + not alwaysThrows(func) and + ( + call.getReceiver().getALocalSource() = array() + or + call.getCalleeNode().getALocalSource() instanceof LodashUnderscore::Member + ) +} + +from DataFlow::CallNode call, Function func, string name, string msg +where + ( + callToVoidFunction(call, func) and + msg = "the $@ does not return anything, yet the return value is used." and + name = func.describe() + or + voidArrayCallback(call, func) and + msg = "the $@ does not return anything, yet the return value from the call to " + call.getCalleeName() + " is used." and + name = "callback function" ) and - not benignContext(call.asExpr()) and - + not lastStatementHasNoEffect(func) and // anonymous one-shot closure. Those are used in weird ways and we ignore them. not oneshotClosure(call.asExpr()) select - call, "the function $@ does not return anything, yet the return value is used.", call.getACallee(), call.getCalleeName() - + call, msg, func, name diff --git a/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/UseOfReturnlessFunction.expected b/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/UseOfReturnlessFunction.expected index 500d900ff9e..f23b702bf20 100644 --- a/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/UseOfReturnlessFunction.expected +++ b/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/UseOfReturnlessFunction.expected @@ -1,5 +1,7 @@ -| tst.js:20:17:20:33 | onlySideEffects() | the function $@ does not return anything, yet the return value is used. | tst.js:11:5:13:5 | functio ... )\\n } | onlySideEffects | -| tst.js:24:13:24:29 | onlySideEffects() | the function $@ does not return anything, yet the return value is used. | tst.js:11:5:13:5 | functio ... )\\n } | onlySideEffects | -| tst.js:30:20:30:36 | onlySideEffects() | the function $@ does not return anything, yet the return value is used. | tst.js:11:5:13:5 | functio ... )\\n } | onlySideEffects | -| tst.js:53:10:53:34 | bothOnl ... fects() | the function $@ does not return anything, yet the return value is used. | tst.js:11:5:13:5 | functio ... )\\n } | bothOnlyHaveSideEffects | -| tst.js:53:10:53:34 | bothOnl ... fects() | the function $@ does not return anything, yet the return value is used. | tst.js:48:2:50:5 | functio ... )\\n } | bothOnlyHaveSideEffects | +| tst.js:20:17:20:33 | onlySideEffects() | the $@ does not return anything, yet the return value is used. | tst.js:11:5:13:5 | functio ... )\\n } | function onlySideEffects | +| tst.js:24:13:24:29 | onlySideEffects() | the $@ does not return anything, yet the return value is used. | tst.js:11:5:13:5 | functio ... )\\n } | function onlySideEffects | +| tst.js:30:20:30:36 | onlySideEffects() | the $@ does not return anything, yet the return value is used. | tst.js:11:5:13:5 | functio ... )\\n } | function onlySideEffects | +| tst.js:53:10:53:34 | bothOnl ... fects() | the $@ does not return anything, yet the return value is used. | tst.js:11:5:13:5 | functio ... )\\n } | function onlySideEffects | +| tst.js:53:10:53:34 | bothOnl ... fects() | the $@ does not return anything, yet the return value is used. | tst.js:48:2:50:5 | functio ... )\\n } | function onlySideEffects2 | +| tst.js:76:12:76:46 | [1,2,3] ... n, 3)}) | the $@ does not return anything, yet the return value from the call to filter is used. | tst.js:76:27:76:45 | n => {equals(n, 3)} | callback function | +| tst.js:80:12:80:50 | filter( ... 3) } ) | the $@ does not return anything, yet the return value from the call to filter is used. | tst.js:80:28:80:48 | x => { ... x, 3) } | callback function | diff --git a/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/tst.js b/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/tst.js index 8bb49f7e761..8a6a44707ed 100644 --- a/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/tst.js +++ b/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/tst.js @@ -68,4 +68,18 @@ var h = returnsValue() || alwaysThrows(); // OK! console.log(h); + + function equals(x, y) { + return x === y; + } + + var foo = [1,2,3].filter(n => {equals(n, 3)}) // NOT OK! + console.log(foo); + + import { filter } from 'lodash' + var bar = filter([1,2,4], x => { equals(x, 3) } ) // NOT OK! + console.log(bar); + + var baz = [1,2,3].filter(n => {n === 3}) // OK + console.log(baz); })(); \ No newline at end of file