C#: Remove expanded assignments.

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

@@ -22,26 +22,12 @@ public static Assignment Create(ExpressionNodeInfo info)
 
         protected override void PopulateExpression(TextWriter trapFile)
         {
-            var operatorKind = OperatorKind;
-            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);
-            }
-            else
-            {
-                Create(Context, Syntax.Left, this, 1);
-                Create(Context, Syntax.Right, this, 0);
+            Create(Context, Syntax.Left, this, 0);
+            Create(Context, Syntax.Right, this, 1);
 
-                if (Kind == ExprKind.ADD_EVENT || Kind == ExprKind.REMOVE_EVENT)
-                {
-                    OperatorCall(trapFile, Syntax);
-                }
+            if (Kind != ExprKind.SIMPLE_ASSIGN)
+            {
+                OperatorCall(trapFile, Syntax);
             }
         }
 
@@ -108,56 +94,5 @@ private static ExprKind GetKind(Context cx, AssignmentExpressionSyntax syntax)
 
             return kind;
         }
-
-        /// <summary>
-        /// Gets the kind of this assignment operator (<code>null</code> if the
-        /// assignment is not an assignment operator). For example, the operator
-        /// kind of `*=` is `*`.
-        /// </summary>
-        private ExprKind? OperatorKind
-        {
-            get
-            {
-                var kind = Kind;
-                if (kind == ExprKind.REMOVE_EVENT || kind == ExprKind.ADD_EVENT || kind == ExprKind.SIMPLE_ASSIGN)
-                    return null;
-
-                if (CallType.AdjustKind(kind) == ExprKind.OPERATOR_INVOCATION)
-                    return ExprKind.OPERATOR_INVOCATION;
-
-                switch (kind)
-                {
-                    case ExprKind.ASSIGN_ADD:
-                        return ExprKind.ADD;
-                    case ExprKind.ASSIGN_AND:
-                        return ExprKind.BIT_AND;
-                    case ExprKind.ASSIGN_DIV:
-                        return ExprKind.DIV;
-                    case ExprKind.ASSIGN_LSHIFT:
-                        return ExprKind.LSHIFT;
-                    case ExprKind.ASSIGN_MUL:
-                        return ExprKind.MUL;
-                    case ExprKind.ASSIGN_OR:
-                        return ExprKind.BIT_OR;
-                    case ExprKind.ASSIGN_REM:
-                        return ExprKind.REM;
-                    case ExprKind.ASSIGN_RSHIFT:
-                        return ExprKind.RSHIFT;
-                    case ExprKind.ASSIGN_URSHIFT:
-                        return ExprKind.URSHIFT;
-                    case ExprKind.ASSIGN_SUB:
-                        return ExprKind.SUB;
-                    case ExprKind.ASSIGN_XOR:
-                        return ExprKind.BIT_XOR;
-                    case ExprKind.ASSIGN_COALESCE:
-                        return ExprKind.NULL_COALESCING;
-                    default:
-                        Context.ModelError(Syntax, $"Couldn't unfold assignment of type {kind}");
-                        return ExprKind.UNKNOWN;
-                }
-            }
-        }
-
-        public new CallType CallType => GetCallType(Context, Syntax);
     }
 }

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

