Merge pull request github#3508 from asger-semmle/js/shared-data-flow-node

Approved by esbena

Author: semmle-qlci

change-notes/1.25/analysis-javascript.md

@@ -70,3 +70,7 @@ The following low-precision queries are no longer run by default on LGTM (their
 
 * A library `semmle.javascript.explore.CallGraph` has been added to help write queries for exploring the call graph.
 * Added data flow for `Map` and `Set`, and added matching type-tracking steps that can accessed using the `CollectionsTypeTracking` module.
+* The data-flow node representing a parameter or destructuring pattern is now always the `ValueNode` corresponding to that AST node. This has a few consequences:
+  - `Parameter.flow()` now gets the correct data flow node for a parameter. Previously this had a result, but the node was disconnected from the data flow graph.
+  - `ParameterNode.asExpr()` and `.getAstNode()` now gets the parameter's AST node, whereas previously it had no result.
+  - `Expr.flow()` now has a more meaningful result for destructuring patterns. Previously this node was disconnected from the data flow graph. Now it represents the values being destructured by the pattern.

javascript/ql/src/Statements/UselessConditional.ql

@@ -62,6 +62,14 @@ predicate isInitialParameterUse(Expr e) {
     not p.isRestParameter()
   )
   or
+  // same as above, but for captured variables
+  exists(SimpleParameter p, LocalVariable var |
+    var = p.getVariable() and
+    var.isCaptured() and
+    e = var.getAnAccess() and
+    not p.isRestParameter()
+  )
+  or
   isInitialParameterUse(e.(LogNotExpr).getOperand())
 }
 

javascript/ql/src/semmle/javascript/AST.qll

