Merge pull request #10594 from michaelnebel/csharp/postupdatenotes

C#: Postupdate notes for ternary expressions.

Author: michaelnebel

csharp/ql/consistency-queries/DataFlowConsistency.ql

@@ -35,13 +35,7 @@ private class MyConsistencyConfiguration extends ConsistencyConfiguration {
   override predicate argHasPostUpdateExclude(ArgumentNode n) {
     n instanceof SummaryNode
     or
-    n.asExpr().(Expr).stripCasts().getType() =
-      any(Type t |
-        not t instanceof RefType and
-        not t = any(TypeParameter tp | not tp.isValueType())
-        or
-        t instanceof NullType
-      )
+    not exists(LocalFlow::getAPostUpdateNodeForArg(n.getControlFlowNode()))
     or
     n instanceof ImplicitCapturedArgumentNode
     or
@@ -50,5 +44,21 @@ private class MyConsistencyConfiguration extends ConsistencyConfiguration {
     n.asExpr() instanceof CIL::Expr
   }
 
+  override predicate postHasUniquePreExclude(PostUpdateNode n) {
+    exists(ControlFlow::Nodes::ExprNode e, ControlFlow::Nodes::ExprNode arg |
+      e = LocalFlow::getAPostUpdateNodeForArg(arg) and
+      e != arg and
+      n = TExprPostUpdateNode(e)
+    )
+  }
+
+  override predicate uniquePostUpdateExclude(Node n) {
+    exists(ControlFlow::Nodes::ExprNode e, ControlFlow::Nodes::ExprNode arg |
+      e = LocalFlow::getAPostUpdateNodeForArg(arg) and
+      e != arg and
+      n.asExpr() = arg.getExpr()
+    )
+  }
+
   override predicate reverseReadExclude(Node n) { n.asExpr() = any(AwaitExpr ae).getExpr() }
 }

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

@@ -410,6 +410,34 @@ module LocalFlow {
     n instanceof SummaryNode or
     n instanceof ImplicitCapturedArgumentNode
   }
+
+  /**
+   * Gets a node that may execute last in `n`, and which, when it executes last,
+   * will be the value of `n`.
+   */
+  private ControlFlow::Nodes::ExprNode getALastEvalNode(ControlFlow::Nodes::ExprNode cfn) {
+    exists(Expr e | any(LocalExprStepConfiguration x).hasExprPath(_, result, e, cfn) |
+      e instanceof ConditionalExpr or
+      e instanceof Cast or
+      e instanceof NullCoalescingExpr or
+      e instanceof SwitchExpr or
+      e instanceof SuppressNullableWarningExpr or
+      e instanceof AssignExpr
+    )
+  }
+
+  /** Gets a node for which to construct a post-update node for argument `arg`. */
+  ControlFlow::Nodes::ExprNode getAPostUpdateNodeForArg(ControlFlow::Nodes::ExprNode arg) {
+    arg.getExpr() instanceof Argument and
+    result = getALastEvalNode*(arg) and
+    exists(Expr e, Type t | result.getExpr() = e and t = e.stripCasts().getType() |
+      t instanceof RefType and
+      not t instanceof NullType
+      or
+      t = any(TypeParameter tp | not tp.isValueType())
+    ) and
+    not exists(getALastEvalNode(result))
+  }
 }
 
 /**
@@ -719,14 +747,9 @@ private module Cached {
       cfn.getElement().(ObjectCreation).hasInitializer()
     } or
     TExprPostUpdateNode(ControlFlow::Nodes::ExprNode cfn) {
+      cfn = LocalFlow::getAPostUpdateNodeForArg(_)
+      or
       exists(Expr e | e = cfn.getExpr() |
-        exists(Type t | t = e.(Argument).stripCasts().getType() |
-          t instanceof RefType and
-          not t instanceof NullType
-          or
-          t = any(TypeParameter tp | not tp.isValueType())
-        )
-        or
         fieldOrPropertyStore(_, _, _, e, true)
         or
         arrayStore(_, _, e, true)
@@ -1921,7 +1944,18 @@ private module PostUpdateNodes {
 
     ExprPostUpdateNode() { this = TExprPostUpdateNode(cfn) }
 
-    override ExprNode getPreUpdateNode() { cfn = result.getControlFlowNode() }
+    override ExprNode getPreUpdateNode() {
+      // For compund arguments, such as `m(b ? x : y)`, we want the leaf nodes
+      // `[post] x` and `[post] y` to have two pre-update nodes: (1) the compund argument,
+      // `if b then x else y`; and the (2) the underlying expressions; `x` and `y`,
+      // respectively.
+      //
+      // This ensures that we get flow out of the call into both leafs (1), while still
+      // maintaining the invariant that the underlying expression is a pre-update node (2).
+      cfn = LocalFlow::getAPostUpdateNodeForArg(result.getControlFlowNode())
+      or
+      cfn = result.getControlFlowNode()
+    }
 
     override DataFlowCallable getEnclosingCallableImpl() {
       result.asCallable() = cfn.getEnclosingCallable()

csharp/ql/test/library-tests/dataflow/global/DataFlow.expected

@@ -53,6 +53,19 @@
 | GlobalDataFlow.cs:427:41:427:46 | access to local variable sink20 |
 | GlobalDataFlow.cs:478:15:478:20 | access to local variable sink45 |
 | GlobalDataFlow.cs:486:32:486:32 | access to parameter s |
+| GlobalDataFlow.cs:508:15:508:22 | access to field field |
+| GlobalDataFlow.cs:509:15:509:22 | access to field field |
+| GlobalDataFlow.cs:515:15:515:22 | access to field field |
+| GlobalDataFlow.cs:516:15:516:22 | access to field field |
+| GlobalDataFlow.cs:517:15:517:22 | access to field field |
+| GlobalDataFlow.cs:526:15:526:21 | access to field field |
+| GlobalDataFlow.cs:533:15:533:21 | access to field field |
+| GlobalDataFlow.cs:534:15:534:21 | access to field field |
+| GlobalDataFlow.cs:548:15:548:21 | access to field field |
+| GlobalDataFlow.cs:549:15:549:21 | access to field field |
+| GlobalDataFlow.cs:550:15:550:21 | access to field field |
+| GlobalDataFlow.cs:556:15:556:22 | access to field field |
+| GlobalDataFlow.cs:564:15:564:21 | access to field field |
 | Splitting.cs:9:15:9:15 | [b (line 3): false] access to local variable x |
 | Splitting.cs:9:15:9:15 | [b (line 3): true] access to local variable x |
 | Splitting.cs:11:19:11:19 | access to local variable x |