add query for detecting ignored calls to Array.prototype.concat

Author: erik-krogh

change-notes/1.23/analysis-javascript.md

@@ -23,6 +23,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 return from concat (`js/ignore-return-from-concat`)              | maintainability, correctness                                      | Highlights calls to the concat method on array where the return value is ignored. Results are shown on LGTM by default. |
 
 ## Changes to existing queries
 

javascript/ql/src/Statements/IgnoreConcatReturn.qhelp

@@ -0,0 +1,44 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+The <code>concat</code> method on is pure and does not modify any of the input 
+arrays. It is therefore generally an error to ignore the return value from a 
+call to  <code>concat</code>. 
+</p>
+
+</overview>
+<recommendation>
+
+<p>
+Use the returned value from the call to <code>concat</code>. 
+</p>
+
+</recommendation>
+<example>
+
+<p>
+In the following example a function <code>extend</code> is defined. 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/IgnoreConcat.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/IgnoreConcatFixed.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>
+
+</references>
+</qhelp>

javascript/ql/src/Statements/IgnoreConcatReturn.ql

@@ -0,0 +1,42 @@
+/**
+ * @name Ignoring return from concat
+ * @description The concat method does not modify an array, ignoring the result of a call to concat is therefore generally an error.
+ * @kind problem
+ * @problem.severity warning
+ * @id js/ignore-result-from-concat
+ * @tags maintainability,
+ *       correctness
+ * @precision high
+ */
+
+import javascript
+import Expressions.ExprHasNoEffect
+
+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 isArrayMethod(DataFlow::MethodCallNode call) {
+  call.getReceiver().getALocalSource() = array()
+}
+
+predicate isIncomplete(DataFlow::Node node) {
+  any(DataFlow::Incompleteness cause | node.analyze().getAValue().isIndefinite(cause)) != "global"
+}
+
+from DataFlow::CallNode call
+where 
+  isArrayMethod(call) and
+  call.getCalleeName() = "concat" and
+  call.getNumArgument() = 1 and 
+  (call.getArgument(0).getALocalSource() = array() or isIncomplete(call.getArgument(0))) and 
+  not call.getArgument(0).asExpr().(ArrayExpr).getSize() = 0 and 
+  inVoidContext(call.asExpr())
+select call, "Return value from call to concat ignored."

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

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

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

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

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

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

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

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

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

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