C#: Move implicit parameter definitions inside method bodies in SSA c…

…onstruction

csharp/ql/lib/semmle/code/csharp/Assignable.qll

@@ -393,14 +393,18 @@ private import AssignableInternal
  */
 class AssignableDefinition extends TAssignableDefinition {
   /**
+   * DEPRECATED: Use `this.getExpr().getAControlFlowNode()` instead.
+   *
    * Gets a control flow node that updates the targeted assignable when
    * reached.
    *
    * Multiple definitions may relate to the same control flow node. For example,
    * the definitions of `x` and `y` in `M(out x, out y)` and `(x, y) = (0, 1)`
    * relate to the same call to `M` and assignment node, respectively.
    */
-  ControlFlow::Node getAControlFlowNode() { result = this.getExpr().getAControlFlowNode() }
+  deprecated ControlFlow::Node getAControlFlowNode() {
+    result = this.getExpr().getAControlFlowNode()
+  }
 
   /**
    * Gets the underlying expression that updates the targeted assignable when
@@ -477,6 +481,11 @@ class AssignableDefinition extends TAssignableDefinition {
       exists(Ssa::ExplicitDefinition def | result = def.getAFirstReadAtNode(cfn) |
         this = def.getADefinition()
       )
+      or
+      exists(Ssa::ImplicitParameterDefinition def | result = def.getAFirstReadAtNode(cfn) |
+        this.(AssignableDefinitions::ImplicitParameterDefinition).getParameter() =
+          def.getParameter()
+      )
     )
   }
 
@@ -550,7 +559,7 @@ module AssignableDefinitions {
     ControlFlow::BasicBlock bb, Parameter p, AssignableDefinition def
   ) {
     def = any(RefArg arg).getAnAnalyzableRefDef(p) and
-    bb.getANode() = def.getAControlFlowNode()
+    bb.getANode() = def.getExpr().getAControlFlowNode()
   }
 
   /**
@@ -662,7 +671,9 @@ module AssignableDefinitions {
     /** Gets the underlying parameter. */
     Parameter getParameter() { result = p }
 
-    override ControlFlow::Node getAControlFlowNode() { result = p.getCallable().getEntryPoint() }
+    deprecated override ControlFlow::Node getAControlFlowNode() {
+      result = p.getCallable().getEntryPoint()
+    }
 
     override Parameter getElement() { result = p }
 

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

@@ -157,9 +157,9 @@ private ControlFlowElement getANullCheck(
   exists(Expr e, G::AbstractValue v | v.branch(result, s, e) | exprImpliesSsaDef(e, v, def, nv))
 }
 
-private predicate isMaybeNullArgument(Ssa::ExplicitDefinition def, MaybeNullExpr arg) {
+private predicate isMaybeNullArgument(Ssa::ImplicitParameterDefinition def, MaybeNullExpr arg) {
   exists(AssignableDefinitions::ImplicitParameterDefinition pdef, Parameter p |
-    pdef = def.getADefinition()
+    p = def.getParameter()
   |
     p = pdef.getParameter().getUnboundDeclaration() and
     arg = p.getAnAssignedArgument() and
@@ -208,9 +208,9 @@ private predicate hasMultipleParamsArguments(Call c) {
   )
 }
 
-private predicate isNullDefaultArgument(Ssa::ExplicitDefinition def, AlwaysNullExpr arg) {
+private predicate isNullDefaultArgument(Ssa::ImplicitParameterDefinition def, AlwaysNullExpr arg) {
   exists(AssignableDefinitions::ImplicitParameterDefinition pdef, Parameter p |
-    pdef = def.getADefinition()
+    p = def.getParameter()
   |
     p = pdef.getParameter().getUnboundDeclaration() and
     arg = p.getDefaultValue() and
@@ -543,9 +543,10 @@ class Dereference extends G::DereferenceableExpr {
         p.fromSource() // assume all non-source extension methods perform a dereference
         implies
         exists(
-          Ssa::ExplicitDefinition def, AssignableDefinitions::ImplicitParameterDefinition pdef
+          Ssa::ImplicitParameterDefinition def,
+          AssignableDefinitions::ImplicitParameterDefinition pdef
         |
-          pdef = def.getADefinition()
+          p = def.getParameter()
         |
           p.getUnboundDeclaration() = pdef.getParameter() and
           def.getARead() instanceof Dereference

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

@@ -72,13 +72,13 @@ module Ssa {
   module SourceVariables {
     /** A local scope variable. */
     class LocalScopeSourceVariable extends SourceVariable, SsaImpl::TLocalVar {
-      override LocalScopeVariable getAssignable() { this = SsaImpl::TLocalVar(_, result) }
+      override LocalScopeVariable getAssignable() { this = SsaImpl::TLocalVar(_, result, _) }
 
-      override Callable getEnclosingCallable() { this = SsaImpl::TLocalVar(result, _) }
+      override Callable getEnclosingCallable() { this = SsaImpl::TLocalVar(result, _, _) }
 
       override string toString() { result = this.getAssignable().getName() }
 
-      override Location getLocation() { result = this.getAssignable().getLocation() }
+      override Location getLocation() { this = SsaImpl::TLocalVar(_, _, result) }
     }
 
     /** A fully qualified field or property. */
@@ -507,9 +507,7 @@ module Ssa {
     override Element getElement() { result = ad.getElement() }
 
     override string toString() {
-      if this.getADefinition() instanceof AssignableDefinitions::ImplicitParameterDefinition
-      then result = SsaImpl::getToStringPrefix(this) + "SSA param(" + this.getSourceVariable() + ")"
-      else result = SsaImpl::getToStringPrefix(this) + "SSA def(" + this.getSourceVariable() + ")"
+      result = SsaImpl::getToStringPrefix(this) + "SSA def(" + this.getSourceVariable() + ")"
     }
 
     override Location getLocation() { result = ad.getLocation() }
@@ -537,7 +535,7 @@ module Ssa {
 
   /**
    * An SSA definition representing the implicit initialization of a variable
-   * at the beginning of a callable. Either the variable is a local scope variable
+   * at the beginning of a callable. Either a parameter, a local scope variable
    * captured by the callable, or a field or property accessed inside the callable.
    */
   class ImplicitEntryDefinition extends ImplicitDefinition {
@@ -551,7 +549,7 @@ module Ssa {
     /** Gets the callable that this entry definition belongs to. */
     final Callable getCallable() { result = this.getBasicBlock().getCallable() }
 
-    override Callable getElement() { result = this.getCallable() }
+    override Element getElement() { result = this.getCallable() }
 
     override string toString() {
       if this.getSourceVariable().getAssignable() instanceof LocalScopeVariable
@@ -566,6 +564,30 @@ module Ssa {
     override Location getLocation() { result = this.getCallable().getLocation() }
   }
 
+  /**
+   * An SSA definition representing the implicit initialization of a parameter
+   * at the beginning of a callable.
+   */
+  class ImplicitParameterDefinition extends ImplicitEntryDefinition {
+    private Parameter p;
+
+    ImplicitParameterDefinition() {
+      p = this.getSourceVariable().getAssignable() and
+      p.getCallable() = this.getCallable()
+    }
+
+    /** Gets the parameter that this entry definition represents. */
+    Parameter getParameter() { result = p }
+
+    override Element getElement() { result = this.getParameter() }
+
+    override string toString() {
+      result = SsaImpl::getToStringPrefix(this) + "SSA param(" + this.getParameter() + ")"
+    }
+
+    override Location getLocation() { result = this.getSourceVariable().getLocation() }
+  }
+
   /**
    * An SSA definition representing the potential definition of a variable
    * via a call.

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

@@ -14,7 +14,7 @@ module BaseSsa {
   private predicate definitionAt(
     AssignableDefinition def, ControlFlow::BasicBlock bb, int i, SsaInput::SourceVariable v
   ) {
-    bb.getNode(i) = def.getAControlFlowNode() and
+    bb.getNode(i) = def.getExpr().getAControlFlowNode() and
     v = def.getTarget() and
     // In cases like `(x, x) = (0, 1)`, we discard the first (dead) definition of `x`
     not exists(TupleAssignmentDefinition first, TupleAssignmentDefinition second | first = def |
@@ -27,8 +27,18 @@ module BaseSsa {
   private predicate implicitEntryDef(
     Callable c, ControlFlow::BasicBlocks::EntryBlock bb, SsaInput::SourceVariable v
   ) {
-    v.isReadonlyCapturedBy(c) and
-    c = bb.getCallable()
+    exists(ControlFlow::ControlFlow::BasicBlocks::EntryBlock entry |
+      c = entry.getCallable() and
+      // In case `c` has multiple bodies, we want each body to gets its own implicit
+      // entry definition. In case `c` doesn't have multiple bodies, the line below
+      // is simply the same as `bb = entry`.
+      bb = entry.getFirstNode().getASuccessor().getBasicBlock() and
+      c = v.getCallable()
+    |
+      v.isReadonlyCapturedBy(c)
+      or
+      v instanceof Parameter
+    )
   }
 
   private module SsaInput implements SsaImplCommon::InputSig<Location> {
@@ -83,6 +93,13 @@ module BaseSsa {
       )
     }
 
+    final predicate isImplicitEntryDefinition(SsaInput::SourceVariable v) {
+      exists(ControlFlow::BasicBlock bb |
+        this.definesAt(v, bb, -1) and
+        implicitEntryDef(_, bb, v)
+      )
+    }
+
     private Definition getAPhiInputOrPriorDefinition() {
       result = this.(PhiNode).getAnInput() or
       SsaImpl::uncertainWriteDefinitionInput(this, result)

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

@@ -199,7 +199,7 @@ abstract class ControlFlowReachabilityConfiguration extends string {
     exists(ControlFlow::BasicBlock bb, boolean isSuccessor, int i, int j |
       this.reachesBasicBlockDefinitionBase(e, def, isSuccessor, cfn, i, bb) and
       cfnDef = bb.getNode(j) and
-      def.getAControlFlowNode() = cfnDef
+      def.getExpr().getAControlFlowNode() = cfnDef
     |
       isSuccessor = true and j >= i
       or
@@ -208,7 +208,7 @@ abstract class ControlFlowReachabilityConfiguration extends string {
     or
     exists(ControlFlow::BasicBlock bb |
       this.reachesBasicBlockDefinitionRec(e, def, _, cfn, bb) and
-      def.getAControlFlowNode() = cfnDef and
+      def.getExpr().getAControlFlowNode() = cfnDef and
       cfnDef = bb.getANode()
     )
   }

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

@@ -44,7 +44,8 @@ private ControlFlow::Node getAPrimaryConstructorParameterCfn(ParameterAccess pa)
   (
     result = pa.(ParameterRead).getAControlFlowNode()
     or
-    pa = any(AssignableDefinition def | result = def.getAControlFlowNode()).getTargetAccess()
+    pa =
+      any(AssignableDefinition def | result = def.getExpr().getAControlFlowNode()).getTargetAccess()
   )
 }
 
@@ -352,9 +353,7 @@ module VariableCapture {
 
       VariableWrite() {
         def.getTarget() = v.asLocalScopeVariable() and
-        this = def.getAControlFlowNode() and
-        // the shared variable capture library inserts implicit parameter definitions
-        not def instanceof AssignableDefinitions::ImplicitParameterDefinition
+        this = def.getExpr().getAControlFlowNode()
       }
 
       ControlFlow::Node getRhs() { LocalFlow::defAssigns(def, this, result) }
@@ -1098,13 +1097,10 @@ private module Cached {
     TExprNode(ControlFlow::Nodes::ElementNode cfn) { cfn.getAstNode() instanceof Expr } or
     TSsaDefinitionExtNode(SsaImpl::DefinitionExt def) {
       // Handled by `TExplicitParameterNode` below
-      not def.(Ssa::ExplicitDefinition).getADefinition() instanceof
-        AssignableDefinitions::ImplicitParameterDefinition
+      not def instanceof Ssa::ImplicitParameterDefinition
     } or
     TAssignableDefinitionNode(AssignableDefinition def, ControlFlow::Node cfn) {
-      cfn = def.getAControlFlowNode() and
-      // Handled by `TExplicitParameterNode` below
-      not def instanceof AssignableDefinitions::ImplicitParameterDefinition
+      cfn = def.getExpr().getAControlFlowNode()
     } or
     TExplicitParameterNode(Parameter p) {
       p = any(DataFlowCallable dfc).asCallable().getAParameter()
@@ -1351,10 +1347,7 @@ private module ParameterNodes {
     ExplicitParameterNode() { this = TExplicitParameterNode(parameter) }
 
     /** Gets the SSA definition corresponding to this parameter, if any. */
-    Ssa::ExplicitDefinition getSsaDefinition() {
-      result.getADefinition().(AssignableDefinitions::ImplicitParameterDefinition).getParameter() =
-        parameter
-    }
+    Ssa::ImplicitParameterDefinition getSsaDefinition() { result.getParameter() = parameter }
 
     override predicate isParameterOf(DataFlowCallable c, ParameterPosition pos) {
       c.asCallable().getParameter(pos.getPosition()) = parameter

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

@@ -137,7 +137,7 @@ private module SourceVariableImpl {
     ControlFlow::BasicBlock bb, int i, Ssa::SourceVariable v, AssignableDefinition ad
   ) {
     ad = v.getADefinition() and
-    ad.getAControlFlowNode() = bb.getNode(i) and
+    ad.getExpr().getAControlFlowNode() = bb.getNode(i) and
     // In cases like `(x, x) = (0, 1)`, we discard the first (dead) definition of `x`
     not exists(TupleAssignmentDefinition first, TupleAssignmentDefinition second | first = ad |
       second.getAssignment() = first.getAssignment() and
@@ -231,7 +231,7 @@ private module SourceVariableImpl {
       def.getTarget() = lv and
       lv.isRef() and
       lv = v.getAssignable() and
-      bb.getNode(i) = def.getAControlFlowNode() and
+      bb.getNode(i) = def.getExpr().getAControlFlowNode() and
       not def.getAssignment() instanceof LocalVariableDeclAndInitExpr
     )
   }
@@ -830,11 +830,14 @@ cached
 private module Cached {
   cached
   newtype TSourceVariable =
-    TLocalVar(Callable c, PreSsa::SimpleLocalScopeVariable v) {
-      c = v.getCallable()
-      or
-      // Local scope variables can be captured
-      c = v.getAnAccess().getEnclosingCallable()
+    TLocalVar(Callable c, PreSsa::SimpleLocalScopeVariable v, Location l) {
+      (
+        c = v.getCallable()
+        or
+        // Local scope variables can be captured
+        c = v.getAnAccess().getEnclosingCallable()
+      ) and
+      l = v.getALocation()
     } or
     TPlainFieldOrProp(Callable c, FieldOrProp f) {
       exists(FieldOrPropRead fr | isPlainFieldOrPropAccess(fr, f, c)) and
@@ -849,7 +852,7 @@ private module Cached {
   cached
   AssignableAccess getAnAccess(Ssa::SourceVariable v) {
     exists(Callable c |
-      exists(LocalScopeVariable lsv | v = TLocalVar(c, lsv) |
+      exists(LocalScopeVariable lsv | v = TLocalVar(c, lsv, _) |
         result = lsv.getAnAccess() and
         result.getEnclosingCallable() = c
       )
@@ -864,12 +867,18 @@ private module Cached {
     )
   }
 
+  bindingset[e1, e2]
+  pragma[inline_late]
+  private predicate inSameFile(Location e1, Location e2) { e1.getFile() = e2.getFile() }
+
   cached
-  predicate implicitEntryDefinition(
-    ControlFlow::ControlFlow::BasicBlocks::EntryBlock bb, Ssa::SourceVariable v
-  ) {
-    exists(Callable c |
-      c = bb.getCallable() and
+  predicate implicitEntryDefinition(ControlFlow::ControlFlow::BasicBlock bb, Ssa::SourceVariable v) {
+    exists(ControlFlow::ControlFlow::BasicBlocks::EntryBlock entry, Callable c |
+      c = entry.getCallable() and
+      // In case `c` has multiple bodies, we want each body to gets its own implicit
+      // entry definition. In case `c` doesn't have multiple bodies, the line below
+      // is simply the same as `bb = entry`.
+      bb = entry.getFirstNode().getASuccessor().getBasicBlock() and
       c = v.getEnclosingCallable()
     |
       // Captured variable
@@ -881,6 +890,14 @@ private module Cached {
       or
       // Each tracked field and property has an implicit entry definition
       v instanceof PlainFieldOrPropSourceVariable
+      or
+      // Each parameter has an implicit entry definition. In case of multiple bodies,
+      // we need to match the location of the parameter with the location of the
+      // assigning basic block; requiring them to be in the same file should be
+      // sufficient (multiple method bodies in the same file would be a compilation
+      // error).
+      v.getAssignable() instanceof Parameter and
+      inSameFile(v.getLocation(), bb.getLocation())
     )
   }
 

csharp/ql/lib/semmle/code/csharp/dataflow/internal/rangeanalysis/ControlFlowReachability.qll

@@ -199,7 +199,7 @@ abstract class ControlFlowReachabilityConfiguration extends string {
     exists(ControlFlow::BasicBlock bb, boolean isSuccessor, int i, int j |
       this.reachesBasicBlockDefinitionBase(e, def, isSuccessor, cfn, i, bb) and
       cfnDef = bb.getNode(j) and
-      def.getAControlFlowNode() = cfnDef
+      def.getExpr().getAControlFlowNode() = cfnDef
     |
       isSuccessor = true and j >= i
       or
@@ -208,7 +208,7 @@ abstract class ControlFlowReachabilityConfiguration extends string {
     or
     exists(ControlFlow::BasicBlock bb |
       this.reachesBasicBlockDefinitionRec(e, def, _, cfn, bb) and
-      def.getAControlFlowNode() = cfnDef and
+      def.getExpr().getAControlFlowNode() = cfnDef and
       cfnDef = bb.getANode()
     )
   }

csharp/ql/lib/semmle/code/csharp/dispatch/Dispatch.qll

@@ -314,12 +314,8 @@ private module Internal {
       1 < strictcount(this.getADynamicTarget().getUnboundDeclaration()) and
       c = this.getCall().getEnclosingCallable().getUnboundDeclaration() and
       (
-        exists(
-          BaseSsa::Definition def, AssignableDefinitions::ImplicitParameterDefinition pdef,
-          Parameter p
-        |
-          pdef = def.getDefinition() and
-          p = pdef.getTarget() and
+        exists(BaseSsa::Definition def, Parameter p |
+          def.isImplicitEntryDefinition(p) and
           this.getSyntheticQualifier() = def.getARead() and
           p.getPosition() = i and
           c.getAParameter() = p and