C#: Include parameters and their defaults in the CFG

csharp/ql/lib/printCfg.ql

@@ -22,14 +22,16 @@ external int selectedSourceColumn();
 private predicate selectedSourceColumnAlias = selectedSourceColumn/0;
 
 module ViewCfgQueryInput implements ControlFlow::ViewCfgQueryInputSig<File> {
+  private import semmle.code.csharp.controlflow.ControlFlowGraph
+
   predicate selectedSourceFile = selectedSourceFileAlias/0;
 
   predicate selectedSourceLine = selectedSourceLineAlias/0;
 
   predicate selectedSourceColumn = selectedSourceColumnAlias/0;
 
   predicate cfgScopeSpan(
-    Callable scope, File file, int startLine, int startColumn, int endLine, int endColumn
+    Ast::Callable scope, File file, int startLine, int startColumn, int endLine, int endColumn
   ) {
     file = scope.getFile() and
     scope.getLocation().getStartLine() = startLine and

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

@@ -271,6 +271,8 @@
     def = TPatternDefinition(result)
     or
     def = TAssignOperationDefinition(result)
+    or
+    def = TParameterDefaultDefinition(_, result)
   }
 
   /** A local variable declaration at the top-level of a pattern. */
@@ -308,10 +310,17 @@
         exists(Callable c | p = c.getAParameter() |
           c.hasBody()
           or
-          // Same as `c.(Constructor).hasInitializer()`, but avoids negative recursion warning
-          c.getAChildExpr() instanceof @constructor_init_expr
+          c.(Constructor).hasInitializer()
         )
       } or
+      TParameterDefaultDefinition(Parameter p, Expr default) {
+        exists(Callable c | p = c.getAParameter() |
+          c.hasBody()
+          or
+          c.(Constructor).hasInitializer()
+        ) and
+        default = p.getDefaultValue()
+      } or
       TAddressOfDefinition(AddressOfExpr aoe) or
       TPatternDefinition(TopLevelPatternDecl tlpd) or
       TAssignOperationDefinition(AssignOperation ao) {
@@ -349,6 +358,8 @@
         any(AssignableDefinitions::PatternDefinition pd | result = pd.getDeclaration().getVariable())
       or
       def = any(AssignableDefinitions::InitializerDefinition init | result = init.getAssignable())
+      or
+      def = TParameterDefaultDefinition(result, _)
     }
 
     // Not defined by dispatch in order to avoid too conservative negative recursion error
@@ -492,11 +503,6 @@
       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()
-      )
     )
   }
 
@@ -688,7 +694,33 @@
 
     override string toString() { result = p.toString() }
 
