JavaScript: Improve CFG for non-toplevel imports

change-notes/1.19/analysis-javascript.md

@@ -9,6 +9,7 @@
 * Support for popular libraries has been improved. Consequently, queries may produce more results on code bases that use the following features:
   - file system access, for example through [fs-extra](https://github.com/jprichardson/node-fs-extra) or [globby](https://www.npmjs.com/package/globby)
 
+* The type inference now handles nested imports (that is, imports not appearing at the toplevel). This may yield fewer false-positive results on projects that use this non-standard language feature.
 
 ## New queries
 

javascript/ql/src/semmle/javascript/CFG.qll

@@ -281,7 +281,7 @@ import javascript
  */
 class ControlFlowNode extends @cfg_node, Locatable {
   /** Gets a node succeeding this node in the CFG. */
-  ControlFlowNode getASuccessor() {
+  cached ControlFlowNode getASuccessor() {
     successor(this, result)
   }
 
@@ -457,3 +457,47 @@ class ConcreteControlFlowNode extends ControlFlowNode {
     not this instanceof SyntheticControlFlowNode
   }
 }
+
+/**
+ * A CFG node corresponding to a nested (that is, non-toplevel) import declaration.
+ *
+ * This is a non-standard language feature that is not currently supported very well
+ * by the extractor, in particular such imports do not appear in the control flow graph
+ * generated by the extractor. We patch them in by overriding `getASuccessor`; once an
+ * extractor fix becomes available, this class can be removed.
+ */
+private class NestedImportDeclaration extends ImportDeclaration {
+  NestedImportDeclaration() {
+    exists (ASTNode p | p = getParent() |
+      not p instanceof TopLevel
+    ) and
+    // if there are no specifiers, the default control flow graph is fine
+    exists(getASpecifier())
+  }
+
+  override ControlFlowNode getASuccessor() {
+    result = getSpecifier(0).getFirstControlFlowNode()
+  }
+}
+
+/**
+ * A CFG node corresponding to an import specifier in a nested import declaration.
+ *
+ * As for `NestedImportDeclaration` above, this is a temporary workaround that will be
+ * removed once extractor support for this non-standard language feature becomes available.
+ */
+private class NestedImportSpecifier extends ImportSpecifier {
+  NestedImportDeclaration decl;
+  int i;
+
+  NestedImportSpecifier() {
+    this = decl.getSpecifier(i)
+  }
+
+  override ControlFlowNode getASuccessor() {
+    result = decl.getSpecifier(i+1).getFirstControlFlowNode()
+    or
+    not exists(decl.getSpecifier(i+1)) and
+    successor(decl, result)
+  }
+}

javascript/ql/src/semmle/javascript/ES2015Modules.qll

@@ -27,7 +27,7 @@ class ES2015Module extends Module {
 
   /** Gets an export declaration in this module. */
   ExportDeclaration getAnExport() {
-    result.getContainer() = this
+    result.getTopLevel() = this
   }
 
   override Module getAnImportedModule() {
@@ -55,7 +55,7 @@ class ES2015Module extends Module {
 /** An import declaration. */
 class ImportDeclaration extends Stmt, Import, @importdeclaration {
   override ES2015Module getEnclosingModule() {
-    result = getContainer()
+    result = getTopLevel()
   }
 
   override PathExprInModule getImportedPath() {
@@ -254,7 +254,7 @@ class BulkReExportDeclaration extends ReExportDeclaration, @exportalldeclaration
   * but we ignore this subtlety.
   */
 private predicate isShadowedFromBulkExport(BulkReExportDeclaration reExport, string name) {
-  exists (ExportNamedDeclaration other | other.getContainer() = reExport.getEnclosingModule() |
+  exists (ExportNamedDeclaration other | other.getTopLevel() = reExport.getEnclosingModule() |
     other.getAnExportedDecl().getName() = name
     or
     other.getASpecifier().getExportedName() = name)

javascript/ql/src/semmle/javascript/dataflow/internal/InterModuleTypeInference.qll

@@ -301,7 +301,7 @@ private class AnalyzedExportAssign extends AnalyzedPropertyWrite, DataFlow::Valu
   }
 
   override predicate writes(AbstractValue baseVal, string propName, DataFlow::AnalyzedNode source) {
-    baseVal = TAbstractModuleObject(exportAssign.getContainer()) and
+    baseVal = TAbstractModuleObject(exportAssign.getTopLevel()) and
     propName = "exports" and
     source = this
   }

javascript/ql/test/library-tests/Flow/AbstractValues.expected

@@ -180,6 +180,10 @@
 | n.js:3:16:3:23 | object literal |
 | namespace-reexport.js:1:1:4:0 | exports object of module namespace-reexport |
 | namespace-reexport.js:1:1:4:0 | module object of module namespace-reexport |
+| nestedImport.js:1:1:13:0 | exports object of module nestedImport |
+| nestedImport.js:1:1:13:0 | module object of module nestedImport |
+| nestedImport.js:9:1:12:1 | function tst |
+| nestedImport.js:9:1:12:1 | instance of function tst |
 | nodeJsClient.js:1:1:6:0 | exports object of module nodeJsClient |
 | nodeJsClient.js:1:1:6:0 | module object of module nodeJsClient |
 | nodeJsLib.js:1:1:4:0 | exports object of module nodeJsLib |

javascript/ql/test/library-tests/Flow/abseval.expected

@@ -54,6 +54,7 @@
 | b.js:42:5:42:7 | z11 | b.js:42:11:42:18 | toString | file://:0:0:0:0 | indefinite value (import) |
 | b.js:45:5:45:7 | z12 | b.js:45:11:45:12 | f2 | ts2.ts:1:1:6:0 | exports object of module ts2 |
 | b.js:45:5:45:7 | z12 | b.js:45:11:45:12 | f2 | ts2.ts:1:10:1:22 | anonymous function |
+| b.js:45:5:45:7 | z12 | b.js:45:11:45:12 | f2 | ts2.ts:4:12:4:13 | object literal |
 | b.js:48:5:48:7 | z13 | b.js:48:11:48:11 | w | file://:0:0:0:0 | non-empty, non-numeric string |
 | b.js:51:5:51:7 | z14 | b.js:51:11:51:24 | foo_reexported | file://:0:0:0:0 | indefinite value (import) |
 | b.js:54:5:54:7 | z15 | b.js:54:11:54:19 | something | file://:0:0:0:0 | indefinite value (import) |
@@ -171,6 +172,10 @@
 | mixed.js:9:5:9:6 | fn | mixed.js:9:10:9:19 | __filename | file://:0:0:0:0 | numeric string |
 | mixed.js:10:5:10:6 | dn | mixed.js:10:10:10:18 | __dirname | file://:0:0:0:0 | non-empty, non-numeric string |
 | mixed.js:10:5:10:6 | dn | mixed.js:10:10:10:18 | __dirname | file://:0:0:0:0 | numeric string |
+| nestedImport.js:2:5:2:6 | x1 | nestedImport.js:2:10:2:12 | foo | esLib.js:3:8:3:24 | function foo |
+| nestedImport.js:6:7:6:8 | x2 | nestedImport.js:6:12:6:14 | foo | file://:0:0:0:0 | indefinite value (import) |
+| nestedImport.js:6:7:6:8 | x2 | nestedImport.js:6:12:6:14 | foo | nodeJsLib.js:3:15:3:37 | function nodeJsFoo |
+| nestedImport.js:11:7:11:8 | x3 | nestedImport.js:11:12:11:14 | foo | esLib.js:3:8:3:24 | function foo |
 | nodeJsClient.js:1:5:1:6 | nj | nodeJsClient.js:1:10:1:31 | require ... JsLib') | file://:0:0:0:0 | indefinite value (call) |
 | nodeJsClient.js:1:5:1:6 | nj | nodeJsClient.js:1:10:1:31 | require ... JsLib') | nodeJsLib.js:1:1:4:0 | exports object of module nodeJsLib |
 | nodeJsClient.js:1:5:1:6 | nj | nodeJsClient.js:1:10:1:31 | require ... JsLib') | nodeJsLib.js:1:18:1:43 | function nodeJsModule |

javascript/ql/test/library-tests/Flow/nestedImport.js

@@ -0,0 +1,12 @@
+import { foo } from './esLib';
+let x1 = foo;
+
+if (!foo) {
+  import { foo } from './nodeJsLib';
+  let x2 = foo;
+}
+
+function tst() {
+  import { foo } from './esLib';
+  let x3 = foo;
+}

javascript/ql/test/library-tests/Flow/types.expected

@@ -98,6 +98,9 @@
 | mixed.js:8:5:8:7 | exp | mixed.js:8:11:8:17 | exports | object |
 | mixed.js:9:5:9:6 | fn | mixed.js:9:10:9:19 | __filename | string |
 | mixed.js:10:5:10:6 | dn | mixed.js:10:10:10:18 | __dirname | string |
+| nestedImport.js:2:5:2:6 | x1 | nestedImport.js:2:10:2:12 | foo | function |
+| nestedImport.js:6:7:6:8 | x2 | nestedImport.js:6:12:6:14 | foo | boolean, class, date, function, null, number, object, regular expression,string or undefined |
+| nestedImport.js:11:7:11:8 | x3 | nestedImport.js:11:12:11:14 | foo | function |
 | nodeJsClient.js:1:5:1:6 | nj | nodeJsClient.js:1:10:1:31 | require ... JsLib') | boolean, class, date, function, null, number, object, regular expression,string or undefined |
 | nodeJsClient.js:2:5:2:6 | es | nodeJsClient.js:2:10:2:27 | require('./esLib') | boolean, class, date, function, null, number, object, regular expression,string or undefined |
 | nodeJsClient.js:4:5:4:6 | x1 | nodeJsClient.js:4:10:4:15 | nj.foo | boolean, class, date, function, null, number, object, regular expression,string or undefined |