C#: Include "phi reads" in DataFlow::Node

Author: hvitved

csharp/ql/lib/semmle/code/csharp/dataflow/SSA.qll

@@ -138,25 +138,6 @@ module Ssa {
     }
   }
 
-  private string getSplitString(Definition def) {
-    exists(ControlFlow::BasicBlock bb, int i, ControlFlow::Node cfn |
-      def.definesAt(_, bb, i) and
-      result = cfn.(ControlFlow::Nodes::ElementNode).getSplitsString()
-    |
-      cfn = bb.getNode(i)
-      or
-      not exists(bb.getNode(i)) and
-      cfn = bb.getFirstNode()
-    )
-  }
-
-  private string getToStringPrefix(Definition def) {
-    result = "[" + getSplitString(def) + "] "
-    or
-    not exists(getSplitString(def)) and
-    result = ""
-  }
-
   /**
    * A static single assignment (SSA) definition. Either an explicit variable
    * definition (`ExplicitDefinition`), an implicit variable definition
@@ -521,8 +502,8 @@ module Ssa {
 
     override string toString() {
       if this.getADefinition() instanceof AssignableDefinitions::ImplicitParameterDefinition
-      then result = getToStringPrefix(this) + "SSA param(" + this.getSourceVariable() + ")"
-      else result = getToStringPrefix(this) + "SSA def(" + this.getSourceVariable() + ")"
+      then result = SsaImpl::getToStringPrefix(this) + "SSA param(" + this.getSourceVariable() + ")"
+      else result = SsaImpl::getToStringPrefix(this) + "SSA def(" + this.getSourceVariable() + ")"
     }
 
     override Location getLocation() { result = ad.getLocation() }
@@ -570,8 +551,12 @@ module Ssa {
 
     override string toString() {
       if this.getSourceVariable().getAssignable() instanceof LocalScopeVariable
-      then result = getToStringPrefix(this) + "SSA capture def(" + this.getSourceVariable() + ")"
-      else result = getToStringPrefix(this) + "SSA entry def(" + this.getSourceVariable() + ")"
+      then
+        result =
+          SsaImpl::getToStringPrefix(this) + "SSA capture def(" + this.getSourceVariable() + ")"
+      else
+        result =
+          SsaImpl::getToStringPrefix(this) + "SSA entry def(" + this.getSourceVariable() + ")"
     }
 
     override Location getLocation() { result = this.getCallable().getLocation() }
@@ -612,7 +597,7 @@ module Ssa {
     }
 
     override string toString() {
-      result = getToStringPrefix(this) + "SSA call def(" + this.getSourceVariable() + ")"
+      result = SsaImpl::getToStringPrefix(this) + "SSA call def(" + this.getSourceVariable() + ")"
     }
 
     override Location getLocation() { result = this.getCall().getLocation() }
@@ -640,7 +625,8 @@ module Ssa {
     final Definition getQualifierDefinition() { result = q }
 
     override string toString() {
-      result = getToStringPrefix(this) + "SSA qualifier def(" + this.getSourceVariable() + ")"
+      result =
+        SsaImpl::getToStringPrefix(this) + "SSA qualifier def(" + this.getSourceVariable() + ")"
     }
 
     override Location getLocation() { result = this.getQualifierDefinition().getLocation() }
@@ -682,7 +668,7 @@ module Ssa {
     }
 
     override string toString() {
-      result = getToStringPrefix(this) + "SSA phi(" + this.getSourceVariable() + ")"
+      result = SsaImpl::getToStringPrefix(this) + "SSA phi(" + this.getSourceVariable() + ")"
     }
 
     /*

csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll

@@ -174,7 +174,7 @@ predicate hasNodePath(ControlFlowReachabilityConfiguration conf, ExprNode n1, No
     cfn = n1.getControlFlowNode() and
     ssaDef.getADefinition() = def and
     ssaDef.getControlFlowNode() = cfnDef and
-    n2.(SsaDefinitionNode).getDefinition() = ssaDef
+    n2.(SsaDefinitionExtNode).getDefinitionExt() = ssaDef
   )
 }
 
@@ -310,17 +310,20 @@ module LocalFlow {
    * Holds if `nodeFrom` is a last node referencing SSA definition `def`, which
    * can reach `next`.
    */
