C#: Address review comments.

Author: michaelnebel

csharp/ql/consistency-queries/DataFlowConsistency.ql

@@ -60,12 +60,6 @@ private module Input implements InputSig<Location, CsharpDataFlow> {
       qe.isConditional() and
       qe.getQualifier() = arg.asExpr()
     )
-    or
-    // TODO: Remove once underlying issue is fixed
-    exists(AssignableDefinitions::OutRefDefinition def |
-      def.getTargetAccess().(QualifiableExpr) = arg.asExpr() and
-      def.getTargetAccess().isOutArgument()
-    )
   }
 
   predicate multipleArgumentCallExclude(ArgumentNode arg, DataFlowCall call) {

csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll

@@ -430,7 +430,6 @@ module Expressions {
       not this instanceof ArrayCreation and
       not this instanceof QualifiedWriteAccess and
       not this instanceof QualifiedAccessorWrite and
-      not this instanceof QualifiedAccessorWriteOutParam and
       not this instanceof NoNodeExpr and
       not this instanceof SwitchExpr and
       not this instanceof SwitchCaseExpr and
@@ -447,25 +446,29 @@ module Expressions {
   }
 
   /**
-   * A qualified write access. In a qualified write access, the access itself is
-   * not evaluated, only the qualifier and the indexer arguments (if any).
+   * A qualified write access.
    *
-   * Note that the successor declaration in `QualifiedAccessorWrite` ensures that the access itself
+   * The successor declaration in `QualifiedAccessorWrite` ensures that the access itself
    * is evaluated after the qualifier and the indexer arguments (if any)
    * and the right hand side of the assignment.
+   *
+   * When a qualified write access is used as an `out/ref` argument, the access itself is evaluated immediately.
    */
   private class QualifiedWriteAccess extends ControlFlowTree instanceof WriteAccess, QualifiableExpr
   {
     QualifiedWriteAccess() {
-      this.hasQualifier()
-      or
-      // Member initializers like
-      // ```csharp
-      // new Dictionary<int, string>() { [0] = "Zero", [1] = "One", [2] = "Two" }
-      // ```
-      // need special treatment, because the accesses `[0]`, `[1]`, and `[2]`
-      // have no qualifier.
-      this = any(MemberInitializer mi).getLValue()
+      (
+        this.hasQualifier()
+        or
+        // Member initializers like
+        // ```csharp
+        // new Dictionary<int, string>() { [0] = "Zero", [1] = "One", [2] = "Two" }
+        // ```
+        // need special treatment, because the accesses `[0]`, `[1]`, and `[2]`
+        // have no qualifier.
+        this = any(MemberInitializer mi).getLValue()
+      ) and
+      not exists(AssignableDefinitions::OutRefDefinition def | def.getTargetAccess() = this)
     }
 
     final override predicate propagatesAbnormal(AstNode child) { child = getExprChild(this, _) }
@@ -486,101 +489,14 @@ module Expressions {
       exists(int i |
         last(getExprChild(this, i), pred, c) and
         c instanceof NormalCompletion and
-        (i != 0 or not c.(NullnessCompletion).isNull()) and
-        first(getExprChild(this, i + 1), succ)
-      )
-    }
-  }
-
-  /**
-   * An expression that writes via an accessor in an `out` parameter, for example `s = M(out x.Field)`,
-   * where `Field` is a field.
-   *
-   * Note that `ref` parameters are not included here as they are considered reads before the call.
-   * Ideally, we would model `ref` parameters as both reads and writes, but that is not currently supported.
-   *
-   * Accessor writes need special attention, because we need to model that the
-   * access is written after the method call.
-   *
-   * In the example above, this means we want a CFG that looks like
-   *
-   * ```csharp
-   * x -> call M -> x.Field -> s = M(out x.Field)
-   * ```
-   */
-  private class QualifiedAccessorWriteOutParam extends PostOrderTree instanceof Expr {
-    QualifiedAccessorWriteOutParam() {
-      exists(AssignableDefinitions::OutRefDefinition def |
-        def.getExpr() = this and
-        def.getTargetAccess() instanceof QualifiableExpr and
-        def.getTargetAccess().isOutArgument()
-      )
-    }
-
-    private QualifiableExpr getOutAccess(int i) {
-      result =
-        rank[i + 1](AssignableDefinitions::OutRefDefinition def |
-          def.getExpr() = this and
-          def.getTargetAccess() instanceof QualifiableExpr and
-          def.getTargetAccess().isOutArgument()
-        |
-          def order by def.getIndex()
-        ).getTargetAccess()
-    }
-
-    private QualifiableExpr getLastOutAccess() {
-      exists(int last |
-        result = this.getOutAccess(last) and
-        not exists(this.getOutAccess(last + 1))
-      )
-    }
-
-    final override predicate propagatesAbnormal(AstNode child) { child = getExprChild(this, _) }
-
-    final override predicate first(AstNode first) {
-      first(getExprChild(this, 0), first)
-      or
-      not exists(getExprChild(this, 0)) and
-      first = this
-    }
-
-    final override predicate last(AstNode last, Completion c) {
-      // The last ast node is the last out writeaccess.
-      // Completion from the call itself is propagated (required for eg. conditions).
-      last = this.getLastOutAccess() and
-      c.isValidFor(this)
-    }
-
-    final override predicate succ(AstNode pred, AstNode succ, Completion c) {
-      exists(int i |
-        last(getExprChild(this, i), pred, c) and
-        c instanceof NormalCompletion
-      |
-        // Post-order: flow from last element of last child to element itself
-        i = max(int j | exists(getExprChild(this, j))) and
-        succ = this
-        or
-        // Standard left-to-right evaluation
+        (if i = 0 then not c.(NullnessCompletion).isNull() else any()) and
         first(getExprChild(this, i + 1), succ)
       )
-      or
-      // Flow from this element to the first write access.
-      pred = this and
-      succ = this.getOutAccess(0) and
-      c.isValidFor(pred) and
-      c instanceof NormalCompletion
-      or
-      // Flow from one access to the next
-      exists(int i | pred = this.getOutAccess(i) |
-        succ = this.getOutAccess(i + 1) and
-        c.isValidFor(pred) and
-        c instanceof NormalCompletion
-      )
     }
   }
 
   /**
-   * An expression that writes via an accessor, for example `x.Prop = 0`,
+   * An expression that writes via a qualifiable expression, for example `x.Prop = 0`,
    * where `Prop` is a property.
    *
    * Accessor writes need special attention, because we need to model the fact
@@ -591,7 +507,7 @@ module Expressions {
    * x -> 0 -> set_Prop -> x.Prop = 0
    * ```
    *
-   * For consistency, control flow is implemented this way for all accessor writes.
+   * For consistency, control flow is implemented the same way for other qualified writes.
    * For example, `x.Field = 0`, where `Field` is a field, we want a CFG that looks like
    *
    * ```csharp
@@ -645,7 +561,7 @@ module Expressions {
       exists(int i |
         last(getExprChild(this, i), pred, c) and
         c instanceof NormalCompletion and
-        (i != 0 or not c.(NullnessCompletion).isNull()) and
+        (if i = 0 then not c.(NullnessCompletion).isNull() else any()) and
         first(getExprChild(this, i + 1), succ)
       )
       or