C#: Use separate newtype branch for AssignableDefinitionNode

csharp/ql/consistency-queries/DataFlowConsistency.ql

@@ -1,5 +1,6 @@
 import csharp
 import cil
+private import semmle.code.csharp.controlflow.internal.ControlFlowGraphImpl as ControlFlowGraphImpl
 private import semmle.code.csharp.dataflow.internal.DataFlowImplSpecific
 private import semmle.code.csharp.dataflow.internal.TaintTrackingImplSpecific
 private import codeql.dataflow.internal.DataFlowImplConsistency
@@ -7,13 +8,18 @@ private import codeql.dataflow.internal.DataFlowImplConsistency
 private module Input implements InputSig<CsharpDataFlow> {
   private import CsharpDataFlow
 
+  private predicate isStaticAssignable(Assignable a) { a.(Modifiable).isStatic() }
+
+  predicate uniqueEnclosingCallableExclude(Node node) {
+    // TODO: Remove once static initializers are folded into the
+    // static constructors
+    isStaticAssignable(ControlFlowGraphImpl::getNodeCfgScope(node.getControlFlowNode()))
+  }
+
   predicate uniqueCallEnclosingCallableExclude(DataFlowCall call) {
     // TODO: Remove once static initializers are folded into the
     // static constructors
-    exists(ControlFlow::Node cfn |
+    isStaticAssignable(ControlFlowGraphImpl::getNodeCfgScope(call.getControlFlowNode()))
-      cfn.getAstNode() = any(FieldOrProperty f | f.isStatic()).getAChild+() and
-      cfn = call.getControlFlowNode()
-    )
   }
 
   predicate uniqueNodeLocationExclude(Node n) {

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

@@ -24,4 +24,8 @@ module CsharpDataFlow implements InputSig {
   predicate mayBenefitFromCallContext = Private::mayBenefitFromCallContext/1;
 
   predicate viableImplInCallContext = Private::viableImplInCallContext/2;
+
+  predicate neverSkipInPathGraph(Node n) {
+    exists(n.(AssignableDefinitionNode).getDefinition().getTargetAccess())
+  }
 }

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

@@ -208,16 +208,10 @@ predicate hasNodePath(ControlFlowReachabilityConfiguration conf, ExprNode n1, No
     cfn2 = n2.(ExprNode).getControlFlowNode()
   )
   or
-  exists(
+  exists(ControlFlow::Node cfn, AssignableDefinition def, ControlFlow::Node cfnDef |
-    ControlFlow::Node cfn, AssignableDefinition def, ControlFlow::Node cfnDef,
+    conf.hasDefPath(_, cfn, def, cfnDef) and
-    Ssa::ExplicitDefinition ssaDef
-  |
-    conf.hasDefPath(_, cfn, def, cfnDef)
-  |
     cfn = n1.getControlFlowNode() and
-    ssaDef.getADefinition() = def and
+    n2 = TAssignableDefinitionNode(def, cfnDef)
-    ssaDef.getControlFlowNode() = cfnDef and
-    n2.(SsaDefinitionExtNode).getDefinitionExt() = ssaDef
   )
 }
 
@@ -544,6 +538,13 @@ module LocalFlow {
     ThisFlow::adjacentThisRefs(nodeFrom.(PostUpdateNode).getPreUpdateNode(), nodeTo)
     or
     CilFlow::localFlowStepCil(nodeFrom, nodeTo)
+    or
+    exists(AssignableDefinition def, ControlFlow::Node cfn, Ssa::ExplicitDefinition ssaDef |
+      ssaDef.getADefinition() = def and
+      ssaDef.getControlFlowNode() = cfn and
+      nodeFrom = TAssignableDefinitionNode(def, cfn) and
+      nodeTo.(SsaDefinitionExtNode).getDefinitionExt() = ssaDef
+    )
   }
 
   /**
@@ -599,7 +600,7 @@ module LocalFlow {
     or
     hasNodePath(any(LocalExprStepConfiguration x), node1, node2) and
     (
-      node2 instanceof SsaDefinitionExtNode or
+      node2 instanceof AssignableDefinitionNode or
       node2.asExpr() instanceof Cast or
       node2.asExpr() instanceof AssignExpr
     )
@@ -906,6 +907,11 @@ private module Cached {
       not def.(Ssa::ExplicitDefinition).getADefinition() instanceof
         AssignableDefinitions::ImplicitParameterDefinition
     } or
+    TAssignableDefinitionNode(AssignableDefinition def, ControlFlow::Node cfn) {
+      cfn = def.getAControlFlowNode() and
+      // Handled by `TExplicitParameterNode` below
+      not def instanceof AssignableDefinitions::ImplicitParameterDefinition
+    } or
     TExplicitParameterNode(DotNet::Parameter p) {
       p = any(DataFlowCallable dfc).asCallable().getAParameter()
     } or
@@ -1044,15 +1050,7 @@ import Cached
 
 /** Holds if `n` should be hidden from path explanations. */
 predicate nodeIsHidden(Node n) {
-  exists(SsaImpl::DefinitionExt def | def = n.(SsaDefinitionExtNode).getDefinitionExt() |
+  n instanceof SsaDefinitionExtNode
-    def instanceof Ssa::PhiNode
-    or
-    def instanceof SsaImpl::PhiReadNode
-    or
-    def instanceof Ssa::ImplicitEntryDefinition
-    or
-    def instanceof Ssa::ImplicitCallDefinition
-  )
   or
   exists(Parameter p | p = n.(ParameterNode).getParameter() | not p.fromSource())
   or
@@ -1080,6 +1078,8 @@ predicate nodeIsHidden(Node n) {
   n instanceof InstanceParameterAccessNode
   or
   n instanceof PrimaryConstructorThisAccessNode
+  or
+  n = any(AssignableDefinitionNode def | not exists(def.getDefinition().getTargetAccess()))
 }
 
 /** A CIL SSA definition, viewed as a node in a data flow graph. */
@@ -1128,6 +1128,43 @@ class SsaDefinitionExtNode extends NodeImpl, TSsaDefinitionExtNode {
   override string toStringImpl() { result = def.toString() }
 }
 
+/** A definition, viewed as a node in a data flow graph. */
+class AssignableDefinitionNodeImpl extends NodeImpl, TAssignableDefinitionNode {
+  private AssignableDefinition def;
+  private ControlFlow::Node cfn_;
+
+  AssignableDefinitionNodeImpl() { this = TAssignableDefinitionNode(def, cfn_) }
+
+  /** Gets the underlying definition. */
+  AssignableDefinition getDefinition() { result = def }
+
+  /** Gets the underlying definition, at control flow node `cfn`, if any. */
+  AssignableDefinition getDefinitionAtNode(ControlFlow::Node cfn) {
+    result = def and
+    cfn = cfn_
+  }
+
+  override DataFlowCallable getEnclosingCallableImpl() {
+    result.asCallable() = cfn_.getEnclosingCallable()
+  }
+
+  override Type getTypeImpl() { result = def.getTarget().getType() }
+
+  override ControlFlow::Node getControlFlowNodeImpl() { result = cfn_ }
+
+  override Location getLocationImpl() {
+    result = def.getTargetAccess().getLocation()
+    or
+    not exists(def.getTargetAccess()) and result = def.getLocation()
+  }
+
+  override string toStringImpl() {
+    result = def.getTargetAccess().toString()
+    or
+    not exists(def.getTargetAccess()) and result = def.toString()
+  }
+}
+
 abstract class ParameterNodeImpl extends NodeImpl {
   abstract predicate isParameterOf(DataFlowCallable c, ParameterPosition pos);
 }
@@ -2021,12 +2058,11 @@ private PropertyContent getResultContent() {
 }
 
 private predicate primaryConstructorParameterStore(
-  SsaDefinitionExtNode node1, PrimaryConstructorParameterContent c, Node node2
+  AssignableDefinitionNode node1, PrimaryConstructorParameterContent c, Node node2
 ) {
-  exists(Ssa::ExplicitDefinition def, ControlFlow::Node cfn, Parameter p |
+  exists(AssignableDefinition def, ControlFlow::Node cfn, Parameter p |
-    def = node1.getDefinitionExt() and
+    node1 = TAssignableDefinitionNode(def, cfn) and
-    p = def.getSourceVariable().getAssignable() and
+    p = def.getTarget() and
-    cfn = def.getControlFlowNode() and
     node2 = TInstanceParameterAccessNode(cfn, true) and
     c.getParameter() = p
   )
@@ -2193,18 +2229,16 @@ predicate readStep(Node node1, ContentSet c, Node node2) {
       hasNodePath(x, node1, node2)
       or
       // item = variable in node1 = (..., variable, ...)
-      exists(AssignableDefinitions::TupleAssignmentDefinition tad, Ssa::ExplicitDefinition def |
+      exists(AssignableDefinitions::TupleAssignmentDefinition tad |
-        node2.(SsaDefinitionExtNode).getDefinitionExt() = def and
+        node2.(AssignableDefinitionNode).getDefinition() = tad and
-        def.getADefinition() = tad and
         tad.getLeaf() = item and
         hasNodePath(x, node1, node2)
       )
       or
       // item = variable in node1 = (..., variable, ...) in a case/is var (..., ...)
       te = any(PatternExpr pe).getAChildExpr*() and
-      exists(AssignableDefinitions::LocalVariableDefinition lvd, Ssa::ExplicitDefinition def |
+      exists(AssignableDefinitions::LocalVariableDefinition lvd |
-        node2.(SsaDefinitionExtNode).getDefinitionExt() = def and
+        node2.(AssignableDefinitionNode).getDefinition() = lvd and
-        def.getADefinition() = lvd and
         lvd.getDeclaration() = item and
         hasNodePath(x, node1, node2)
       )

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

@@ -109,18 +109,13 @@ class ParameterNode extends Node instanceof ParameterNodeImpl {
 }
 
 /** A definition, viewed as a node in a data flow graph. */
-class AssignableDefinitionNode extends Node, TSsaDefinitionExtNode {
+class AssignableDefinitionNode extends Node instanceof AssignableDefinitionNodeImpl {
-  private Ssa::ExplicitDefinition edef;
-
-  AssignableDefinitionNode() { this = TSsaDefinitionExtNode(edef) }
-
   /** Gets the underlying definition. */
-  AssignableDefinition getDefinition() { result = this.getDefinitionAtNode(_) }
+  AssignableDefinition getDefinition() { result = super.getDefinition() }
 
   /** Gets the underlying definition, at control flow node `cfn`, if any. */
   AssignableDefinition getDefinitionAtNode(ControlFlow::Node cfn) {
-    result = edef.getADefinition() and
+    result = super.getDefinitionAtNode(cfn)
-    cfn = edef.getControlFlowNode()
   }
 }