-  private predicate localFlowSsaInput(Node nodeFrom, Ssa::Definition def, Ssa::Definition next) {
-    exists(ControlFlow::BasicBlock bb, int i | SsaImpl::lastRefBeforeRedef(def, bb, i, next) |
-      def.definesAt(_, bb, i) and
-      def = getSsaDefinition(nodeFrom)
+  private predicate localFlowSsaInput(
+    Node nodeFrom, SsaImpl::DefinitionExt def, SsaImpl::DefinitionExt next
+  ) {
+    exists(ControlFlow::BasicBlock bb, int i | SsaImpl::lastRefBeforeRedefExt(def, bb, i, next) |
+      def.definesAt(_, bb, i, _) and
+      def = getSsaDefinitionExt(nodeFrom)
       or
-      nodeFrom.asExprAtNode(bb.getNode(i)) instanceof AssignableRead
+      [nodeFrom, nodeFrom.(PostUpdateNode).getPreUpdateNode()].asExprAtNode(bb.getNode(i))
+        instanceof AssignableRead
     )
   }
 
-  private Ssa::Definition getSsaDefinition(Node n) {
-    result = n.(SsaDefinitionNode).getDefinition()
+  private SsaImpl::DefinitionExt getSsaDefinitionExt(Node n) {
+    result = n.(SsaDefinitionExtNode).getDefinitionExt()
     or
     result = n.(ExplicitParameterNode).getSsaDefinition()
   }
@@ -329,9 +332,9 @@ module LocalFlow {
    * Holds if there is a local use-use flow step from `nodeFrom` to `nodeTo`
    * involving SSA definition `def`.
    */
-  predicate localSsaFlowStepUseUse(Ssa::Definition def, Node nodeFrom, Node nodeTo) {
+  predicate localSsaFlowStepUseUse(SsaImpl::DefinitionExt def, Node nodeFrom, Node nodeTo) {
     exists(ControlFlow::Node cfnFrom, ControlFlow::Node cfnTo |
-      SsaImpl::adjacentReadPairSameVar(def, cfnFrom, cfnTo) and
+      SsaImpl::adjacentReadPairSameVarExt(def, cfnFrom, cfnTo) and
       nodeTo = TExprNode(cfnTo) and
       nodeFrom = TExprNode(cfnFrom)
     )
@@ -341,35 +344,36 @@ module LocalFlow {
    * Holds if there is a local flow step from `nodeFrom` to `nodeTo` involving
    * SSA definition `def`.
    */
-  predicate localSsaFlowStep(Ssa::Definition def, Node nodeFrom, Node nodeTo) {
+  predicate localSsaFlowStep(SsaImpl::DefinitionExt def, Node nodeFrom, Node nodeTo) {
     // Flow from SSA definition/parameter to first read
-    exists(ControlFlow::Node cfn |
-      def = getSsaDefinition(nodeFrom) and
-      nodeTo.asExprAtNode(cfn) = def.getAFirstReadAtNode(cfn)
-    )
+    def = getSsaDefinitionExt(nodeFrom) and
+    SsaImpl::firstReadSameVarExt(def, nodeTo.(ExprNode).getControlFlowNode())
     or
     // Flow from read to next read
     localSsaFlowStepUseUse(def, nodeFrom.(PostUpdateNode).getPreUpdateNode(), nodeTo)
     or
-    // Flow into phi node
-    exists(Ssa::PhiNode phi |
+    // Flow into phi (read) node
+    exists(SsaImpl::DefinitionExt phi |
       localFlowSsaInput(nodeFrom, def, phi) and
-      phi = nodeTo.(SsaDefinitionNode).getDefinition() and
-      def = phi.getAnInput()
+      phi = nodeTo.(SsaDefinitionExtNode).getDefinitionExt()
+    |
+      phi instanceof Ssa::PhiNode
+      or
+      phi instanceof SsaImpl::PhiReadNode
     )
     or
     // Flow into uncertain SSA definition
     exists(LocalFlow::UncertainExplicitSsaDefinition uncertain |
       localFlowSsaInput(nodeFrom, def, uncertain) and
-      uncertain = nodeTo.(SsaDefinitionNode).getDefinition() and
+      uncertain = nodeTo.(SsaDefinitionExtNode).getDefinitionExt() and
       def = uncertain.getPriorDefinition()
     )
   }
 
   /**
    * Holds if the source variable of SSA definition `def` is an instance field.
    */
-  predicate usesInstanceField(Ssa::Definition def) {
+  predicate usesInstanceField(SsaImpl::DefinitionExt def) {
     exists(Ssa::SourceVariables::FieldOrPropSourceVariable fp | fp = def.getSourceVariable() |
       not fp.getAssignable().(Modifiable).isStatic()
     )
@@ -378,7 +382,7 @@ module LocalFlow {
   predicate localFlowCapturedVarStep(Node nodeFrom, ImplicitCapturedArgumentNode nodeTo) {
     // Flow from SSA definition to implicit captured variable argument
     exists(Ssa::ExplicitDefinition def, ControlFlow::Nodes::ElementNode call |
-      def = getSsaDefinition(nodeFrom) and
+      def = getSsaDefinitionExt(nodeFrom) and
       def.isCapturedVariableDefinitionFlowIn(_, call, _) and
       nodeTo = TImplicitCapturedArgumentNode(call, def.getSourceVariable().getAssignable())
     )
@@ -455,7 +459,7 @@ module LocalFlow {
   }
 
   predicate localFlowStepCommon(Node nodeFrom, Node nodeTo) {
-    exists(Ssa::Definition def |
+    exists(SsaImpl::DefinitionExt def |
       localSsaFlowStep(def, nodeFrom, nodeTo) and
       not usesInstanceField(def)
     )
@@ -517,7 +521,7 @@ module LocalFlow {
 predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) {
   LocalFlow::localFlowStepCommon(nodeFrom, nodeTo)
   or
-  exists(Ssa::Definition def |
+  exists(SsaImpl::DefinitionExt def |
     LocalFlow::localSsaFlowStepUseUse(def, nodeFrom, nodeTo) and
     not FlowSummaryImpl::Private::Steps::prohibitsUseUseFlow(nodeFrom,
       any(DataFlowSummarizedCallable sc)) and
@@ -788,7 +792,7 @@ private module Cached {
     } or
     TCilExprNode(CIL::Expr e) { e.getImplementation() instanceof CIL::BestImplementation } or
     TCilSsaDefinitionNode(CilSsa::Definition def) or
-    TSsaDefinitionNode(Ssa::Definition def) {
+    TSsaDefinitionExtNode(SsaImpl::DefinitionExt def) {
       // Handled by `TExplicitParameterNode` below
       not def.(Ssa::ExplicitDefinition).getADefinition() instanceof
         AssignableDefinitions::ImplicitParameterDefinition
@@ -854,7 +858,7 @@ private module Cached {
     or
     LocalFlow::localSsaFlowStepUseUse(_, nodeFrom, nodeTo)
     or
-    exists(Ssa::Definition def |
+    exists(SsaImpl::DefinitionExt def |
       LocalFlow::localSsaFlowStep(def, nodeFrom, nodeTo) and
       LocalFlow::usesInstanceField(def)
     )
@@ -904,9 +908,11 @@ import Cached
 
 /** Holds if `n` should be hidden from path explanations. */
 predicate nodeIsHidden(Node n) {
-  exists(Ssa::Definition def | def = n.(SsaDefinitionNode).getDefinition() |
+  exists(SsaImpl::DefinitionExt def | def = n.(SsaDefinitionExtNode).getDefinitionExt() |
     def instanceof Ssa::PhiNode
     or
+    def instanceof SsaImpl::PhiReadNode
+    or
     def instanceof Ssa::ImplicitEntryDefinition
     or
     def instanceof Ssa::ImplicitCallDefinition
@@ -959,23 +965,31 @@ class CilSsaDefinitionNode extends NodeImpl, TCilSsaDefinitionNode {
 }
 
 /** An SSA definition, viewed as a node in a data flow graph. */
-class SsaDefinitionNode extends NodeImpl, TSsaDefinitionNode {
-  Ssa::Definition def;
+class SsaDefinitionExtNode extends NodeImpl, TSsaDefinitionExtNode {
+  SsaImpl::DefinitionExt def;
 
-  SsaDefinitionNode() { this = TSsaDefinitionNode(def) }
+  SsaDefinitionExtNode() { this = TSsaDefinitionExtNode(def) }
 
   /** Gets the underlying SSA definition. */
-  Ssa::Definition getDefinition() { result = def }
+  SsaImpl::DefinitionExt getDefinitionExt() { result = def }
 
   override DataFlowCallable getEnclosingCallableImpl() {
-    result.asCallable() = def.getEnclosingCallable()
+    result.asCallable() = def.(Ssa::Definition).getEnclosingCallable()
+    or
+    result.asCallable() = def.(SsaImpl::PhiReadNode).getSourceVariable().getEnclosingCallable()
   }
 
   override Type getTypeImpl() { result = def.getSourceVariable().getType() }
 
-  override ControlFlow::Node getControlFlowNodeImpl() { result = def.getControlFlowNode() }
+  override ControlFlow::Node getControlFlowNodeImpl() {
+    result = def.(Ssa::Definition).getControlFlowNode()
+  }
 
-  override Location getLocationImpl() { result = def.getLocation() }
+  override Location getLocationImpl() {
+    result = def.(Ssa::Definition).getLocation()
+    or
+    result = def.(SsaImpl::PhiReadNode).getBasicBlock().getLocation()
+  }
 
   override string toStringImpl() { result = def.toString() }
 }
@@ -1067,13 +1081,11 @@ private module ParameterNodes {
    * }                      }
    * ```
    */
-  class ImplicitCapturedParameterNode extends ParameterNodeImpl, SsaDefinitionNode {
-    override SsaCapturedEntryDefinition def;
-
-    ImplicitCapturedParameterNode() { def = this.getDefinition() }
+  class ImplicitCapturedParameterNode extends ParameterNodeImpl, SsaDefinitionExtNode {
+    ImplicitCapturedParameterNode() { def instanceof SsaCapturedEntryDefinition }
 
     /** Gets the captured variable that this implicit parameter models. */
-    LocalScopeVariable getVariable() { result = def.getVariable() }
+    LocalScopeVariable getVariable() { result = def.(SsaCapturedEntryDefinition).getVariable() }
 
     override predicate isParameterOf(DataFlowCallable c, ParameterPosition pos) {
       pos.isImplicitCapturedParameterPosition(def.getSourceVariable().getAssignable()) and
@@ -1308,12 +1320,12 @@ private module ReturnNodes {
    * A data-flow node that represents an assignment to an `out` or a `ref`
    * parameter.
    */
-  class OutRefReturnNode extends ReturnNode, SsaDefinitionNode {
+  class OutRefReturnNode extends ReturnNode, SsaDefinitionExtNode {
     OutRefReturnKind kind;
 
     OutRefReturnNode() {
       exists(Parameter p |
-        this.getDefinition().isLiveOutRefParameterDefinition(p) and
+        this.getDefinitionExt().(Ssa::Definition).isLiveOutRefParameterDefinition(p) and
         kind.getPosition() = p.getPosition()
       |
         p.isOut() and kind instanceof OutReturnKind
@@ -1397,20 +1409,20 @@ private module ReturnNodes {
    * }                       }
    * ```
    */
-  class ImplicitCapturedReturnNode extends ReturnNode, SsaDefinitionNode {
+  class ImplicitCapturedReturnNode extends ReturnNode, SsaDefinitionExtNode {
     private Ssa::ExplicitDefinition edef;
 
     ImplicitCapturedReturnNode() {
-      edef = this.getDefinition() and
+      edef = this.getDefinitionExt() and
       edef.isCapturedVariableDefinitionFlowOut(_, _)
     }
 
     /**
      * Holds if the value at this node may flow out to the implicit call definition
      * at `node`, using one or more calls.
      */
-    predicate flowsOutTo(SsaDefinitionNode node, boolean additionalCalls) {
-      edef.isCapturedVariableDefinitionFlowOut(node.getDefinition(), additionalCalls)
+    predicate flowsOutTo(SsaDefinitionExtNode node, boolean additionalCalls) {
+      edef.isCapturedVariableDefinitionFlowOut(node.getDefinitionExt(), additionalCalls)
     }
 
     override ImplicitCapturedReturnKind getKind() {
@@ -1517,13 +1529,13 @@ private module OutNodes {
    * A data-flow node that reads a value returned implicitly by a callable
    * using a captured variable.
    */
-  class CapturedOutNode extends OutNode, SsaDefinitionNode {
+  class CapturedOutNode extends OutNode, SsaDefinitionExtNode {
     private DataFlowCall call;
 
     CapturedOutNode() {
       exists(ImplicitCapturedReturnNode n, boolean additionalCalls, ControlFlow::Node cfn |
         n.flowsOutTo(this, additionalCalls) and
-        cfn = this.getDefinition().getControlFlowNode()
+        cfn = this.getDefinitionExt().(Ssa::Definition).getControlFlowNode()
       |
         additionalCalls = false and call = csharpCall(_, cfn)
         or
@@ -1535,7 +1547,7 @@ private module OutNodes {
     override DataFlowCall getCall(ReturnKind kind) {
       result = call and
       kind.(ImplicitCapturedReturnKind).getVariable() =
-        this.getDefinition().getSourceVariable().getAssignable()
+        this.getDefinitionExt().getSourceVariable().getAssignable()
     }
   }
 
@@ -1816,7 +1828,7 @@ predicate readStep(Node node1, Content c, Node node2) {
     exists(ForeachStmt fs, Ssa::ExplicitDefinition def |
       x.hasDefPath(fs.getIterableExpr(), node1.getControlFlowNode(), def.getADefinition(),
         def.getControlFlowNode()) and
-      node2.(SsaDefinitionNode).getDefinition() = def and
+      node2.(SsaDefinitionExtNode).getDefinitionExt() = def and
       c instanceof ElementContent
     )
     or
@@ -1839,7 +1851,7 @@ predicate readStep(Node node1, Content c, Node node2) {
       or
       // item = variable in node1 = (..., variable, ...)
       exists(AssignableDefinitions::TupleAssignmentDefinition tad, Ssa::ExplicitDefinition def |
-        node2.(SsaDefinitionNode).getDefinition() = def and
+        node2.(SsaDefinitionExtNode).getDefinitionExt() = def and
         def.getADefinition() = tad and
         tad.getLeaf() = item and
         hasNodePath(x, node1, node2)
@@ -1848,7 +1860,7 @@ predicate readStep(Node node1, Content c, Node node2) {
       // item = variable in node1 = (..., variable, ...) in a case/is var (..., ...)
       te = any(PatternExpr pe).getAChildExpr*() and
       exists(AssignableDefinitions::LocalVariableDefinition lvd, Ssa::ExplicitDefinition def |
-        node2.(SsaDefinitionNode).getDefinition() = def and
+        node2.(SsaDefinitionExtNode).getDefinitionExt() = def and
         def.getADefinition() = lvd and
         lvd.getDeclaration() = item and
         hasNodePath(x, node1, node2)
@@ -2182,7 +2194,7 @@ predicate lambdaCall(DataFlowCall call, LambdaCallKind kind, Node receiver) {
 
 /** Extra data-flow steps needed for lambda flow analysis. */
 predicate additionalLambdaFlowStep(Node nodeFrom, Node nodeTo, boolean preservesValue) {
-  exists(Ssa::Definition def |
+  exists(SsaImpl::DefinitionExt def |
     LocalFlow::localSsaFlowStep(def, nodeFrom, nodeTo) and
     LocalFlow::usesInstanceField(def) and
     preservesValue = true