C#: Include parameters and their defaults in the CFG

Author: hvitved

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/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,8 +19,12 @@ private import Cfg1
 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;
@@ -73,18 +77,41 @@ private module Ast implements AstSig<Location> {
   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() {
+      result = super.getDefaultValue() and
+      getCompilation(result) = getCompilation(this)
+    }
+  }
+
+  Parameter callableGetParameter(Callable c, int i) {
+    not skipControlFlow(result) and
+    result = c.getParameter(i)
+  }
+
   class Stmt = CS::Stmt;
 
   class Expr = CS::Expr;
@@ -232,9 +259,11 @@ private class CompilationExt extends TCompilationExt {
 }
 
 /** 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 @@ private module Input implements InputSig1, InputSig2 {
     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 @@ private module Input implements InputSig1, InputSig2 {
         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/dataflow/internal/DataFlowPrivate.qll

@@ -918,17 +918,13 @@ private Gvn::GvnType getANonTypeParameterSubTypeRestricted(RelevantGvnType t) {
 
 /** A callable with an implicit `this` parameter. */
 private class InstanceCallable extends Callable {
-  private Location l;
-
   InstanceCallable() {
     not this.(Modifiable).isStatic() and
     // local functions and delegate capture `this` and should therefore
     // not have a `this` parameter
     not this instanceof LocalFunction and
     not this instanceof AnonymousFunctionExpr
   }
-
-  Location getARelevantLocation() { result = l }
 }
 
 /**
@@ -1019,8 +1015,7 @@ private module Cached {
     } or
     TInstanceParameterNode(InstanceCallable c, Location l) {
       c = any(DataFlowCallable dfc).asCallable(l) and
-      c instanceof CallableUsedInSource and
-      l = c.getARelevantLocation()
+      c instanceof CallableUsedInSource
     } or
     TDelegateSelfReferenceNode(Callable c) { lambdaCreationExpr(_, c) } or
     TLocalFunctionCreationNode(ControlFlowNodes::ElementNode cfn, Boolean isPostUpdate) {

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

@@ -753,13 +753,11 @@ private module Cached {
 
   cached
   predicate implicitEntryDefinition(BasicBlock bb, Ssa::SourceVariable v) {
-    exists(EntryBasicBlock entry, Callable c |
-      c = entry.getEnclosingCallable() and
+    exists(Callable c |
       // In case `c` has multiple bodies, we want each body to get 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
+      // entry definition, so we use the basic block containing the body instead of
+      // the entry block.
+      bb = c.getBody().getBasicBlock() and
       c = v.getEnclosingCallable()
     |
       // Captured variable

csharp/ql/lib/semmlecode.csharp.dbscheme

@@ -1362,7 +1362,7 @@ compiler_generated(unique int id: @element ref);
 
 /** CONTROL/DATA FLOW **/
 
-@control_flow_element = @stmt | @expr;
+@control_flow_element = @stmt | @expr | @parameter;
 
 /* XML Files */