C#: Cleanup expanded assignments.

csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/Assignment.cs

@@ -23,15 +23,12 @@ public static Assignment Create(ExpressionNodeInfo info)
         protected override void PopulateExpression(TextWriter trapFile)
         {
             var operatorKind = OperatorKind;
+            // TODO: THIS CHECK CAN BE SIMPLIFIED - As we now always consider this to be an operator invocation.
             if (operatorKind.HasValue)
             {
-                // Convert assignment such as `a += b` into `a = a + b`.
-                var simpleAssignExpr = new Expression(new ExpressionInfo(Context, Type, Location, ExprKind.SIMPLE_ASSIGN, this, 2, isCompilerGenerated: true, null));
-                Create(Context, Syntax.Left, simpleAssignExpr, 1);
-                var opexpr = new Expression(new ExpressionInfo(Context, Type, Location, operatorKind.Value, simpleAssignExpr, 0, isCompilerGenerated: true, null));
-                Create(Context, Syntax.Left, opexpr, 0, isCompilerGenerated: true);
-                Create(Context, Syntax.Right, opexpr, 1);
-                opexpr.OperatorCall(trapFile, Syntax);
+                Create(Context, Syntax.Left, this, 0);
+                Create(Context, Syntax.Right, this, 1);
+                OperatorCall(trapFile, Syntax);
             }
             else
             {

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

@@ -78,6 +78,8 @@
       this.isRefArgument()
       or
       this = any(AssignableDefinitions::AddressOfDefinition def).getTargetAccess()
+      or
+      this = any(AssignableDefinitions::AssignOperationDefinition def).getTargetAccess()
     ) and
     not nameOfChild(_, this)
   }
@@ -271,6 +273,8 @@
     def = TAddressOfDefinition(result)
     or
     def = TPatternDefinition(result)
+    or
+    def = TAssignOperationDefinition(result)
   }
 
   /** A local variable declaration at the top-level of a pattern. */
@@ -286,7 +290,10 @@
   private module Cached {
     cached
     newtype TAssignableDefinition =
-      TAssignmentDefinition(Assignment a) { not a.getLValue() instanceof TupleExpr } or
+      TAssignmentDefinition(Assignment a) {
+        not a.getLValue() instanceof TupleExpr and
+        not a instanceof AssignOperation
+      } or
       TTupleAssignmentDefinition(AssignExpr ae, Expr leaf) { tupleAssignmentDefinition(ae, leaf) } or
       TOutRefDefinition(AssignableAccess aa) {
         aa.isOutArgument()
@@ -309,7 +316,8 @@
         )
       } or
       TAddressOfDefinition(AddressOfExpr aoe) or
-      TPatternDefinition(TopLevelPatternDecl tlpd)
+      TPatternDefinition(TopLevelPatternDecl tlpd) or
+      TAssignOperationDefinition(AssignOperation ao)
 
     /**
      * Gets the source expression assigned in tuple definition `def`, if any.
@@ -355,6 +363,8 @@
       def = TMutationDefinition(any(MutatorOperation mo | mo.getOperand() = result))
       or
       def = TAddressOfDefinition(any(AddressOfExpr aoe | aoe.getOperand() = result))
+      or
+      def = TAssignOperationDefinition(any(AssignOperation ao | ao.getLeftOperand() = result))
     }
 
     /**
@@ -369,7 +379,7 @@
       or
       exists(Assignment ass | ac = ass.getLValue() |
         result = ass.getRValue() and
-        not ass.(AssignOperation).hasExpandedAssignment()
+        not ass instanceof AssignOperation
       )
     }
   }
@@ -388,8 +398,9 @@
  * a mutation update (`AssignableDefinitions::MutationDefinition`), a local variable
  * declaration without an initializer (`AssignableDefinitions::LocalVariableDefinition`),
  * an implicit parameter definition (`AssignableDefinitions::ImplicitParameterDefinition`),
- * an address-of definition (`AssignableDefinitions::AddressOfDefinition`), or a pattern
- * definition (`AssignableDefinitions::PatternDefinition`).
+ * an address-of definition (`AssignableDefinitions::AddressOfDefinition`), a pattern
+ * definition (`AssignableDefinitions::PatternDefinition`), or a compound assignment
+ * operation definition (`AssignableDefinitions::AssignOperationDefinition`)
  */
 class AssignableDefinition extends TAssignableDefinition {
   /**
@@ -511,7 +522,7 @@
 
     override Expr getSource() {
       result = a.getRValue() and
-      not a instanceof AssignOperation
+      not a instanceof AddOrRemoveEventExpr
     }
 
     override string toString() { result = a.toString() }
@@ -735,4 +746,17 @@
     /** Gets the assignable (field or property) being initialized. */
     Assignable getAssignable() { result = fieldOrProp }
   }
