From 592cb18bf486ba6f8b37abf291516e365d4f8e25 Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik@semmle.com>
Date: Fri, 4 Oct 2019 16:28:38 +0200
Subject: [PATCH 1/9] add array callbacks to useOfReturnlessFunction query

---
 .../ql/src/Expressions/ExprHasNoEffect.ql     | 115 +----------------
 .../ql/src/Expressions/ExprHasNoEffect.qll    | 121 ++++++++++++++++++
 .../src/Statements/UseOfReturnlessFunction.ql |  63 ++++++++-
 3 files changed, 178 insertions(+), 121 deletions(-)

diff --git a/javascript/ql/src/Expressions/ExprHasNoEffect.ql b/javascript/ql/src/Expressions/ExprHasNoEffect.ql
index 14024ec8f381..929e84616f07 100644
--- a/javascript/ql/src/Expressions/ExprHasNoEffect.ql
+++ b/javascript/ql/src/Expressions/ExprHasNoEffect.ql
@@ -13,122 +13,9 @@
  */
 
 import javascript
-import DOMProperties
-import semmle.javascript.frameworks.xUnit
-import semmle.javascript.RestrictedLocations
 import ExprHasNoEffect
 
-/**
- * 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 858f719ba0a0..822bcbb26fae 100644
--- a/javascript/ql/src/Expressions/ExprHasNoEffect.qll
+++ b/javascript/ql/src/Expressions/ExprHasNoEffect.qll
@@ -3,6 +3,9 @@
  */
 
 import javascript
