Merge pull request #2203 from erik-krogh/ignorePureFunction

Approved by max-schaefer, mchammer01

Author: semmle-qlci

change-notes/1.23/analysis-javascript.md

@@ -27,6 +27,7 @@
 | Use of returnless function (`js/use-of-returnless-function`)              | maintainability, correctness                                      | Highlights calls where the return value is used, but the callee never returns a value. Results are shown on LGTM by default. |
 | Useless regular expression character escape (`js/useless-regexp-character-escape`) | correctness, security, external/cwe/cwe-20 | Highlights regular expression strings with useless character escapes, indicating a possible violation of [CWE-20](https://cwe.mitre.org/data/definitions/20.html). Results are shown on LGTM by default. |
 | Unreachable method overloads (`js/unreachable-method-overloads`)          | correctness, typescript                                           | Highlights method overloads that are impossible to use from client code. Results are shown on LGTM by default. |
+| Ignoring result from pure array method (`js/ignore-array-result`)         | maintainability, correctness                                      | Highlights calls to array methods without side effects where the return value is ignored. Results are shown on LGTM by default. |
 
 ## Changes to existing queries
 

javascript/config/suites/javascript/correctness-core

@@ -37,5 +37,6 @@
 + semmlecode-javascript-queries/RegExp/UnboundBackref.ql: /Correctness/Regular Expressions
 + semmlecode-javascript-queries/RegExp/UnmatchableCaret.ql: /Correctness/Regular Expressions
 + semmlecode-javascript-queries/RegExp/UnmatchableDollar.ql: /Correctness/Regular Expressions
++ semmlecode-javascript-queries/Statements/IgnoreArrayResult.ql: /Correctness/Statements
 + semmlecode-javascript-queries/Statements/InconsistentLoopOrientation.ql: /Correctness/Statements
 + semmlecode-javascript-queries/Statements/UnreachableStatement.ql: /Correctness/Statements

javascript/ql/src/Statements/IgnoreArrayResult.qhelp

@@ -0,0 +1,48 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+The <code>concat</code>, <code>join</code> and <code>slice</code> methods are
+pure and do not modify any of the inputs or the array the method is called 
+on. It is therefore generally an error to ignore the return value from a call
+to one of these methods. 
+</p>
+
+</overview>
+<recommendation>
+
+<p>
+Use the returned value from the calls to <code>concat</code>, <code>join</code> 
+or <code>slice</code>. 
+</p>
+
+</recommendation>
+<example>
+
+<p>
+A function <code>extend</code> is defined in the following example. The 
+function uses the <code>concat</code> method to add elements to the 
+<code>arr</code> array. However, the <code>extend</code> function has no 
+effect as the return value from <code>concat</code> is ignored:
+</p>
+
+<sample src="examples/IgnoreArrayResult.js" />
+
+<p>
+Assigning the returned value from the call to <code>concat</code> to the 
+<code>arr</code> variable fixes the error:
+</p>
+
+<sample src="examples/IgnoreArrayResultFixed.js" />
+
+</example>
+<references>
+
+<li>Mozilla Developer Network: <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/concat">Array concat</a>.</li>
+<li>Mozilla Developer Network: <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/slice">Array slice</a>.</li>
+<li>Mozilla Developer Network: <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/join">Array join</a>.</li>
+
+</references>
+</qhelp>

javascript/ql/src/Statements/IgnoreArrayResult.ql

@@ -0,0 +1,45 @@
+/**
+ * @name Ignoring result from pure array method
+ * @description Ignoring the result of an array method that does not modify its receiver is generally an error.
+ * @kind problem
+ * @problem.severity warning
+ * @id js/ignore-array-result
+ * @tags maintainability
+ *       correctness
+ * @precision high
+ */
+
+import javascript
+import Expressions.ExprHasNoEffect
+
+DataFlow::SourceNode callsArray(DataFlow::TypeBackTracker t, DataFlow::MethodCallNode call) {
+  isIgnoredPureArrayCall(call) and 
+  t.start() and
+  result = call.getReceiver().getALocalSource()
+  or
+  exists(DataFlow::TypeBackTracker t2 | result = callsArray(t2, call).backtrack(t2, t))
+}
+
+DataFlow::SourceNode callsArray(DataFlow::MethodCallNode call) {
+  result = callsArray(DataFlow::TypeBackTracker::end(), call)
+}
+
+predicate isIgnoredPureArrayCall(DataFlow::MethodCallNode call) {
+  inVoidContext(call.asExpr()) and
+  (
+    call.getMethodName() = "concat" and
+    call.getNumArgument() = 1
+    or
+    call.getMethodName() = "join" and
+    call.getNumArgument() < 2
+    or
+    call.getMethodName() = "slice" and
+    call.getNumArgument() < 3
+  )
+}
+
+from DataFlow::MethodCallNode call
+where 
+  callsArray(call) instanceof DataFlow::ArrayCreationNode and
+  not call.getReceiver().asExpr().(ArrayExpr).getSize() = 0
+select call, "Result from call to " + call.getMethodName() + " ignored."

javascript/ql/src/Statements/examples/IgnoreArrayResult.js

@@ -0,0 +1,5 @@
+var arr = [1,2,3];
+
+function extend(others) {
+	arr.concat(others);
+}

javascript/ql/src/Statements/examples/IgnoreArrayResultFixed.js

@@ -0,0 +1,5 @@
+var arr = [1,2,3];
+
+function extend(others) {
+	arr = arr.concat(others);
+}

javascript/ql/test/query-tests/Statements/IgnoreArrayResult/IgnoreArrayResult.expected

@@ -0,0 +1,2 @@
+| tst.js:3:1:3:19 | arr.concat([1,2,3]) | Result from call to concat ignored. |
+| tst.js:5:1:5:15 | arr.concat(arr) | Result from call to concat ignored. |

javascript/ql/test/query-tests/Statements/IgnoreArrayResult/IgnoreArrayResult.qlref

@@ -0,0 +1 @@
+Statements/IgnoreArrayResult.ql

javascript/ql/test/query-tests/Statements/IgnoreArrayResult/tst.js

@@ -0,0 +1,11 @@
+var arr = [1,2,3];
+
+arr.concat([1,2,3]); // NOT OK!
+
+arr.concat(arr); // NOT OK!
+
+console.log(arr.concat([1,2,3]));
+
+({concat: Array.prototype.concat}.concat(arr));
+
+[].concat([1,2,3]);