+
+  /**
+   * A definition by a compound assignment operation, for example `x += y`.
+   */
+  class AssignOperationDefinition extends AssignableDefinition, TAssignOperationDefinition {
+    AssignOperation ao;
+
+    AssignOperationDefinition() { this = TAssignOperationDefinition(ao) }
+
+    override Expr getSource() { result = ao }
+
+    override string toString() { result = ao.toString() }
+  }
 }

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

@@ -20,7 +20,7 @@
 
   /** Gets the `i`th child expression of this element (zero-based). */
   final Expr getChildExpr(int i) {
-    expr_parent_adjusted(result, i, this) or
+    expr_parent(result, i, this) or
     expr_parent_top_level_adjusted(result, i, this)
   }
 
@@ -119,66 +119,14 @@
   }
 
   /**
-   * The `expr_parent()` relation adjusted for expandable assignments. For example,
-   * the assignment `x += y` is extracted as
-   *
-   * ```
-   *          +=
-   *           |
-   *           2
-   *           |
-   *           =
-   *          / \
-   *         1   0
-   *        /     \
-   *       x       +
-   *              / \
-   *             1   0
-   *            /     \
-   *           x       y
-   * ```
-   *
-   * in order to be able to retrieve the expanded assignment `x = x + y` as the 2nd
-   * child. This predicate changes the diagram above into
-   *
-   * ```
-   *          +=
-   *         /  \
-   *        1    0
-   *       /      \
-   *      x        y
-   * ```
+   * Use `expr_parent` instead.
    */
   cached