@@ -83,30 +83,31 @@ protected override void PopulateExpression(TextWriter trapFile)
                 {
                     var assignmentInfo = new ExpressionNodeInfo(Context, init, this, child++).SetKind(ExprKind.SIMPLE_ASSIGN);
                     var assignmentEntity = new Expression(assignmentInfo);
-                    var typeInfoRight = Context.GetTypeInfo(assignment.Right);
-                    if (typeInfoRight.Type is null)
-                        // The type may be null for nested initializers such as
-                        // ```csharp
-                        // new ClassWithArrayField() { As = { [0] = a } }
-                        // ```
-                        // In this case we take the type from the assignment
-                        // `As = { [0] = a }` instead
-                        typeInfoRight = assignmentInfo.TypeInfo;
-                    CreateFromNode(new ExpressionNodeInfo(Context, assignment.Right, assignmentEntity, 0, typeInfoRight));
-
                     var target = Context.GetSymbolInfo(assignment.Left);
 
                     // If the target is null, then assume that this is an array initializer (of the form `[...] = ...`)
-
                     var access = target.Symbol is null ?
-                        new Expression(new ExpressionNodeInfo(Context, assignment.Left, assignmentEntity, 1).SetKind(ExprKind.ARRAY_ACCESS)) :
-                        Access.Create(new ExpressionNodeInfo(Context, assignment.Left, assignmentEntity, 1), target.Symbol, false, Context.CreateEntity(target.Symbol));
+                        new Expression(new ExpressionNodeInfo(Context, assignment.Left, assignmentEntity, 0).SetKind(ExprKind.ARRAY_ACCESS)) :
+                        Access.Create(new ExpressionNodeInfo(Context, assignment.Left, assignmentEntity, 0), target.Symbol, false, Context.CreateEntity(target.Symbol));
 
                     if (assignment.Left is ImplicitElementAccessSyntax iea)
                     {
                         // An array/indexer initializer of the form `[...] = ...`
                         access.PopulateArguments(trapFile, iea.ArgumentList.Arguments, 0);
                     }
+
+                    var typeInfoRight = Context.GetTypeInfo(assignment.Right);
+                    if (typeInfoRight.Type is null)
+                    {
+                        // The type may be null for nested initializers such as
+                        // ```csharp
+                        // new ClassWithArrayField() { As = { [0] = a } }
+                        // ```
+                        // In this case we take the type from the assignment
+                        // `As = { [0] = a }` instead
+                        typeInfoRight = assignmentInfo.TypeInfo;
+                    }
+                    CreateFromNode(new ExpressionNodeInfo(Context, assignment.Right, assignmentEntity, 1, typeInfoRight));
                 }
                 else
                 {

csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/ObjectCreation/AnonymousObjectCreation.cs

@@ -41,11 +41,11 @@ protected override void PopulateExpression(TextWriter trapFile)
                 var loc = Context.CreateLocation(init.GetLocation());
 
                 var assignment = new Expression(new ExpressionInfo(Context, type, loc, ExprKind.SIMPLE_ASSIGN, objectInitializer, child++, isCompilerGenerated: false, null));
-                Create(Context, init.Expression, assignment, 0);
                 Property.Create(Context, property);
-
-                var access = new Expression(new ExpressionInfo(Context, type, loc, ExprKind.PROPERTY_ACCESS, assignment, 1, isCompilerGenerated: false, null));
+                var access = new Expression(new ExpressionInfo(Context, type, loc, ExprKind.PROPERTY_ACCESS, assignment, 0, isCompilerGenerated: false, null));
                 trapFile.expr_access(access, propEntity);
+
+                Create(Context, init.Expression, assignment, 1);
             }
         }
     }

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

@@ -94,12 +94,12 @@ protected Expression DeclareRangeVariable(Context cx, IExpressionParentEntity pa
                     child
                     );
 
-                Expression.Create(cx, Expr, decl, 0);
-
                 var nameLoc = cx.CreateLocation(name.GetLocation());
-                var access = new Expression(new ExpressionInfo(cx, type, nameLoc, ExprKind.LOCAL_VARIABLE_ACCESS, decl, 1, isCompilerGenerated: false, null));
+                var access = new Expression(new ExpressionInfo(cx, type, nameLoc, ExprKind.LOCAL_VARIABLE_ACCESS, decl, 0, isCompilerGenerated: false, null));
                 cx.TrapWriter.Writer.expr_access(access, LocalVariable.Create(cx, variableSymbol));
 
+                Expression.Create(cx, Expr, decl, 1);
+
                 return decl;
             }
 

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

