Merge pull request #307 from esben-semmle/js/unused-import

JS: make js/unused-local-variable flag import statements

Author: Max Schaefer

change-notes/1.19/analysis-javascript.md

@@ -35,6 +35,7 @@
 | Remote property injection | Fewer results | The precision of this rule has been revised to "medium". Results are no longer shown on LGTM by default. |
 | Missing CSRF middleware | Fewer false-positive results | This rule now recognizes additional CSRF protection middlewares. |
 | Server-side URL redirect | More results | This rule now recognizes redirection calls in more cases. |
+| Unused variable, import, function or class | Fewer results | This rule now flags import statements with multiple unused imports once. |
 | User-controlled bypass of security check | Fewer results | This rule no longer flags conditions that guard early returns. The precision of this rule has been revised to "medium". Results are no longer shown on LGTM by default. | 
 | Whitespace contradicts operator precedence | Fewer false-positive results | This rule no longer flags operators with asymmetric whitespace. |
 

javascript/ql/src/Declarations/UnusedVariable.ql

@@ -94,36 +94,95 @@ predicate isEnumMember(VarDecl decl) {
 }
 
 /**
- * Gets a description of the declaration `vd`, which is either of the form "function f" if
- * it is a function name, or "variable v" if it is not.
+ * Gets a description of the declaration `vd`, which is either of the form
+ * "function f", "variable v" or "class c".
  */