-  predicate expr_parent_adjusted(Expr child, int i, ControlFlowElement parent) {
-    if parent instanceof AssignOperation
-    then
-      parent =
-        any(AssignOperation ao |
-          exists(AssignExpr ae | ae = ao.getExpandedAssignment() |
-            i = 0 and
-            exists(Expr right |
-              // right = `x + y`
-              expr_parent(right, 0, ae)
-            |
-              expr_parent(child, 1, right)
-            )
-            or
-            i = 1 and
-            expr_parent(child, 1, ae)
-          )
-          or
-          not ao.hasExpandedAssignment() and
-          expr_parent(child, i, parent)
-        )
-    else expr_parent(child, i, parent)
-  }
+  deprecated predicate expr_parent_adjusted(Expr child, int i, ControlFlowElement parent) { none() }
 
   private Expr getAChildExpr(ExprOrStmtParent parent) {
     result = parent.getAChildExpr() and
     not result = parent.(DeclarationWithGetSetAccessors).getExpressionBody()
-    or
-    result = parent.(AssignOperation).getExpandedAssignment()
   }
 
   private ControlFlowElement getAChild(ExprOrStmtParent parent) {

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

@@ -89,18 +89,13 @@ class CfgScope extends Element, @top_level_exprorstmt_parent {
 
 private class TAstNode = @callable or @control_flow_element;
 
-private Element getAChild(Element p) {
-  result = p.getAChild() or
-  result = p.(AssignOperation).getExpandedAssignment()
-}
-
 pragma[nomagic]
 private predicate astNode(Element e) {
   e = any(@top_level_exprorstmt_parent p | not p instanceof Attribute)
   or
   exists(Element parent |
     astNode(parent) and
-    e = getAChild(parent)
+    e = parent.getAChild()
   )
 }
 
@@ -486,6 +481,7 @@ module Expressions {
         result = qe.getChild(i)
       )
     or
+    // TODO: This can be fixed if the extracted indexes are fixed.
     e =
       any(Assignment a |
         // The left-hand side of an assignment is evaluated before the right-hand side
@@ -520,7 +516,6 @@ module Expressions {
       not this instanceof LogicalOrExpr and
       not this instanceof NullCoalescingExpr and
       not this instanceof ConditionalExpr and
-      not this instanceof AssignOperationWithExpandedAssignment and
       not this instanceof ConditionallyQualifiedExpr and
       not this instanceof ThrowExpr and
       not this instanceof ObjectCreation and
@@ -618,7 +613,7 @@ module Expressions {
       def.getExpr() = this and
       def.getTargetAccess().(WriteAccess) instanceof QualifiableExpr and
       not def instanceof AssignableDefinitions::OutRefDefinition and
-      not this instanceof AssignOperationWithExpandedAssignment
+      not def instanceof AssignableDefinitions::AssignOperationDefinition
     }
 
     /**
@@ -801,26 +796,6 @@ module Expressions {
     }
   }
 
-  /**
-   * An assignment operation that has an expanded version. We use the expanded
-   * version in the control flow graph in order to get better data flow / taint
-   * tracking.
-   */
-  private class AssignOperationWithExpandedAssignment extends ControlFlowTree instanceof AssignOperation
-  {
-    private Expr expanded;
-
-    AssignOperationWithExpandedAssignment() { expanded = this.getExpandedAssignment() }
-
-    final override predicate first(AstNode first) { first(expanded, first) }
-
-    final override predicate last(AstNode last, Completion c) { last(expanded, last, c) }
-
-    final override predicate propagatesAbnormal(AstNode child) { none() }
-
-    final override predicate succ(AstNode pred, AstNode succ, Completion c) { none() }
-  }
-
   /** A conditionally qualified expression. */
   private class ConditionallyQualifiedExpr extends PostOrderTree instanceof QualifiableExpr {
     private Expr qualifier;
@@ -1578,7 +1553,7 @@ module Statements {
   /** Gets a child of `cfe` that is in CFG scope `scope`. */
   pragma[noinline]
   private ControlFlowElement getAChildInScope(AstNode cfe, Callable scope) {
-    result = getAChild(cfe) and
+    result = cfe.getAChild() and
     scope = result.getEnclosingCallable()
   }
 

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

@@ -1579,6 +1579,16 @@ private module ArgumentNodes {
           scope = def.getExpr()
         )
       else scope = e2
+      or
+      e1.(Argument).isArgumentOf(e2, _) and
+      e2 instanceof DynamicMemberAccess and
+      exists(AssignableDefinitions::AssignOperationDefinition def |
+        def.getTargetAccess() = e2 and
+        def.getExpr() = e1
+      ) and
+      exactScope = false and
+      isSuccessor = false and
+      scope = e1
     }
   }
 

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

@@ -60,6 +60,9 @@ private class LocalTaintExprStepConfiguration extends ControlFlowReachabilityCon
       e1 = e2.(AddExpr).getAnOperand() and
       scope = e2
       or
+      e1 = e2.(AssignAddExpr).getAnOperand() and
+      scope = e2
+      or
       // A comparison expression where taint can flow from one of the
       // operands if the other operand is a constant value.
       exists(ComparisonTest ct, Expr other |

csharp/ql/lib/semmle/code/csharp/dataflow/internal/rangeanalysis/RangeUtils.qll

@@ -33,7 +33,9 @@ private module Impl {
   /** Holds if SSA definition `def` equals `e + delta`. */
   predicate ssaUpdateStep(ExplicitDefinition def, ExprNode e, int delta) {
     exists(ControlFlow::Node cfn | cfn = def.getControlFlowNode() |
-      e = cfn.(ExprNode::Assignment).getRValue() and delta = 0
+      e = cfn.(ExprNode::Assignment).getRValue() and
+      delta = 0 and
+      not cfn instanceof ExprNode::AssignOperation
       or
       e = cfn.(ExprNode::PostIncrExpr).getOperand() and delta = 1
       or
@@ -66,6 +68,12 @@ private module Impl {
       x.getIntValue() = delta
     )
     or
+    exists(ConstantIntegerExpr x |
+      e2.(ExprNode::AssignAddExpr).getLeftOperand() = e1 and
+      e2.(ExprNode::AssignAddExpr).getRightOperand() = x and
+      x.getIntValue() = delta
+    )
+    or
     exists(ConstantIntegerExpr x |
       e2.(ExprNode::SubExpr).getLeftOperand() = e1 and
       e2.(ExprNode::SubExpr).getRightOperand() = x and
@@ -230,6 +238,11 @@ module ExprNode {
     override CS::AssignExpr e;
   }
 
+  /** A compound assignment operation. */
+  class AssignOperation extends Assignment, BinaryOperation {
+    override CS::AssignOperation e;
+  }
+
   /** A unary operation. */
   class UnaryOperation extends ExprNode {
     override CS::UnaryOperation e;
@@ -327,6 +340,12 @@ module ExprNode {
     override TAddOp getOp() { any() }
   }
 
+  class AssignAddExpr extends AssignOperation {
+    override CS::AssignAddExpr e;
+
+    override TAddOp getOp() { any() }
+  }
+
   /** A subtraction operation. */
   class SubExpr extends BinaryOperation {
     override CS::SubExpr e;

csharp/ql/lib/semmle/code/csharp/dispatch/Dispatch.qll

@@ -202,11 +202,9 @@ private module Internal {
   private predicate isPotentialEventCall(
     AssignArithmeticOperation aao, DynamicMemberAccess dma, string name
   ) {
-    exists(DynamicOperatorCall doc, AssignExpr ae |
-      ae = aao.getExpandedAssignment() and
-      dma = ae.getLValue() and
-      doc = ae.getRValue()
-    |
+    aao instanceof DynamicOperatorCall and
+    dma = aao.getLeftOperand() and
+    (
       aao instanceof AssignAddExpr and
       name = "add_" + dma.getLateBoundTargetName()
       or