C#: Replace CFG with the shared implementation

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

@@ -22,7 +22,7 @@ private import TypeRef
  * an anonymous function (`AnonymousFunctionExpr`), or a local function
  * (`LocalFunction`).
  */
-class Callable extends Parameterizable, ExprOrStmtParent, @callable {
+class Callable extends Parameterizable, ControlFlowElementOrCallable, @callable {
   /** Gets the return type of this callable. */
   Type getReturnType() { none() }
 

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

@@ -8,6 +8,10 @@ private import ControlFlow::BasicBlocks
 private import semmle.code.csharp.Caching
 private import internal.ControlFlowGraphImpl as Impl
 
+private class TControlFlowElementOrCallable = @callable or @control_flow_element;
+
+class ControlFlowElementOrCallable extends ExprOrStmtParent, TControlFlowElementOrCallable { }
+
 /**
  * A program element that can possess control flow. That is, either a statement or
  * an expression.
@@ -17,7 +21,7 @@ private import internal.ControlFlowGraphImpl as Impl
  * control flow elements and control flow nodes. This allows control flow
  * splitting, for example modeling the control flow through `finally` blocks.
  */
-class ControlFlowElement extends ExprOrStmtParent, @control_flow_element {
+class ControlFlowElement extends ControlFlowElementOrCallable, @control_flow_element {
   /** Gets the enclosing callable of this element, if any. */
   Callable getEnclosingCallable() { none() }
 

java/ql/lib/semmle/code/java/ControlFlowGraph.qll

@@ -91,9 +91,13 @@ private module Ast implements AstSig<Location> {
 
   class ContinueStmt = J::ContinueStmt;
 
+  class GotoStmt extends Stmt {
+    GotoStmt() { none() }
+  }
+
   class ReturnStmt = J::ReturnStmt;
 
-  class ThrowStmt = J::ThrowStmt;
+  class Throw = J::ThrowStmt;
 
   final private class FinalTryStmt = J::TryStmt;
 
@@ -181,11 +185,37 @@ private module Ast implements AstSig<Location> {
 
   class LogicalNotExpr = LogNotExpr;
 
+  class Assignment = J::Assignment;
+
+  class AssignExpr = J::AssignExpr;
+
+  class CompoundAssignment = J::AssignOp;
+
+  class AssignLogicalAndExpr extends CompoundAssignment {
+    AssignLogicalAndExpr() { none() }
+  }
+
+  class AssignLogicalOrExpr extends CompoundAssignment {
+    AssignLogicalOrExpr() { none() }
+  }
+
+  class AssignNullCoalescingExpr extends CompoundAssignment {
+    AssignNullCoalescingExpr() { none() }
+  }
+
   final private class FinalBooleanLiteral = J::BooleanLiteral;
 
   class BooleanLiteral extends FinalBooleanLiteral {
     boolean getValue() { result = this.getBooleanValue() }
   }
+
+  final private class FinalInstanceOfExpr = J::InstanceOfExpr;
+
+  class PatternMatchExpr extends FinalInstanceOfExpr {
+    PatternMatchExpr() { this.isPattern() }
+
+    AstNode getPattern() { result = super.getPattern() }
+  }
 }
 
 private module Exceptions {
@@ -522,14 +552,8 @@ private module Input implements InputSig1, InputSig2 {
 
   private string assertThrowNodeTag() { result = "[assert-throw]" }
 
-  private string instanceofTrueNodeTag() { result = "[instanceof-true]" }
-
   predicate additionalNode(Ast::AstNode n, string tag, NormalSuccessor t) {
     n instanceof AssertStmt and tag = assertThrowNodeTag() and t instanceof DirectSuccessor
-    or
-    n.(InstanceOfExpr).isPattern() and
-    tag = instanceofTrueNodeTag() and
-    t.(BooleanSuccessor).getValue() = true
   }
 
   /**
@@ -571,34 +595,6 @@ private module Input implements InputSig1, InputSig2 {
 
   /** Holds if there is a local non-abrupt step from `n1` to `n2`. */
   predicate step(PreControlFlowNode n1, PreControlFlowNode n2) {
-    exists(InstanceOfExpr ioe |
-      // common
-      n1.isBefore(ioe) and
-      n2.isBefore(ioe.getExpr())
-      or
-      n1.isAfter(ioe.getExpr()) and
-      n2.isIn(ioe)
-      or
-      // std postorder:
-      not ioe.isPattern() and
-      n1.isIn(ioe) and
-      n2.isAfter(ioe)
-      or
-      // pattern case:
-      ioe.isPattern() and
-      n1.isIn(ioe) and
-      n2.isAfterValue(ioe, any(BooleanSuccessor s | s.getValue() = false))
-      or
-      n1.isIn(ioe) and
-      n2.isAdditional(ioe, instanceofTrueNodeTag())
-      or
-      n1.isAdditional(ioe, instanceofTrueNodeTag()) and
-      n2.isBefore(ioe.getPattern())
-      or
-      n1.isAfter(ioe.getPattern()) and
-      n2.isAfterValue(ioe, any(BooleanSuccessor s | s.getValue() = true))
-    )
-    or
     exists(AssertStmt assertstmt |
       n1.isBefore(assertstmt) and
       n2.isBefore(assertstmt.getExpr())

shared/controlflow/codeql/controlflow/ControlFlowGraph.qll

@@ -143,6 +143,13 @@ signature module AstSig<LocationSig Location> {
    */
   class ContinueStmt extends Stmt;
 
+  /**
+   * A `goto` statement.
+   *
+   * Goto statements complete abruptly and jump to a labeled statement.
+   */
+  class GotoStmt extends Stmt;
+
   /**
    * A `return` statement.
    *
@@ -155,11 +162,11 @@ signature module AstSig<LocationSig Location> {
   }
 
   /**
-   * A `throw` statement.
+   * A `throw` statement or expression.
    *
-   * Throw statements complete abruptly and throw an exception.
+   * Throw statements/expressions complete abruptly and throw an exception.
    */
-  class ThrowStmt extends Stmt {
+  class Throw extends AstNode {
     /** Gets the expression being thrown. */
     Expr getExpr();
   }
@@ -302,11 +309,51 @@ signature module AstSig<LocationSig Location> {
   /** A logical NOT expression. */
   class LogicalNotExpr extends UnaryExpr;
 
+  /**
+   * An assignment expression, either compound or simple.
+   *
+   * Examples:
+   *
+   * ```
+   * x = y
+   * sum += element
+   * ```
+   */
+  class Assignment extends BinaryExpr;
+
+  /** A simple assignment expression, for example `x = y`. */
+  class AssignExpr extends Assignment;
+
+  /** A compound assignment expression, for example `x += y` or `x ??= y`. */
+  class CompoundAssignment extends Assignment;
+
+  /** A short-circuiting logical AND compound assignment expression. */
+  class AssignLogicalAndExpr extends CompoundAssignment;
+
+  /** A short-circuiting logical OR compound assignment expression. */
+  class AssignLogicalOrExpr extends CompoundAssignment;
+
+  /** A short-circuiting null-coalescing compound assignment expression. */
+  class AssignNullCoalescingExpr extends CompoundAssignment;
+
   /** A boolean literal expression. */
   class BooleanLiteral extends Expr {
     /** Gets the boolean value of this literal. */
     boolean getValue();
   }
+
+  /**
+   * A pattern matching expression.
+   *
+   * In Java this is `x instanceof Pattern`, and in C# this is `x is Pattern`.
+   */
+  class PatternMatchExpr extends Expr {
+    /** Gets the expression being matched. */
+    Expr getExpr();
+
+    /** Gets the pattern being matched against. */
+    AstNode getPattern();
+  }
 }
 
 /**
@@ -426,12 +473,14 @@ module Make0<LocationSig Location, AstSig<Location> Ast> {
       or
       n instanceof ReturnStmt
       or
-      n instanceof ThrowStmt
+      n instanceof Throw
       or
       n instanceof BreakStmt
       or
       n instanceof ContinueStmt
       or
+      n instanceof GotoStmt
+      or
       n instanceof Expr and
       exists(getChild(n, _)) and
       not Input1::preOrderExpr(n) and
@@ -449,11 +498,14 @@ module Make0<LocationSig Location, AstSig<Location> Ast> {
      * is the value that causes the short-circuit.
      */
     private predicate shortCircuiting(BinaryExpr expr, ConditionalSuccessor shortcircuitValue) {
-      expr instanceof LogicalAndExpr and shortcircuitValue.(BooleanSuccessor).getValue() = false
+      (expr instanceof LogicalAndExpr or expr instanceof AssignLogicalAndExpr) and
+      shortcircuitValue.(BooleanSuccessor).getValue() = false
       or
-      expr instanceof LogicalOrExpr and shortcircuitValue.(BooleanSuccessor).getValue() = true
+      (expr instanceof LogicalOrExpr or expr instanceof AssignLogicalOrExpr) and
+      shortcircuitValue.(BooleanSuccessor).getValue() = true
       or
-      expr instanceof NullCoalescingExpr and shortcircuitValue.(NullnessSuccessor).getValue() = true
+      (expr instanceof NullCoalescingExpr or expr instanceof AssignNullCoalescingExpr) and
+      shortcircuitValue.(NullnessSuccessor).getValue() = false
     }
 
     /**
@@ -463,9 +515,10 @@ module Make0<LocationSig Location, AstSig<Location> Ast> {
     private predicate propagatesValue(AstNode child, AstNode parent) {
       Input1::propagatesValue(child, parent)
       or
-      // For now, the `not postOrInOrder(parent)` is superfluous, as we don't
-      // have any short-circuiting post-order expressions yet, but this will
-      // change once we add support for e.g. C#'s `??=`.
+      // Short-circuiting post-order expressions, i.e. short-circuiting
+      // compound assignments, e.g. C#'s `??=`, cannot propagate the value of
+      // the right-hand side to the parent, as the assignment must take place
+      // in-between, so propagating the value would imply splitting.
       shortCircuiting(parent, _) and
       not postOrInOrder(parent) and
       parent.(BinaryExpr).getRightOperand() = child
@@ -535,6 +588,8 @@ module Make0<LocationSig Location, AstSig<Location> Ast> {
 
     private string loopHeaderTag() { result = "[LoopHeader]" }
 
+    private string patternMatchTrueTag() { result = "[match-true]" }
+
     /**
      * Holds if an additional node tagged with `tag` should be created for
      * `n`. Edges targeting such nodes are labeled with `t` and therefore `t`
@@ -546,6 +601,10 @@ module Make0<LocationSig Location, AstSig<Location> Ast> {
       n instanceof LoopStmt and
       tag = loopHeaderTag() and
       t instanceof DirectSuccessor
+      or
+      n instanceof PatternMatchExpr and
+      tag = patternMatchTrueTag() and
+      t.(BooleanSuccessor).getValue() = true
     }
 
     /**
@@ -561,9 +620,11 @@ module Make0<LocationSig Location, AstSig<Location> Ast> {
       or
       n instanceof ContinueStmt
       or
+      n instanceof GotoStmt
+      or
       n instanceof ReturnStmt
       or
-      n instanceof ThrowStmt
+      n instanceof Throw
       or
       cannotTerminateNormally(n.(BlockStmt).getLastStmt())
       or
@@ -992,14 +1053,17 @@ module Make0<LocationSig Location, AstSig<Location> Ast> {
           ast instanceof ReturnStmt and
           c.getSuccessorType() instanceof ReturnSuccessor
           or
-          ast instanceof ThrowStmt and
+          ast instanceof Throw and
           c.getSuccessorType() instanceof ExceptionSuccessor
           or
           ast instanceof BreakStmt and
           c.getSuccessorType() instanceof BreakSuccessor
           or
           ast instanceof ContinueStmt and
           c.getSuccessorType() instanceof ContinueSuccessor
+          or
+          ast instanceof GotoStmt and
+          c.getSuccessorType() instanceof GotoSuccessor
         ) and
         (
           not Input1::hasLabel(ast, _) and not c.hasLabel(_)
@@ -1020,6 +1084,11 @@ module Make0<LocationSig Location, AstSig<Location> Ast> {
         )
       }
 
+      private Stmt getAStmtInBlock(AstNode block) {
+        result = block.(BlockStmt).getStmt(_) or
+        result = block.(Switch).getStmt(_)
+      }
+
       /**
        * Holds if an abrupt completion `c` from within `ast` is caught with
        * flow continuing at `n`.
@@ -1098,6 +1167,16 @@ module Make0<LocationSig Location, AstSig<Location> Ast> {
             Input1::hasLabel(switch, l)
           )
         )
+        or
+        exists(AstNode block, Input1::Label l, Stmt lblstmt |
+          ast = getAStmtInBlock(block) and
+          lblstmt = getAStmtInBlock(block) and
+          not lblstmt instanceof GotoStmt and
+          Input1::hasLabel(pragma[only_bind_into](lblstmt), l) and
+          n.isBefore(lblstmt) and
+          c.getSuccessorType() instanceof GotoSuccessor and
+          c.hasLabel(l)
+        )
       }
 
       /**
@@ -1214,7 +1293,7 @@ module Make0<LocationSig Location, AstSig<Location> Ast> {
           n1.isAfterValue(binexpr.getLeftOperand(), shortcircuitValue) and
           n2.isAfterValue(binexpr, shortcircuitValue)
           or
-          // short-circuiting operations with side-effects (e.g. `x &&= y`, `x?.Prop = y`) are in post-order:
+          // short-circuiting operations with side-effects (e.g. `x &&= y`) are in post-order:
           n1.isAfter(binexpr.getRightOperand()) and
           n2.isIn(binexpr)
           or
@@ -1249,6 +1328,26 @@ module Make0<LocationSig Location, AstSig<Location> Ast> {
           n2.isAfterValue(boollit, any(BooleanSuccessor t | t.getValue() = boollit.getValue()))
         )
         or
+        exists(PatternMatchExpr pme |
+          n1.isBefore(pme) and
+          n2.isBefore(pme.getExpr())
+          or
+          n1.isAfter(pme.getExpr()) and
+          n2.isIn(pme)
+          or
+          n1.isIn(pme) and
+          n2.isAfterValue(pme, any(BooleanSuccessor s | s.getValue() = false))
+          or
+          n1.isIn(pme) and
+          n2.isAdditional(pme, patternMatchTrueTag())
+          or
+          n1.isAdditional(pme, patternMatchTrueTag()) and
+          n2.isBefore(pme.getPattern())
+          or
+          n1.isAfter(pme.getPattern()) and
+          n2.isAfterValue(pme, any(BooleanSuccessor s | s.getValue() = true))
+        )
+        or
         exists(IfStmt ifstmt |
           n1.isBefore(ifstmt) and
           n2.isBefore(ifstmt.getCondition())