@@ -176,11 +176,11 @@ public static VariableDeclaration CreateDeclarator(Context cx, VariableDeclarato
 
                 if (d.Initializer is not null)
                 {
-                    Create(cx, d.Initializer.Value, ret, 0);
-
                     // Create an access
-                    var access = new Expression(new ExpressionInfo(cx, type, localVar.Location, ExprKind.LOCAL_VARIABLE_ACCESS, ret, 1, isCompilerGenerated: false, null));
+                    var access = new Expression(new ExpressionInfo(cx, type, localVar.Location, ExprKind.LOCAL_VARIABLE_ACCESS, ret, 0, isCompilerGenerated: false, null));
                     cx.TrapWriter.Writer.expr_access(access, localVar);
+
+                    Create(cx, d.Initializer.Value, ret, 1);
                 }
 
                 if (d.Parent is VariableDeclarationSyntax decl)

csharp/extractor/Semmle.Extraction.CSharp/Entities/Field.cs

@@ -116,9 +116,9 @@ private Expression AddInitializerAssignment(TextWriter trapFile, ExpressionSynta
         {
             var type = Symbol.GetAnnotatedType();
             var simpleAssignExpr = new Expression(new ExpressionInfo(Context, type, loc, ExprKind.SIMPLE_ASSIGN, this, child++, isCompilerGenerated: true, constValue));
-            Expression.CreateFromNode(new ExpressionNodeInfo(Context, initializer, simpleAssignExpr, 0));
-            var access = new Expression(new ExpressionInfo(Context, type, Location, ExprKind.FIELD_ACCESS, simpleAssignExpr, 1, isCompilerGenerated: true, constValue));
+            var access = new Expression(new ExpressionInfo(Context, type, Location, ExprKind.FIELD_ACCESS, simpleAssignExpr, 0, isCompilerGenerated: true, constValue));
             trapFile.expr_access(access, this);
+            Expression.CreateFromNode(new ExpressionNodeInfo(Context, initializer, simpleAssignExpr, 1));
             return access;
         }
 

csharp/extractor/Semmle.Extraction.CSharp/Entities/Property.cs

@@ -94,9 +94,9 @@ public override void Populate(TextWriter trapFile)
                         var loc = Context.CreateLocation(initializer!.GetLocation());
                         var annotatedType = AnnotatedTypeSymbol.CreateNotAnnotated(Symbol.Type);
                         var simpleAssignExpr = new Expression(new ExpressionInfo(Context, annotatedType, loc, ExprKind.SIMPLE_ASSIGN, this, child++, isCompilerGenerated: true, null));
-                        Expression.CreateFromNode(new ExpressionNodeInfo(Context, initializer.Value, simpleAssignExpr, 0));
-                        var access = new Expression(new ExpressionInfo(Context, annotatedType, Location, ExprKind.PROPERTY_ACCESS, simpleAssignExpr, 1, isCompilerGenerated: true, null));
+                        var access = new Expression(new ExpressionInfo(Context, annotatedType, Location, ExprKind.PROPERTY_ACCESS, simpleAssignExpr, 0, isCompilerGenerated: true, null));
                         trapFile.expr_access(access, this);
+                        Expression.CreateFromNode(new ExpressionNodeInfo(Context, initializer.Value, simpleAssignExpr, 1));
                         if (!Symbol.IsStatic)
                         {
                             This.CreateImplicit(Context, Symbol.ContainingType, Location, access, -1);

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,8 +379,10 @@
       or
       exists(Assignment ass | ac = ass.getLValue() |
         result = ass.getRValue() and
-        not ass.(AssignOperation).hasExpandedAssignment()
+        not ass instanceof AssignOperation
       )
+      or
+      exists(AssignOperation ao | ac = ao.getLeftOperand() | result = ao)
     }
   }
 
@@ -388,8 +400,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 +524,7 @@
 
     override Expr getSource() {
       result = a.getRValue() and
-      not a instanceof AssignOperation
+      not a instanceof AddOrRemoveEventExpr
     }
 
     override string toString() { result = a.toString() }
@@ -735,4 +748,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/Property.qll

@@ -226,7 +226,7 @@ class Property extends DeclarationWithGetSetAccessors, @property {
    * }
    * ```
    */
-  Expr getInitializer() { result = this.getChildExpr(1).getChildExpr(0) }
+  Expr getInitializer() { result = this.getChildExpr(1).getChildExpr(1) }
 
   /**
    * Holds if this property has an initial value. For example, the initial