Merge pull request #2116 from erik-krogh/arrayCBRet

Approved by max-schaefer

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."

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())
+  )	
+}

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

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 |

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);
 })();