JS: whitelist trivial throwers in js/superfluous-trailing-arguments

Author: Esben Sparre Andreasen

change-notes/1.22/analysis-javascript.md

@@ -26,6 +26,7 @@
 | **Query**                      | **Expected impact**          | **Change**                                                                |
 |--------------------------------|------------------------------|---------------------------------------------------------------------------|
 | Shift out of range | Fewer false positive results | This rule now correctly handles BigInt shift operands. |
+| Superfluous trailing arguments | Fewer false-positive results. | This rule no longer flags calls to placeholder functions that trivially throw an exception. |
 
 ## Changes to QL libraries
 

javascript/ql/src/LanguageFeatures/SpuriousArguments.ql

@@ -94,5 +94,10 @@ where
     f instanceof ArrowFunctionExpr or // cannot be empty
     f instanceof ExternalFunction or // always empty
     f.isAmbient() // always empty
+  ) and
+  not (
+    // exclude no-param functions that trivially throw exceptions, they are probably placeholders
+    f.getNumParameter() = 0 and
+    f.getBodyStmt(0) instanceof ThrowStmt
   )
 select args, "Superfluous " + arguments + " passed to $@.", f, f.describe()

javascript/ql/test/query-tests/LanguageFeatures/SpuriousArguments/SpuriousArguments.expected

@@ -17,3 +17,8 @@
 | tst.js:76:31:76:32 | 42 | Superfluous argument passed to $@. | tst.js:64:33:64:32 | () {} | default constructor of class ImplicitEmptyConstructor |
 | tst.js:77:31:77:32 | 42 | Superfluous argument passed to $@. | tst.js:67:14:68:3 | (){\\n\\t\\t} | constructor of class ExplicitEmptyConstructor |
 | tst.js:78:20:78:21 | 10 | Superfluous argument passed to $@. | externs.js:36:1:36:27 | functio ... num) {} | function parseFloat |
+| tst.js:114:20:114:21 | 42 | Superfluous argument passed to $@. | tst.js:82:2:86:2 | functio ... \\n\\t\\t}\\n\\t} | function notAPlainThrower1 |
+| tst.js:115:20:115:21 | 42 | Superfluous argument passed to $@. | tst.js:87:2:90:2 | functio ... .");\\n\\t} | function notAPlainThrower2 |
+| tst.js:116:20:116:21 | 42 | Superfluous argument passed to $@. | tst.js:91:2:94:2 | functio ... .");\\n\\t} | function notAPlainThrower3 |
+| tst.js:120:23:120:24 | 87 | Superfluous argument passed to $@. | tst.js:102:2:104:2 | functio ... (p);\\n\\t} | function throwerWithParam |
+| tst.js:121:18:121:19 | 42 | Superfluous argument passed to $@. | tst.js:105:2:113:2 | functio ... )();\\n\\t} | function throwerIndirect |

javascript/ql/test/query-tests/LanguageFeatures/SpuriousArguments/tst.js

@@ -76,4 +76,47 @@ parseFloat("123", 10);
 	new ImplicitEmptyConstructor(42); // NOT OK
 	new ExplicitEmptyConstructor(42); // NOT OK
 	parseFloat("123", 10); // NOT OK
-})
+});
+
+(function testWhitelistThrowingFunctions() {
+	function notAPlainThrower1(){
+		if(DEBUG) {
+			throw new Error("Remove this statement and implement this function");
+		}
+	};
+	function notAPlainThrower2(){
+		f();
+		throw new Error("Internal error: should have thrown an exception before this.");
+	};
+	function notAPlainThrower3(){
+		return;
+		throw new Error("Internal error: should have returned before this.");
+	};
+	function thrower(){
+		throw new Error("Remove this statement and implement this function");
+	};
+	const throwerArrow = () => { throw new Error("Remove this statement and implement this function"); };
+	function throwerCustom(){
+		throw new MyError("Remove this statement and implement this function");
+	};
+	function throwerWithParam(p){
+		throw new Error(p);
+	};
+	function throwerIndirect(){
+		(function(){
+			{
+				{
+					throw Error("Remove this statement and implement this function");
+				}
+			}
+		})();
+	}
+	notAPlainThrower1(42); // NOT OK
+	notAPlainThrower2(42); // NOT OK
+	notAPlainThrower3(42); // NOT OK
+	thrower(42); // OK
+	throwerArrow(42); // OK
+	throwerCustom(42); // OK
+	throwerWithParam(42, 87); // NOT OK
+	throwerIndirect(42); // OK, but still flagged due to complexity
+});