Merge pull request #16875 from hvitved/csharp/ssa-param-def

C#: Move implicit entry definitions inside method bodies in SSA construction

Author: hvitved

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/ExprOrStmtParent.qll

@@ -5,6 +5,7 @@
  */
 
 import csharp
+private import internal.Location
 
 /**
  * INTERNAL: Do not use.
@@ -65,14 +66,6 @@ private ControlFlowElement getBody(Callable c) {
   result = getStatementBody(c)
 }
 
-pragma[nomagic]
-private SourceLocation getASourceLocation(Element e) {
-  result = e.getALocation().(SourceLocation) and
-  not exists(e.getALocation().(SourceLocation).getMappedLocation())
-  or
-  result = e.getALocation().(SourceLocation).getMappedLocation()
-}
-
 pragma[nomagic]
 private predicate hasNoSourceLocation(Element e) { not exists(getASourceLocation(e)) }
 

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

@@ -50,6 +50,27 @@ class Location extends @location {
 
   /** Gets the 1-based column number (inclusive) where this location ends. */
   final int getEndColumn() { this.hasLocationInfo(_, _, _, _, result) }
+
+  /** Holds if this location starts strictly before the specified location. */
+  bindingset[this, other]
+  pragma[inline_late]
+  predicate strictlyBefore(Location other) {
+    this.getFile() = other.getFile() and
+    (
+      this.getStartLine() < other.getStartLine()
+      or
+      this.getStartLine() = other.getStartLine() and this.getStartColumn() < other.getStartColumn()
+    )
+  }
+
+  /** Holds if this location starts before the specified location. */
+  bindingset[this, other]
+  pragma[inline_late]
+  predicate before(Location other) {
+    this.getStartLine() < other.getStartLine()
+    or
+    this.getStartLine() = other.getStartLine() and this.getStartColumn() <= other.getStartColumn()
+  }
 }
 
 /** An empty location. */

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

@@ -9,6 +9,24 @@ import csharp
  */
 module Ssa {
   private import internal.SsaImpl as SsaImpl
+  private import semmle.code.csharp.internal.Location
+
+  pragma[nomagic]
+  private predicate assignableDefinitionLocalScopeVariable(
+    AssignableDefinition ad, LocalScopeVariable v, Callable c
+  ) {
+    ad.getTarget() = v and
+    ad.getEnclosingCallable() = c
+  }
+
+  pragma[nomagic]
+  private predicate localScopeSourceVariable(
+    SourceVariables::LocalScopeSourceVariable sv, LocalScopeVariable v, Callable c1, Callable c2
+  ) {
+    sv.getAssignable() = v and
+    sv.getEnclosingCallable() = c1 and
+    v.getCallable() = c2
+  }
 
   /**
    * A variable that can be SSA converted.
@@ -34,11 +52,10 @@ module Ssa {
       or
       // Local variable declaration without initializer
       not exists(result.getTargetAccess()) and
-      this =
-        any(SourceVariables::LocalScopeSourceVariable v |
-          result.getTarget() = v.getAssignable() and
-          result.getEnclosingCallable() = v.getEnclosingCallable()
-        )
+      exists(LocalScopeVariable v, Callable c |
+        assignableDefinitionLocalScopeVariable(result, v, c) and
+        localScopeSourceVariable(this, v, c, _)
+      )
     }
 
     /**
@@ -507,9 +524,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 +552,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 +566,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 +581,54 @@ module Ssa {
     override Location getLocation() { result = this.getCallable().getLocation() }
   }
 
+  private module NearestLocationInput implements NearestLocationInputSig {
+    class C = ImplicitParameterDefinition;
+
+    predicate relevantLocations(ImplicitParameterDefinition def, Location l1, Location l2) {
+      not def.getBasicBlock() instanceof ControlFlow::BasicBlocks::EntryBlock and
+      l1 = def.getParameter().getALocation() and
+      l2 = def.getBasicBlock().getLocation()
+    }
+  }
+
+  pragma[nomagic]
+  private predicate implicitEntryDef(ImplicitEntryDefinition def, SourceVariable v, Callable c) {
+    v = def.getSourceVariable() and
+    c = def.getCallable()
+  }
+
+  /**
+   * An SSA definition representing the implicit initialization of a parameter
+   * at the beginning of a callable.
+   */
+  class ImplicitParameterDefinition extends ImplicitEntryDefinition {
+    private Parameter p;
+
+    ImplicitParameterDefinition() {
+      exists(SourceVariable sv, Callable c |
+        implicitEntryDef(this, sv, c) and
+        localScopeSourceVariable(sv, p, _, c)
+      )
+    }
+
+    /** 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() {
+      not NearestLocation<NearestLocationInput>::nearestLocation(this, _, _) and
+      result = p.getLocation()
+      or
+      // multi-bodied method: use matching parameter location
+      NearestLocation<NearestLocationInput>::nearestLocation(this, result, _)
+    }
+  }
+
   /**
    * 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,19 @@ 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`, because `entry.getFirstNode().getASuccessor()`
+      // will be in the entry block.
+      bb = entry.getFirstNode().getASuccessor().getBasicBlock() and
+      c = v.getCallable()
+    |
+      v.isReadonlyCapturedBy(c)
+      or
+      v instanceof Parameter
+    )
   }
 
   private module SsaInput implements SsaImplCommon::InputSig<Location> {
@@ -85,6 +96,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()
   )
 }
 
@@ -354,9 +355,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) }
@@ -1100,13 +1099,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()
@@ -1353,10 +1349,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