+import DOMProperties
+import semmle.javascript.frameworks.xUnit
+import semmle.javascript.RestrictedLocations
 
 /**
  * Holds if `e` appears in a syntactic context where its value is discarded.
@@ -37,3 +40,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 c63127b83a37..61a485313e4d 100644
--- a/javascript/ql/src/Statements/UseOfReturnlessFunction.ql
+++ b/javascript/ql/src/Statements/UseOfReturnlessFunction.ql
@@ -82,17 +82,66 @@ predicate alwaysThrows(Function f) {
   )
 }
 
-from DataFlow::CallNode call
-where
+predicate callToVoidFunction(DataFlow::CallNode call, Function func) {
   not call.isIndefinite(_) and 
-  forex(Function f | f = call.getACallee() |
+  func = call.getACallee() and
+  forall(Function f | f = call.getACallee() |
     returnsVoid(f) and not isStub(f) and not alwaysThrows(f)
+  )
+}
+
+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()) }
+
+predicate voidArrayCallback(DataFlow::MethodCallNode call, Function func) {
+  hasNonVoidCallbackMethod(call.getMethodName()) and
+  func = call.getAnArgument().getALocalSource().asExpr() and
+  1 = count(DataFlow::Node arg | arg = call.getAnArgument() and arg.getALocalSource().asExpr() instanceof Function) and
+  returnsVoid(func) and
+  not isStub(func) and
+  not alwaysThrows(func) and
+  (
+    call.getReceiver().getALocalSource() = array()
+    or
+    call.getCalleeNode() 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 = "function " + call.getCalleeName()
+    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
-  
+  // Avoid double reporting from js/useless-expression
+  not hasNoEffect(func.getBodyStmt(func.getNumBodyStmt() - 1).(ExprStmt).getExpr()) 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

From a7c1c34e1e48a311057ae223d545ed59a54439ee Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Fri, 11 Oct 2019 17:14:58 +0200
Subject: [PATCH 2/9] fix test output, and add new test for array callbacks

---
 .../UseOfReturnlessFunction.expected                  | 11 ++++++-----
 .../Statements/UseOfReturnlessFunction/tst.js         |  7 +++++++
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/UseOfReturnlessFunction.expected b/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/UseOfReturnlessFunction.expected
index 500d900ff9e3..0dc283d5edc6 100644
--- a/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/UseOfReturnlessFunction.expected
+++ b/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/UseOfReturnlessFunction.expected
@@ -1,5 +1,6 @@
-| 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 bothOnlyHaveSideEffects |
+| 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 bothOnlyHaveSideEffects |
+| 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 |
diff --git a/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/tst.js b/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/tst.js
index 8bb49f7e7615..1306c6e7f65a 100644
--- a/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/tst.js
+++ b/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/tst.js
@@ -68,4 +68,11 @@
 	
 	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);
 })();
\ No newline at end of file

From 28056791a57d53a265cf6348d71e93a4aa60a4f8 Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Mon, 14 Oct 2019 14:14:26 +0200
Subject: [PATCH 3/9] add .getALocalSource() when testing for lodash-members

---
 javascript/ql/src/Statements/UseOfReturnlessFunction.ql     | 6 +++---
 .../UseOfReturnlessFunction.expected                        | 1 +
 .../query-tests/Statements/UseOfReturnlessFunction/tst.js   | 4 ++++
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/javascript/ql/src/Statements/UseOfReturnlessFunction.ql b/javascript/ql/src/Statements/UseOfReturnlessFunction.ql
index 61a485313e4d..2d70cecb0d3a 100644
--- a/javascript/ql/src/Statements/UseOfReturnlessFunction.ql
+++ b/javascript/ql/src/Statements/UseOfReturnlessFunction.ql
@@ -113,8 +113,8 @@ DataFlow::SourceNode array(DataFlow::TypeTracker t) {
 
 DataFlow::SourceNode array() { result = array(DataFlow::TypeTracker::end()) }
 
-predicate voidArrayCallback(DataFlow::MethodCallNode call, Function func) {
-  hasNonVoidCallbackMethod(call.getMethodName()) and
+predicate voidArrayCallback(DataFlow::CallNode call, Function func) {
+  hasNonVoidCallbackMethod(call.getCalleeName()) and
   func = call.getAnArgument().getALocalSource().asExpr() and
   1 = count(DataFlow::Node arg | arg = call.getAnArgument() and arg.getALocalSource().asExpr() instanceof Function) and
   returnsVoid(func) and
@@ -123,7 +123,7 @@ predicate voidArrayCallback(DataFlow::MethodCallNode call, Function func) {
   (
     call.getReceiver().getALocalSource() = array()
     or
-    call.getCalleeNode() instanceof LodashUnderscore::Member
+    call.getCalleeNode().getALocalSource() instanceof LodashUnderscore::Member
   )
 }
 
diff --git a/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/UseOfReturnlessFunction.expected b/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/UseOfReturnlessFunction.expected
index 0dc283d5edc6..98ae4e3696c7 100644
--- a/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/UseOfReturnlessFunction.expected
+++ b/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/UseOfReturnlessFunction.expected
@@ -4,3 +4,4 @@
 | 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 bothOnlyHaveSideEffects |
 | 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 bothOnlyHaveSideEffects |
 | 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 1306c6e7f65a..2b523223e91f 100644
--- a/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/tst.js
+++ b/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/tst.js
@@ -75,4 +75,8 @@
 	
 	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);
 })();
\ No newline at end of file

From 2e0244cda62deea17afe438c30e289d5c97913ca Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Mon, 21 Oct 2019 20:21:41 +0200
Subject: [PATCH 4/9] address review feedback

---
 .../ql/src/Expressions/ExprHasNoEffect.ql     |  1 +
 .../ql/src/Expressions/ExprHasNoEffect.qll    |  1 -
 .../src/Statements/UseOfReturnlessFunction.ql | 25 +++++++++++++++----
 .../UseOfReturnlessFunction.expected          |  8 +++---
 .../Statements/UseOfReturnlessFunction/tst.js |  3 +++
 5 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/javascript/ql/src/Expressions/ExprHasNoEffect.ql b/javascript/ql/src/Expressions/ExprHasNoEffect.ql
index 929e84616f07..917ab81a3e73 100644
--- a/javascript/ql/src/Expressions/ExprHasNoEffect.ql
+++ b/javascript/ql/src/Expressions/ExprHasNoEffect.ql
@@ -14,6 +14,7 @@
 
 import javascript
 import ExprHasNoEffect
+import semmle.javascript.RestrictedLocations
 
 
 from Expr e
diff --git a/javascript/ql/src/Expressions/ExprHasNoEffect.qll b/javascript/ql/src/Expressions/ExprHasNoEffect.qll
index 822bcbb26fae..bee980e5fd84 100644
--- a/javascript/ql/src/Expressions/ExprHasNoEffect.qll
+++ b/javascript/ql/src/Expressions/ExprHasNoEffect.qll
@@ -5,7 +5,6 @@
 import javascript
 import DOMProperties
 import semmle.javascript.frameworks.xUnit
-import semmle.javascript.RestrictedLocations
 
 /**
  * Holds if `e` appears in a syntactic context where its value is discarded.
diff --git a/javascript/ql/src/Statements/UseOfReturnlessFunction.ql b/javascript/ql/src/Statements/UseOfReturnlessFunction.ql
index 2d70cecb0d3a..fe03c66cf4ab 100644
--- a/javascript/ql/src/Statements/UseOfReturnlessFunction.ql
+++ b/javascript/ql/src/Statements/UseOfReturnlessFunction.ql
@@ -82,8 +82,19 @@ predicate alwaysThrows(Function f) {
   )
 }
 
+/**
+ * Holds if the last statement in the function is flagged by the js/useless-expression query.
+ */
+predicate alwaysHasNoEffect(Function f) {
+  exists(ReachableBasicBlock entry, DataFlow::Node noEffect |
+    entry = f.getEntryBB() and
+    hasNoEffect(noEffect.asExpr()) and
+    entry.dominates(noEffect.getBasicBlock())
+  )
+}
+
 predicate callToVoidFunction(DataFlow::CallNode call, Function func) {
-  not call.isIndefinite(_) and 
+  not call.isIncomplete() and 
   func = call.getACallee() and
   forall(Function f | f = call.getACallee() |
     returnsVoid(f) and not isStub(f) and not alwaysThrows(f)
@@ -113,6 +124,11 @@ DataFlow::SourceNode array(DataFlow::TypeTracker 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 return a value. 
+ */
 predicate voidArrayCallback(DataFlow::CallNode call, Function func) {
   hasNonVoidCallbackMethod(call.getCalleeName()) and
   func = call.getAnArgument().getALocalSource().asExpr() and
@@ -132,15 +148,14 @@ where
   (
     callToVoidFunction(call, func) and 
     msg = "the $@ does not return anything, yet the return value is used." and
-    name = "function " + call.getCalleeName()
+    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
+    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
-  // Avoid double reporting from js/useless-expression
-  not hasNoEffect(func.getBodyStmt(func.getNumBodyStmt() - 1).(ExprStmt).getExpr()) and
+  not alwaysHasNoEffect(func) and
   // anonymous one-shot closure. Those are used in weird ways and we ignore them.
   not oneshotClosure(call.asExpr())
 select
diff --git a/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/UseOfReturnlessFunction.expected b/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/UseOfReturnlessFunction.expected
index 98ae4e3696c7..f23b702bf202 100644
--- a/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/UseOfReturnlessFunction.expected
+++ b/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/UseOfReturnlessFunction.expected
@@ -1,7 +1,7 @@
 | 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 bothOnlyHaveSideEffects |
-| 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 bothOnlyHaveSideEffects |
-| 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 |
+| 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 2b523223e91f..8a6a44707edd 100644
--- a/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/tst.js
+++ b/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/tst.js
@@ -79,4 +79,7 @@
 	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

From db22916850202d1e159fc21e651670f501d29bc2 Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Tue, 22 Oct 2019 09:37:19 +0200
Subject: [PATCH 5/9] fix the alwaysHasNoEffect predicate, and rename it to
 lastStatementHasNoEffect

---
 .../ql/src/Statements/UseOfReturnlessFunction.ql     | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/javascript/ql/src/Statements/UseOfReturnlessFunction.ql b/javascript/ql/src/Statements/UseOfReturnlessFunction.ql
index fe03c66cf4ab..0f168e263ddd 100644
--- a/javascript/ql/src/Statements/UseOfReturnlessFunction.ql
+++ b/javascript/ql/src/Statements/UseOfReturnlessFunction.ql
@@ -85,11 +85,11 @@ predicate alwaysThrows(Function f) {
 /**
  * Holds if the last statement in the function is flagged by the js/useless-expression query.
  */
-predicate alwaysHasNoEffect(Function f) {
-  exists(ReachableBasicBlock entry, DataFlow::Node noEffect |
-    entry = f.getEntryBB() and
-    hasNoEffect(noEffect.asExpr()) and
-    entry.dominates(noEffect.getBasicBlock())
+predicate lastStatementHasNoEffect(Function f) {
+  exists(DataFlow::Node noEffect |
+    noEffect.getContainer() = f and
+    hasNoEffect(noEffect.asExpr()) and 
+    not exists(noEffect.getASuccessor())
   )
 }
 
@@ -155,7 +155,7 @@ where
     name = "callback function"
   ) and
   not benignContext(call.asExpr()) and
-  not alwaysHasNoEffect(func) and
+  not lastStatementHasNoEffect(func) and
   // anonymous one-shot closure. Those are used in weird ways and we ignore them.
   not oneshotClosure(call.asExpr())
 select

From ad3185c5586efc622cea59956868e729a545ae4a Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Tue, 22 Oct 2019 10:33:05 +0200
Subject: [PATCH 6/9] simplify lastStatementHasNoEffect and use the
 control-flow to determine which statement is the last

---
 javascript/ql/src/Statements/UseOfReturnlessFunction.ql | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/javascript/ql/src/Statements/UseOfReturnlessFunction.ql b/javascript/ql/src/Statements/UseOfReturnlessFunction.ql
index 0f168e263ddd..2fc3f01a560f 100644
--- a/javascript/ql/src/Statements/UseOfReturnlessFunction.ql
+++ b/javascript/ql/src/Statements/UseOfReturnlessFunction.ql
@@ -86,11 +86,7 @@ predicate alwaysThrows(Function f) {
  * Holds if the last statement in the function is flagged by the js/useless-expression query.
  */
 predicate lastStatementHasNoEffect(Function f) {
-  exists(DataFlow::Node noEffect |
-    noEffect.getContainer() = f and
-    hasNoEffect(noEffect.asExpr()) and 
-    not exists(noEffect.getASuccessor())
-  )
+  hasNoEffect(f.getExit().getAPredecessor())
 }
 
 predicate callToVoidFunction(DataFlow::CallNode call, Function func) {

From 841dac1abad86183a0c590fbe45eb486efd81039 Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Fri, 25 Oct 2019 17:46:55 +0200
Subject: [PATCH 7/9] address review feedback

---
 javascript/ql/src/Statements/UseOfReturnlessFunction.ql | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/javascript/ql/src/Statements/UseOfReturnlessFunction.ql b/javascript/ql/src/Statements/UseOfReturnlessFunction.ql
index 2fc3f01a560f..c01c6b5e3ebf 100644
--- a/javascript/ql/src/Statements/UseOfReturnlessFunction.ql
+++ b/javascript/ql/src/Statements/UseOfReturnlessFunction.ql
@@ -127,8 +127,10 @@ DataFlow::SourceNode array() { result = array(DataFlow::TypeTracker::end()) }
  */
 predicate voidArrayCallback(DataFlow::CallNode call, Function func) {
   hasNonVoidCallbackMethod(call.getCalleeName()) and
-  func = call.getAnArgument().getALocalSource().asExpr() and
-  1 = count(DataFlow::Node arg | arg = call.getAnArgument() and arg.getALocalSource().asExpr() instanceof Function) 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

From b2c31701f3cf6dfdfb900fdaeba38b5e0ced2444 Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Sun, 27 Oct 2019 09:07:45 +0100
Subject: [PATCH 8/9] add documentation to two predicates

---
 javascript/ql/src/Statements/UseOfReturnlessFunction.ql | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/javascript/ql/src/Statements/UseOfReturnlessFunction.ql b/javascript/ql/src/Statements/UseOfReturnlessFunction.ql
index c01c6b5e3ebf..77ec29ba9ee7 100644
--- a/javascript/ql/src/Statements/UseOfReturnlessFunction.ql
+++ b/javascript/ql/src/Statements/UseOfReturnlessFunction.ql
@@ -89,6 +89,9 @@ 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
@@ -97,6 +100,11 @@ predicate callToVoidFunction(DataFlow::CallNode call, Function func) {
   )
 }
 
+/**
+ * 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

From 6cac9619d3faeaa47b1db702425731058517dcec Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Mon, 4 Nov 2019 18:44:13 +0100
Subject: [PATCH 9/9] add missing not

Co-Authored-By: Max Schaefer <54907921+max-schaefer@users.noreply.github.com>
---
 javascript/ql/src/Statements/UseOfReturnlessFunction.ql | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/javascript/ql/src/Statements/UseOfReturnlessFunction.ql b/javascript/ql/src/Statements/UseOfReturnlessFunction.ql
index 77ec29ba9ee7..59d88bc51a01 100644
--- a/javascript/ql/src/Statements/UseOfReturnlessFunction.ql
+++ b/javascript/ql/src/Statements/UseOfReturnlessFunction.ql
@@ -131,7 +131,7 @@ 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 return a value. 
+ * but `func` does not return a value. 
  */
 predicate voidArrayCallback(DataFlow::CallNode call, Function func) {
   hasNonVoidCallbackMethod(call.getCalleeName()) and