@@ -447,9 +447,9 @@ class StmtContainer extends @stmt_container, ASTNode {
  */
 module AST {
   /**
-   * A program element that evaluates to a value at runtime. This includes expressions,
-   * but also function and class declaration statements, as well as TypeScript
-   * namespace and enum declarations.
+   * A program element that evaluates to a value or destructures a value at runtime.
+   * This includes expressions and destructuring patterns, but also function and
+   * class declaration statements, as well as TypeScript namespace and enum declarations.
    *
    * Examples:
    *

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

@@ -299,11 +299,19 @@ class ControlFlowNode extends @cfg_node, Locatable, NodeInStmtContainer {
    */
   predicate isStart() { this = any(StmtContainer sc).getStart() }
 
+  /**
+   * Holds if this is a final node of `container`, that is, a CFG node where execution
+   * of that toplevel or function terminates.
+   */
+  predicate isAFinalNodeOfContainer(StmtContainer container) {
+    getASuccessor().(SyntheticControlFlowNode).isAFinalNodeOfContainer(container)
+  }
+
   /**
    * Holds if this is a final node, that is, a CFG node where execution of a
    * toplevel or function terminates.
    */
-  predicate isAFinalNode() { getASuccessor().(SyntheticControlFlowNode).isAFinalNode() }
+  final predicate isAFinalNode() { isAFinalNodeOfContainer(_) }
 
   /**
    * Holds if this node is unreachable, that is, it has no predecessors in the CFG.
@@ -361,7 +369,9 @@ class ControlFlowEntryNode extends SyntheticControlFlowNode, @entry_node {
 
 /** A synthetic CFG node marking the exit of a function or toplevel script. */
 class ControlFlowExitNode extends SyntheticControlFlowNode, @exit_node {
-  override predicate isAFinalNode() { any() }
+  override predicate isAFinalNodeOfContainer(StmtContainer container) {
+    exit_cfg_node(this, container)
+  }
 
   override string toString() { result = "exit node of " + getContainer().toString() }
 }

javascript/ql/src/semmle/javascript/DefUse.qll

@@ -45,8 +45,6 @@ private predicate defn(ControlFlowNode def, Expr lhs, AST::ValueNode rhs) {
   exists(EnumMember member | def = member.getIdentifier() |
     lhs = def and rhs = member.getInitializer()
   )
-  or
-  lhs = def and def.(Parameter).getDefault() = rhs
 }
 
 /**

javascript/ql/src/semmle/javascript/dataflow/Configuration.qll

@@ -1601,6 +1601,9 @@ class MidPathNode extends PathNode, MkMidNode {
     nd.(DataFlow::SsaDefinitionNode).getSsaVariable().getDefinition() instanceof
       SsaImplicitDefinition
     or
+    // Skip SSA definition of parameter as its location coincides with the parameter node
+    nd = DataFlow::ssaDefinitionNode(SSA::definition(any(SimpleParameter p)))
+    or
     // Skip to the top of big left-leaning string concatenation trees.
     nd = any(AddExpr add).flow() and
     nd = any(AddExpr add).getAnOperand().flow()

javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll

@@ -21,32 +21,10 @@
 import javascript
 private import internal.CallGraphs
 private import internal.FlowSteps as FlowSteps
+private import internal.DataFlowNode
+private import internal.AnalyzedParameters
 
 module DataFlow {
-  cached
-  private newtype TNode =
-    TValueNode(AST::ValueNode nd) or
-    TSsaDefNode(SsaDefinition d) or
-    TCapturedVariableNode(LocalVariable v) { v.isCaptured() } or
-    TPropNode(@property p) or
-    TRestPatternNode(DestructuringPattern dp, Expr rest) { rest = dp.getRest() } or
-    TDestructuringPatternNode(DestructuringPattern dp) or
-    TElementPatternNode(ArrayPattern ap, Expr p) { p = ap.getElement(_) } or
-    TElementNode(ArrayExpr arr, Expr e) { e = arr.getAnElement() } or
-    TReflectiveCallNode(MethodCallExpr ce, string kind) {
-      ce.getMethodName() = kind and
-      (kind = "call" or kind = "apply")
-    } or
-    TThisNode(StmtContainer f) { f.(Function).getThisBinder() = f or f instanceof TopLevel } or
-    TUnusedParameterNode(SimpleParameter p) { not exists(SSA::definition(p)) } or
-    TDestructuredModuleImportNode(ImportDeclaration decl) {
-      exists(decl.getASpecifier().getImportedName())
-    } or
-    THtmlAttributeNode(HTML::Attribute attr) or
-    TExceptionalFunctionReturnNode(Function f) or
-    TExceptionalInvocationReturnNode(InvokeExpr e) or
-    TGlobalAccessPathRoot()
-
   /**
    * A node in the data flow graph.
    */
@@ -90,13 +68,11 @@ module DataFlow {
     /**
      * Gets the expression enclosing this data flow node.
      * In most cases the result is the same as `asExpr()`, however this method
-     * additionally the `InvokeExpr` corresponding to reflective calls, and the `Parameter`
-     * for a `DataFlow::ParameterNode`.
+     * additionally includes the `InvokeExpr` corresponding to reflective calls.
      */
     Expr getEnclosingExpr() {
       result = asExpr() or
-      this = DataFlow::reflectiveCallNode(result) or
-      result = this.(ParameterNode).getParameter()
+      this = DataFlow::reflectiveCallNode(result)
     }
 
     /** Gets the AST node corresponding to this data flow node, if any. */
@@ -251,7 +227,7 @@ module DataFlow {
      */
     private JSDocTypeExpr getFallbackTypeAnnotation() {
       exists(BindingPattern pattern |
-        this = lvalueNode(pattern) and
+        this = valueNode(pattern) and
         not ast_node_type(pattern, _) and
         result = pattern.getTypeAnnotation()
       )
@@ -281,8 +257,8 @@ module DataFlow {
   }
 
   /**
-   * An expression or a declaration of a function, class, namespace or enum,
-   * viewed as a node in the data flow graph.
+   * A node in the data flow graph which corresponds to an expression,
+   * destructuring pattern, or declaration of a function, class, namespace, or enum.
    *
    * Examples:
    * ```js
@@ -390,30 +366,6 @@ module DataFlow {
     override ASTNode getAstNode() { result = rest }
   }
 
-  /**
-   * A node in the data flow graph which corresponds to the value destructured by an
-   * object or array pattern.
-   */
-  private class DestructuringPatternNode extends Node, TDestructuringPatternNode {
-    DestructuringPattern pattern;
-
-    DestructuringPatternNode() { this = TDestructuringPatternNode(pattern) }
-
-    override BasicBlock getBasicBlock() { result = pattern.getBasicBlock() }
-
-    override predicate hasLocationInfo(
-      string filepath, int startline, int startcolumn, int endline, int endcolumn
-    ) {
-      pattern.getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
-    }
-
-    override string toString() { result = pattern.toString() }
-
-    override File getFile() { result = pattern.getFile() }
-
-    override ASTNode getAstNode() { result = pattern }
-  }
-
   /**
    * A node in the data flow graph which corresponds to an element pattern of an
    * array pattern.
@@ -759,10 +711,6 @@ module DataFlow {
         parameterNode(paramNode, param)
       |
         result = paramNode
-        or
-        // special case: there is no SSA flow step for unused parameters
-        paramNode instanceof UnusedParameterNode and
-        result = param.getDefault().flow()
       )
     }
 
@@ -850,7 +798,7 @@ module DataFlow {
     /** Gets the value pattern of this property pattern. */
     Expr getValuePattern() { result = prop.getValuePattern() }
 
-    override Node getBase() { result = TDestructuringPatternNode(prop.getObjectPattern()) }
+    override Node getBase() { result = TValueNode(prop.getObjectPattern()) }
 
     override Expr getPropertyNameExpr() { result = prop.getNameExpr() }
 
@@ -863,7 +811,7 @@ module DataFlow {
    * for `[ ...elts ] = arr`.
    */
   private class RestPatternAsPropRead extends PropRead, RestPatternNode {
-    override Node getBase() { result = TDestructuringPatternNode(pattern) }
+    override Node getBase() { result = TValueNode(pattern) }
 
     override Expr getPropertyNameExpr() { none() }
 
@@ -876,7 +824,7 @@ module DataFlow {
    * for `y`.
    */
   private class ElementPatternAsPropRead extends PropRead, ElementPatternNode {
-    override Node getBase() { result = TDestructuringPatternNode(pattern) }
+    override Node getBase() { result = TValueNode(pattern) }
 
     override Expr getPropertyNameExpr() { none() }
 
@@ -923,32 +871,6 @@ module DataFlow {
     override string getPropertyName() { none() }
   }
 
-  /**
-   * A data flow node representing an unused parameter.
-   *
-   * This case exists to ensure all parameters have a corresponding data-flow node.
-   * In most cases, parameters are represented by SSA definitions or destructuring pattern nodes.
-   */
-  private class UnusedParameterNode extends DataFlow::Node, TUnusedParameterNode {
-    SimpleParameter p;
-
-    UnusedParameterNode() { this = TUnusedParameterNode(p) }
-
-    override string toString() { result = p.toString() }
-
-    override ASTNode getAstNode() { result = p }
-
-    override BasicBlock getBasicBlock() { result = p.getBasicBlock() }
-
-    override predicate hasLocationInfo(
-      string filepath, int startline, int startcolumn, int endline, int endcolumn
-    ) {
-      p.getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
-    }
-
-    override File getFile() { result = p.getFile() }
-  }
-
   /**
    * A data flow node representing an HTML attribute.
    */
@@ -1302,7 +1224,7 @@ module DataFlow {
   /**
    * INTERNAL: Use `parameterNode(Parameter)` instead.
    */
-  predicate parameterNode(DataFlow::Node nd, Parameter p) { nd = lvalueNode(p) }
+  predicate parameterNode(DataFlow::Node nd, Parameter p) { nd = valueNode(p) }
 
   /**
    * INTERNAL: Use `thisNode(StmtContainer container)` instead.
@@ -1354,9 +1276,7 @@ module DataFlow {
       result = TSsaDefNode(ssa)
     )
     or
-    result = TDestructuringPatternNode(lvalue)
-    or
-    result = TUnusedParameterNode(lvalue)
+    result = TValueNode(lvalue.(DestructuringPattern))
   }
 
   /**
@@ -1411,6 +1331,17 @@ module DataFlow {
       succ = lvalueNode(def.getTarget())
     )
     or
+    exists(SimpleParameter param |
+      pred = valueNode(param) and // The value node represents the incoming argument
+      succ = lvalueNode(param) // The SSA node represents the parameters's local variable
+    )
+    or
+    exists(Expr arg, Parameter param |
+      localArgumentPassing(arg, param) and
+      pred = valueNode(arg) and
+      succ = valueNode(param)
+    )
+    or
     exists(PropertyPattern pattern |
       pred = TPropNode(pattern) and
       succ = lvalueNode(pattern.getValuePattern())
@@ -1546,8 +1477,7 @@ module DataFlow {
    */
   private AST::ValueNode defSourceNode(VarDef def) {
     result = def.getSource() or
-    result = def.getDestructuringSource() or
-    localArgumentPassing(result, def)
+    result = def.getDestructuringSource()
   }
 
   /**
@@ -1593,8 +1523,15 @@ module DataFlow {
       e instanceof FunctionBindExpr
       or
       e instanceof TaggedTemplateExpr
+      or
+      e instanceof Parameter and
+      not localArgumentPassing(_, e) and
+      not isAnalyzedParameter(e) and
+      not e.(Parameter).isRestParameter()
     )
     or
+    nd.(AnalyzedNode).hasAdditionalIncompleteness(cause)
+    or
     nd.asExpr() instanceof ExternalModuleReference and
     cause = "import"
     or
@@ -1622,18 +1559,12 @@ module DataFlow {
     exists(PropertyPattern p | nd = TPropNode(p)) and cause = "heap"
     or
     nd instanceof TElementPatternNode and cause = "heap"
-    or
-    nd instanceof UnusedParameterNode and cause = "call"
   }
 
   /**
    * Holds if definition `def` cannot be completely analyzed due to `cause`.
    */
   private predicate defIsIncomplete(VarDef def, Incompleteness cause) {
-    def instanceof Parameter and
-    not localArgumentPassing(_, def) and
-    cause = "call"
-    or
     def instanceof ImportSpecifier and
     cause = "import"
     or

javascript/ql/src/semmle/javascript/dataflow/TypeInference.qll

@@ -156,6 +156,14 @@ class AnalyzedNode extends DataFlow::Node {
 
   /** Holds if the flow analysis can infer at least one abstract value for this node. */
   predicate hasFlow() { exists(getAValue()) }
+
+  /**
+   * INTERNAL. Use `isIncomplete()` instead.
+   *
+   * Subclasses may override this to contribute additional incompleteness to this node
+   * without overriding `isIncomplete()`.
+   */
+  predicate hasAdditionalIncompleteness(DataFlow::Incompleteness cause) { none() }
 }
 
 /**
@@ -255,14 +263,14 @@ class AnalyzedFunction extends DataFlow::AnalyzedValueNode {
    * of functions that cannot actually complete normally, since it does not
    * account for `finally` blocks and does not check reachability.
    */
-  private predicate mayReturnImplicitly() {
-    exists(ConcreteControlFlowNode final |
-      final.getContainer() = astNode and
-      final.isAFinalNode() and
-      not final instanceof ReturnStmt and
-      not final instanceof ThrowStmt
-    )
-  }
+  private predicate mayReturnImplicitly() { terminalNode(astNode, any(ExprOrStmt st)) }
+}
+
+pragma[noinline]
+private predicate terminalNode(Function f, ControlFlowNode final) {
+  final.isAFinalNodeOfContainer(f) and
+  not final instanceof ReturnStmt and
+  not final instanceof ThrowStmt
 }
 
 /**