-string describe(VarDecl vd) {
+string describeVarDecl(VarDecl vd) {
   if vd = any(Function f).getId() then
     result = "function " + vd.getName()
   else if vd = any(ClassDefinition c).getIdentifier() then
     result = "class " + vd.getName()
-  else if (vd = any(ImportSpecifier im).getLocal() or vd = any(ImportEqualsDeclaration im).getId()) then
-    result = "import " + vd.getName()
   else
     result = "variable " + vd.getName()
 }
 
-from VarDecl vd, UnusedLocal v
-where v = vd.getVariable() and
-      // exclude variables mentioned in JSDoc comments in externs
-      not mentionedInJSDocComment(v) and
-      // exclude variables used to filter out unwanted properties
-      not isPropertyFilter(v) and
-      // exclude imports of React that are implicitly referenced by JSX
-      not isReactImportForJSX(v) and
-      // exclude names that are used as types
-      not isUsedAsType(vd) and
-      // exclude names that are used as namespaces from inside a type
-      not isUsedAsNamespace(vd) and
-      // exclude decorated functions and classes
-      not isDecorated(vd) and
-      // exclude names of enum members; they also define property names
-      not isEnumMember(vd) and
-      // ignore ambient declarations - too noisy
-      not vd.isAmbient()
-select vd, "Unused " + describe(vd) + "."
+/**
+ * An import statement that provides variable declarations.
+ */
+class ImportVarDeclProvider extends Stmt {
+
+  ImportVarDeclProvider() {
+    this instanceof ImportDeclaration or
+    this instanceof ImportEqualsDeclaration
+  }
+
+  /**
+   * Gets a variable declaration of this import.
+   */
+  VarDecl getAVarDecl() {
+    result = this.(ImportDeclaration).getASpecifier().getLocal() or
+    result = this.(ImportEqualsDeclaration).getId()
+  }
+
+  /**
+   * Gets an unacceptable unused variable declared by this import.
+   */
+  UnusedLocal getAnUnacceptableUnusedLocal() {
+    result = getAVarDecl().getVariable() and
+    not whitelisted(result)
+  }
+
+}
+
+/**
+ * Holds if it is acceptable that `v` is unused.
+ */
+predicate whitelisted(UnusedLocal v) {
+  // exclude variables mentioned in JSDoc comments in externs
+  mentionedInJSDocComment(v) or
+  // exclude variables used to filter out unwanted properties
+  isPropertyFilter(v) or
+  // exclude imports of React that are implicitly referenced by JSX
+  isReactImportForJSX(v) or
+  // exclude names that are used as types
+  exists (VarDecl vd |
+    v = vd.getVariable() |
+    isUsedAsType(vd) or
+    // exclude names that are used as namespaces from inside a type
+    isUsedAsNamespace(vd)or
+    // exclude decorated functions and classes
+    isDecorated(vd) or
+    // exclude names of enum members; they also define property names
+    isEnumMember(vd) or
+    // ignore ambient declarations - too noisy
+    vd.isAmbient()
+  )
+}
+
+/**
+ * Holds if `vd` declares an unused variable that does not come from an import statement, as explained by `msg`.
+ */
+predicate unusedNonImports(VarDecl vd, string msg) {
+  exists (UnusedLocal v |
+    v = vd.getVariable() and
+    msg = "Unused " + describeVarDecl(vd) + "." and
+    not vd = any(ImportVarDeclProvider p).getAVarDecl() and
+    not whitelisted(v)
+  )
+}
+
+/**
+ * Holds if `provider` declares one or more unused variables, as explained by `msg`.
+ */
+predicate unusedImports(ImportVarDeclProvider provider, string msg) {
+  exists (string imports, string names |
+    imports = pluralize("import", count(provider.getAnUnacceptableUnusedLocal())) and
+    names = strictconcat(provider.getAnUnacceptableUnusedLocal().getName(), ", ") and
+    msg = "Unused " + imports + " " + names + "."
+  )
+}
+
+from ASTNode sel, string msg
+where unusedNonImports(sel, msg) or
+      unusedImports(sel, msg)
+select sel, msg

javascript/ql/src/semmle/javascript/Util.qll

@@ -11,3 +11,16 @@ bindingset[s]
 string capitalize(string s) {
   result = s.charAt(0).toUpperCase() + s.suffix(1)
 }
+
+ /**
+  * Gets the pluralization for `n` occurrences of `noun`.
+  *
+  * For example, the pluralization of `"function"` for `n = 2` is `"functions"`.
+  */
+bindingset[noun, n]
+string pluralize(string noun, int n) {
+  if n = 1 then
+    result = noun
+  else
+    result = noun + "s"
+}

javascript/ql/test/library-tests/Util/capitalize.expected

@@ -0,0 +1 @@
+| X | X | Xx | XX | Xx | XX |

javascript/ql/test/library-tests/Util/capitalize.ql

@@ -0,0 +1,3 @@
+import semmle.javascript.Util
+
+select capitalize("x"), capitalize("X"), capitalize("xx"), capitalize("XX"), capitalize("Xx"), capitalize("xX")

javascript/ql/test/library-tests/Util/pluralize.expected

@@ -0,0 +1 @@
+| xs | x | xs | xs |

javascript/ql/test/library-tests/Util/pluralize.ql

@@ -0,0 +1,3 @@
+import semmle.javascript.Util
+
+select pluralize("x", 0), pluralize("x", 1), pluralize("x", 2), pluralize("x", -1)

javascript/ql/test/library-tests/Util/tst.js

@@ -0,0 +1 @@
+// used by qltest to identify the language

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

@@ -1,6 +1,8 @@
-| decorated.ts:1:9:1:21 | actionHandler | Unused import actionHandler. |
+| decorated.ts:1:1:1:126 | import  ... where'; | Unused import actionHandler. |
 | decorated.ts:4:10:4:12 | fun | Unused function fun. |
 | externs.js:6:5:6:13 | iAmUnused | Unused variable iAmUnused. |
+| multi-imports.js:1:1:1:29 | import  ... om 'x'; | Unused imports a, b, d. |
+| multi-imports.js:2:1:2:42 | import  ... om 'x'; | Unused imports alphabetically, ordered. |
 | typeoftype.ts:9:7:9:7 | y | Unused variable y. |
-| unusedShadowed.ts:1:8:1:8 | T | Unused import T. |
-| unusedShadowed.ts:2:8:2:13 | object | Unused import object. |
+| 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/multi-imports.js

@@ -0,0 +1,4 @@
+import {a, b, c, d} from 'x';
+import {ordered, alphabetically} from 'x';
+
+c();