Merge pull request #557 from esben-semmle/js/unused-react-variable

Approved by xiemaisi

Author: semmle-qlci

change-notes/1.20/analysis-javascript.md

@@ -0,0 +1,18 @@
+# Improvements to JavaScript analysis
+
+## General improvements
+
+## New queries
+
+| **Query** | **Tags** | **Purpose** |
+|-----------|----------|-------------|
+|           |          |             |
+
+## Changes to existing queries
+
+| **Query**                                  | **Expected impact**          | **Change**                                                                   |
+|--------------------------------------------|------------------------------|------------------------------------------------------------------------------|
+| Unused variable, import, function or class | Fewer false-positive results | This rule now flags fewer variables that are implictly used by JSX elements. |
+|                                            |                              |                                                                              |
+
+## Changes to QL libraries

javascript/ql/src/Declarations/UnusedVariable.ql

@@ -49,24 +49,29 @@ predicate isPropertyFilter(UnusedLocal v) {
   )
 }
 
+predicate hasJsxInScope(UnusedLocal v) {
+  any(JSXNode n).getParent+() = v.getScope().getScopeElement()
+}
+
 /**
- * Holds if `v` is an import of React, and there is a JSX element that implicitly
- * references it.
- */
-predicate isReactImportForJSX(UnusedLocal v) {
-  exists (ImportSpecifier is |
-    is.getLocal() = v.getADeclaration() and
-    exists (JSXNode jsx | jsx.getTopLevel() = is.getTopLevel())
-    |
+ * Holds if `v` is a "React" variable that is implicitly used by a JSX element.
+*/
+predicate isReactForJSX(UnusedLocal v) {
+  hasJsxInScope(v) and
+  (
     v.getName() = "React"
     or
-    // legacy `@jsx` pragmas
-    exists (JSXPragma p | p.getTopLevel() = is.getTopLevel() | p.getDOMName() = v.getName())
-    or
-    // JSX pragma from a .babelrc file
-    exists (Babel::TransformReactJsxConfig plugin |
-      plugin.appliesTo(is.getTopLevel()) and
-      plugin.getJsxFactoryVariableName() = v.getName())
+    exists(TopLevel tl |
+      tl = v.getADeclaration().getTopLevel() |
+      // legacy `@jsx` pragmas
+      exists(JSXPragma p | p.getTopLevel() = tl | p.getDOMName() = v.getName())
+      or
+      // JSX pragma from a .babelrc file
+      exists(Babel::TransformReactJsxConfig plugin |
+        plugin.appliesTo(tl) and
+        plugin.getJsxFactoryVariableName() = v.getName()
+      )
+    )
   )
 }
 
@@ -150,7 +155,7 @@ predicate whitelisted(UnusedLocal v) {
   // exclude variables used to filter out unwanted properties
   isPropertyFilter(v) or
   // exclude imports of React that are implicitly referenced by JSX
-  isReactImportForJSX(v) or
+  isReactForJSX(v) or
   // exclude names that are used as types
   exists (VarDecl vd |
     v = vd.getVariable() |

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

@@ -7,6 +7,7 @@
 | importWithoutPragma.jsx:1:1:1:27 | import  ... react'; | Unused import h. |
 | 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. |
+| require-react-in-other-scope.js:2:9:2:13 | React | Unused variable React. |
 | typeoftype.ts:9:7:9:7 | y | Unused variable y. |
 | 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/react-jsx.js

@@ -0,0 +1,2 @@
+var React = x; // OK
+(<e/>);

javascript/ql/test/query-tests/Declarations/UnusedVariable/require-react-1.js

@@ -0,0 +1,2 @@
+var React = require("probably-react"); // OK
+(<e/>);

javascript/ql/test/query-tests/Declarations/UnusedVariable/require-react-2.js

@@ -0,0 +1,2 @@
+var { React } = { React: require("probably-react") }; // OK
+(<e/>);

javascript/ql/test/query-tests/Declarations/UnusedVariable/require-react-3.js

@@ -0,0 +1,2 @@
+var { React } = require("probably-react"); // OK
+(<e/>);

javascript/ql/test/query-tests/Declarations/UnusedVariable/require-react-in-other-scope.js

@@ -0,0 +1,6 @@
+(function() {
+    var React  = require("probably-react"); // NOT OK
+})
+(function() {
+    (<e/>);
+})