Merge pull request #656 from xiemaisi/js/unused-local-underscore

Approved by esben-semmle

Author: semmle-qlci

change-notes/1.20/analysis-javascript.md

@@ -20,6 +20,7 @@
 | **Query**                                  | **Expected impact**          | **Change**                                                                   |
 |--------------------------------------------|------------------------------|------------------------------------------------------------------------------|
 | Client-side cross-site scripting           | More results                 | This rule now recognizes WinJS functions that are vulnerable to HTML injection. |
-| Unused variable, import, function or class | Fewer false-positive results | This rule now flags fewer variables that are implictly used by JSX elements. |
+| Unused parameter                           | Fewer false-positive results | This rule no longer flags parameters with leading underscore. |
+| Unused variable, import, function or class | Fewer false-positive results | This rule now flags fewer variables that are implictly used by JSX elements, and no longer flags variables with leading underscore. |
 
 ## Changes to QL libraries

javascript/ql/src/Declarations/UnusedParameter.ql

@@ -9,7 +9,7 @@
  */
 
 import javascript
-import UnusedParameter // local library
+import UnusedParameter
 
 from Parameter p
 where isAnAccidentallyUnusedParameter(p)

javascript/ql/src/Declarations/UnusedParameter.qll

@@ -46,7 +46,9 @@ predicate isUnused(Function f, Parameter p, Variable pv, int i) {
   // functions without a body cannot use their parameters
   f.hasBody() and
   // field parameters are used to initialize a field
-  not p instanceof FieldParameter
+  not p instanceof FieldParameter and
+  // common convention: parameters with leading underscore are intentionally unused
+  pv.getName().charAt(0) != "_"
 }
 
 /**

javascript/ql/src/Declarations/UnusedVariable.ql

@@ -22,7 +22,9 @@ class UnusedLocal extends LocalVariable {
     not exists(FunctionExpr fe | this = fe.getVariable()) and
     not exists(ClassExpr ce | this = ce.getVariable()) and
     not exists(ExportDeclaration ed | ed.exportsAs(this, _)) and
-    not exists(LocalVarTypeAccess type | type.getVariable() = this)
+    not exists(LocalVarTypeAccess type | type.getVariable() = this) and
+    // common convention: variables with leading underscore are intentionally unused
+    getName().charAt(0) != "_"
   }
 }
 

javascript/ql/test/query-tests/Declarations/UnusedParameter/tst.js

@@ -33,4 +33,8 @@ var g = f3;
 // OK
 define(function (require, exports, module) {
 	module.x = 23;
-});
+});
+
+// OK: starts with underscore
+function f(_p) {
+}

javascript/ql/test/query-tests/Declarations/UnusedVariable/UnusedVariable.expected

@@ -9,5 +9,7 @@
 | multi-imports.js:2:1:2:42 | import  ... om 'x'; | Unused imports alphabetically, ordered. |
 | require-react-in-other-scope.js:2:9:2:13 | React | Unused variable React. |
 | typeoftype.ts:9:7:9:7 | y | Unused variable y. |
+| underscore.js:6:7:6:7 | e | Unused variable e. |
+| underscore.js:7:7:7:7 | f | Unused variable f. |
 | unusedShadowed.ts:1:1:1:26 | import  ... where'; | Unused import T. |
 | unusedShadowed.ts:2:1:2:31 | import  ... where'; | Unused import object. |

javascript/ql/test/query-tests/Declarations/UnusedVariable/underscore.js

@@ -0,0 +1,10 @@
+function f(a) {
+  const [a,  // OK: used
+	     _,  // OK: starts with underscore
+	     _c, // OK: starts with underscore
+	     d,  // OK: used
+	     e,  // NOT OK
+	     f]  // NOT OK
+         = a;
+  return a + d;
+}