-    override Location getLocation() { result = this.getTarget().getLocation() }
+    override Location getLocation() { result = p.getLocation() }
+  }
+
+  /**
+   * A default value assigned to a parameter.
+   */
+  class ParameterDefaultDefinition extends AssignableDefinition, TParameterDefaultDefinition {
+    Parameter p;
+    Expr default;
+
+    ParameterDefaultDefinition() { this = TParameterDefaultDefinition(p, default) }
+
+    /** Gets the underlying parameter. */
+    Parameter getParameter() { result = p }
+
+    /** Gets the default value expression for the parameter. */
+    Expr getDefaultValue() { result = default }
+
+    override Expr getSource() { result = default }
+
+    override Expr getElement() { result = default }
+
+    override Callable getEnclosingCallable() { result = p.getCallable() }
+
+    override string toString() { result = p.toString() + " = ..." }
+
+    override Location getLocation() { result = default.getLocation() }
   }
 
   /**

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

@@ -13,7 +13,7 @@ private import internal.Location
  * An element that can have a child statement or expression.
  */
 class ExprOrStmtParent extends Element, @exprorstmt_parent {
-  final override ControlFlowElement getChild(int i) {
+  override ControlFlowElement getChild(int i) {
     result = this.getChildExpr(i) or
     result = this.getChildStmt(i)
   }
@@ -42,14 +42,8 @@ class ExprOrStmtParent extends Element, @exprorstmt_parent {
  *
  * An element that can have a child top-level expression.
  */
-class TopLevelExprParent extends Element, @top_level_expr_parent {
+class TopLevelExprParent extends ExprOrStmtParent, @top_level_expr_parent {
   final override Expr getChild(int i) { result = this.getChildExpr(i) }
-
-  /** Gets the `i`th child expression of this element (zero-based). */
-  final Expr getChildExpr(int i) { expr_parent_top_level_adjusted(result, i, this) }
-
-  /** Gets a child expression of this element, if any. */
-  final Expr getAChildExpr() { result = this.getChildExpr(_) }
 }
 
 /** INTERNAL: Do not use. */

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

@@ -299,7 +299,9 @@ class ControlFlowElementNode extends ElementNode {
     not isNotNeeded(element.getParent+()) and
     // LambdaExpr is both a Callable and a ControlFlowElement,
     // print it with the more specific CallableNode
-    not element instanceof Callable
+    not element instanceof Callable and
+    // Handled in `ParameterNode`
+    not element instanceof Parameter
   }
 
   override PrintAstNode getChild(int childIndex) {

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

@@ -87,7 +87,9 @@ class LocalScopeVariable extends Variable, @local_scope_variable {
  * }
  * ```
  */
-class Parameter extends LocalScopeVariable, Attributable, TopLevelExprParent, @parameter {
+class Parameter extends LocalScopeVariable, Attributable, TopLevelExprParent, ControlFlowElement,
+  @parameter
+{
   /** Gets the raw position of this parameter, including the `this` parameter at index 0. */
   final int getRawPosition() { this = this.getDeclaringElement().getRawParameter(result) }
 

csharp/ql/lib/semmle/code/csharp/controlflow/ControlFlowGraph.qll

@@ -19,13 +19,17 @@
 private import Cfg2
 import Public
 
-/** Provides an implementation of the AST signature for C#. */
-private module Ast implements AstSig<Location> {
+/**
+ * INTERNAL: Do not use.
+ *
+ * Provides an implementation of the AST signature for C#.
+ */
+module Ast implements AstSig<Location> {
   private import csharp as CS
 
   class AstNode = ControlFlowElementOrCallable;
 
   additional predicate skipControlFlow(AstNode e) {
     e instanceof TypeAccess and
     not e instanceof TypeAccessPatternExpr
     or
@@ -73,18 +77,41 @@
   private AstNode getParent(AstNode n) { n = getChild(result, _) }
 
   Callable getEnclosingCallable(AstNode node) {
-    result = node.(ControlFlowElement).getEnclosingCallable() or
-    result.(ObjectInitMethod).initializes(getParent*(node)) or
+    result = node.(ControlFlowElement).getEnclosingCallable()
+    or
+    result.(ObjectInitMethod).initializes(getParent*(node))
+    or
     Initializers::staticMemberInitializer(result, getParent*(node))
+    or
+    result = node.(Parameter).getCallable()
+    or
+    not skipControlFlow(node) and
+    getParent*(node) = any(Parameter p | result = p.getCallable()).getDefaultValue()
   }
 
-  class Callable = CS::Callable;
+  class Callable extends CS::Callable {
+    Callable() { this.isUnboundDeclaration() }
+  }
 
   AstNode callableGetBody(Callable c) {
     not skipControlFlow(result) and
     result = c.getBody()
   }
 
+  final private class ParameterFinal = CS::Parameter;
+
+  class Parameter extends ParameterFinal {
+    Expr getDefaultValue() {
+      // Avoid combinatorial explosions for callables with multiple bodies
+      result = unique( | | super.getDefaultValue())
+    }
+  }
+
+  Parameter callableGetParameter(Callable c, int i) {
+    not skipControlFlow(result) and
+    result = c.getParameter(i)
+  }
+
   class Stmt = CS::Stmt;
 
   class Expr = CS::Expr;
@@ -165,7 +192,7 @@
     AstNode getBody() { result = super.getBody() }
   }
 
   class DefaultCase extends Case instanceof CS::DefaultCase { }
 
   class ConditionalExpr = CS::ConditionalExpr;
 
@@ -232,9 +259,11 @@
 }
 
 /** Gets the compilation that source file `f` belongs to. */
-private CompilationExt getCompilation(File f) {
+bindingset[e]
+pragma[inline_late]
+private CompilationExt getCompilation(Element e) {
   exists(Compilation c |
-    f = c.getAFileCompiled() and
+    e.getALocation().getFile() = c.getAFileCompiled() and
     result = TCompilation(c)
   )
   or
@@ -415,12 +444,12 @@
     l = TLblGoto(n.(LabelStmt).getLabel())
   }
 
-  class CallableBodyPartContext = CompilationExt;
+  class CallableContext = CompilationExt;
 
   pragma[nomagic]
-  Ast::AstNode callableGetBodyPart(Callable c, CallableBodyPartContext ctx, int index) {
+  Ast::AstNode callableGetBodyPart(Ast::Callable c, CallableContext ctx, int index) {
     not Ast::skipControlFlow(result) and
-    ctx = getCompilation(result.getFile()) and
+    ctx = getCompilation(result) and
     (
       result = Initializers::initializedInstanceMemberOrder(c, index)
       or
@@ -437,9 +466,19 @@
         or
         i = 2 and result = ctor.getBody()
       )
+      or
+      not c instanceof Constructor and
+      result = c.getBody() and
+      index = 0
     )
   }
 
+  pragma[nomagic]
+  Ast::Parameter callableGetParameter(Ast::Callable c, CallableContext ctx, int index) {
+    result = Ast::callableGetParameter(c, index) and
+    ctx = getCompilation(result)
+  }
+
   private Expr getQualifier(QualifiableExpr qe) {
     result = qe.getQualifier() or
     result = qe.(ExtensionMethodCall).getArgument(0)

csharp/ql/lib/semmle/code/csharp/controlflow/Guards.qll

@@ -205,7 +205,7 @@ private module LogicInput implements GuardsImpl::LogicInputSig {
     }
   }
 
-  class SsaParameterInit extends SsaDefinition instanceof Ssa::ImplicitParameterDefinition {
+  class SsaParameterInit extends SsaDefinition instanceof Ssa::ParameterDefinition {
     Parameter getParameter() { result = super.getParameter() }
   }
 

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

@@ -130,7 +130,7 @@
   d = def.getAReadAtNode(node)
 }
 
-private predicate isMaybeNullArgument(Ssa::ImplicitParameterDefinition def, MaybeNullExpr arg) {
+private predicate isMaybeNullArgument(Ssa::ParameterDefinition def, MaybeNullExpr arg) {
   exists(AssignableDefinitions::ImplicitParameterDefinition pdef, Parameter p |
     p = def.getParameter()
   |
@@ -181,7 +181,7 @@
   )
 }
 
-private predicate isNullDefaultArgument(Ssa::ImplicitParameterDefinition def, AlwaysNullExpr arg) {
+private predicate isNullDefaultArgument(Ssa::ParameterDefinition def, AlwaysNullExpr arg) {
   exists(AssignableDefinitions::ImplicitParameterDefinition pdef, Parameter p |
     p = def.getParameter()
   |
@@ -216,10 +216,6 @@
       else msg = "because of $@ potential null argument"
     )
     or
-    isNullDefaultArgument(def, reason) and
-    node = def.getControlFlowNode() and
-    msg = "because the parameter has a null default value"
-    or
     // If the source of a variable is `null` then the variable may be `null`
     exists(AssignableDefinition adef | adef = def.(Ssa::ExplicitDefinition).getADefinition() |
       adef.getSource() = maybeNullExpr(node.asExpr()) and
@@ -337,8 +333,7 @@
         or
         p.fromSource() and
         exists(
-          Ssa::ImplicitParameterDefinition def,
-          AssignableDefinitions::ImplicitParameterDefinition pdef
+          Ssa::ParameterDefinition def, AssignableDefinitions::ImplicitParameterDefinition pdef
         |
           p = def.getParameter()
         |