C#: Remove expanded assignments. by michaelnebel · Pull Request #21452 · github/codeql

michaelnebel · 2026-03-11T12:12:21Z

This PR has two goals, primarily to avoid making multiple upgrade/downgrade scripts:

Remove expanded assignments. This aligns our implementation with Java and should make adoption of the shared CFG library easier. It also simplifies expression handling and avoids caching a large predicate. Previously, when the extractor encountered a += b, it would synthesize a = a + b.
Swap the left- and right-hand-side child indices for assignments. This lets us remove several hacks/rotations currently used in the code.

Review on a commit-by-commit basis is encouraged.

Implementation notes

The implementation now “correctly” classifies a += b (e.g., when a and b are int) as an operator call, since it implicitly invokes the user-defined static operator on Int32.
The only compound assignment that does not call an operator is ??= as the null-coalescing operator is a built-in short-circuit like operation.
Added a new set of classes to represent expressions involving a specific operation (e.g., AddOperation, which can represent either a + b or a += b).

DCA notes

Performance appears unchanged.
The missing result for cs/unsynchronized-getter is due to removal of a false positive: the query did not properly account for expanded assignments.
The missing result for cs/web/xss looks acceptable: one alert is removed, but it is essentially a shorter path to the same underlying alert (which is here)
The new results for cs/useless-upcast look acceptable.
Remaining differences appear consistent with normal DCA background noise.

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

-        )
-    else expr_parent(child, i, parent)
-  }
+  deprecated predicate expr_parent_adjusted(Expr child, int i, ControlFlowElement parent) { none() }


michaelnebel · 2026-03-20T13:05:19Z

@hvitved : I would really appreciate a review of this PR even though it is still in draft mode. The PR still needs upgrade/downgrade scripts and more DCA runs.

…ignments.

…se operations are operator calls.

…o extend OperatorCall and add AssignOperation classes.

…ct expanded assignments.

…nments (those that was previously covered by expanded assignments).

…ove hack that rotates children of assignments.

aschackmull · 2026-03-23T10:56:50Z

csharp/ql/lib/semmle/code/csharp/exprs/Assignment.qll

 * An assignment operation. Either an arithmetic assignment operation
- * (`AssignArithmeticOperation`), a bitwise assignment operation
- * (`AssignBitwiseOperation`), or an event assignment (`AddOrRemoveEventExpr`).
+ * (`AssignArithmeticOperation`), a bitwise assignment operation or


Spurious "or"

Suggested change

* (`AssignArithmeticOperation`), a bitwise assignment operation or

* (`AssignArithmeticOperation`), a bitwise assignment operation

aschackmull · 2026-03-23T11:03:25Z

csharp/ql/lib/semmle/code/csharp/exprs/Assignment.qll

+}

+/**
+ * An assignment operation that corresponds to an operator call, for example `x += y` corresponds to `x = x + y`.


This reads somewhat off to me. Did you mean something like this instead?

Suggested change

* An assignment operation that corresponds to an operator call, for example `x += y` corresponds to `x = x + y`.

* An assignment operation that corresponds to an operator call, for example `x += y` corresponds to a call to `+=`.

Some further elaboration on this piece of qldoc might also be nice to clarify exactly which compound assignments are considered OperatorCalls.

aschackmull · 2026-03-23T11:05:26Z

csharp/ql/lib/semmle/code/csharp/exprs/Assignment.qll

+/**
+ * An assignment operation that corresponds to an operator call, for example `x += y` corresponds to `x = x + y`.
+ */
+class AssignCallOperation extends AssignOperation, OperatorCall, @assign_op_call_expr {


Now that most AssignOperations extend OperatorCall, then the qldoc for OperatorCall needs a tweak as it currently claims to only cover calls to user-defined operators.

Yes, we should consider changing the wording as I am not sure what a non user-defined operator is (unless it is a real built-in like &&). The operator used in a += b (when a and b are of type int) is a user-defined operator and it is declared here.

aschackmull · 2026-03-23T11:06:14Z

csharp/ql/lib/semmle/code/csharp/exprs/Assignment.qll

+ * An assignment operation that corresponds to an operator call, for example `x += y` corresponds to `x = x + y`.
+ */
+class AssignCallOperation extends AssignOperation, OperatorCall, @assign_op_call_expr {
  override string toString() { result = "... " + this.getOperator() + " ..." }


This toString can be simplified.

Suggested change

override string toString() { result = "... " + this.getOperator() + " ..." }

override string toString() { result = AssignOperation.super.toString() }

aschackmull · 2026-03-23T12:03:40Z

csharp/ql/lib/semmle/code/csharp/exprs/Operation.qll

+import Expr
+
+/** A binary operation that involves a null-coalescing operation. */
+abstract private class NullCoalescingOperationImpl extends BinaryOperation { }


I believe all the classes in this file break some new ground wrt the AST class hierarchy design, so perhaps ought to be considered in a slightly broader bike-shedding session. Or is there something similar in other languages that I'm not aware of?
Regardless, I'm not a big fan of all the Add-prefixed names. Sure they're private, but they're still not very nice and they overload the meaning of "Add" in a context that already references "Add". Also, it's unfortunate that e.g. AddExpr and AssignAddExpr are not subclasses of AddOperation - that would be more natural. And if the latter is fixed, then I don't think we need all those addition-to-abstract-class classes that have such awkward names, which would address the former point as well.

The other option that I considered was extending the DB-scheme with @add_operation = @add_expr | @assign_add_expr; and similar for the other operations, then the QL part will look more "natural". Would you prefer such a solution instead?

aschackmull · 2026-03-23T12:05:28Z

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

-   * ```
+   * Use `expr_parent` instead.
   */
  cached


May as well get rid of cached.

We can't, it's a public predicate in a cached module - so it has to be cached as well.

aschackmull · 2026-03-23T13:24:58Z

Besides the nits, code changes generally LGTM.

hvitved

Overall LGTM; no further comments than what @aschackmull has already pointed out, thanks for making this change.

github-actions bot added the C# label Mar 11, 2026

michaelnebel force-pushed the csharp/expandedassignment branch from c94de85 to 1d8d712 Compare March 11, 2026 12:47

github-advanced-security bot found potential problems Mar 11, 2026

View reviewed changes

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

)

else expr_parent(child, i, parent)

}

deprecated predicate expr_parent_adjusted(Expr child, int i, ControlFlowElement parent) { none() }

michaelnebel force-pushed the csharp/expandedassignment branch 9 times, most recently from abb75be to 5021ea9 Compare March 16, 2026 10:00

michaelnebel changed the title ~~C#: Cleanup expanded assignments.~~ C#: Remove expanded assignments. Mar 16, 2026

michaelnebel force-pushed the csharp/expandedassignment branch 13 times, most recently from 41fef59 to 07144c2 Compare March 20, 2026 12:09

michaelnebel requested a review from hvitved March 20, 2026 13:18

michaelnebel added 2 commits March 23, 2026 10:08

C#: Don't extract expanded assignments and swap child indices for ass…

b23f1fd

…ignments.

C#: Update the DB scheme to reflect that assign arithmetic- and bitwi…

4dba48c

…se operations are operator calls.

michaelnebel added 7 commits March 23, 2026 10:08

C#: Update the child indices for assignments, update Assign classes t…

dbe90b0

…o extend OperatorCall and add AssignOperation classes.

C#: Deprecate the expanded assignment predicate as we no longer extra…

35dac7c

…ct expanded assignments.

C#: Cleaup expanded assignments from the dispatch logic.

4b3319c

C#: Introduce a new kind of assignable definitions for compound assig…

7a9d8b6

…nments (those that was previously covered by expanded assignments).

C#: Update the CFG implementation based on the new operations and rem…

97a487e

…ove hack that rotates children of assignments.

C#: Upgrade libraries and queries to use the new Operation classes.

d2ee9ef

C#: Add/update tests and expected output.

d2188dd

michaelnebel force-pushed the csharp/expandedassignment branch from 07144c2 to d2188dd Compare March 23, 2026 09:09

aschackmull reviewed Mar 23, 2026

View reviewed changes

hvitved reviewed Mar 23, 2026

View reviewed changes

	* (`AssignArithmeticOperation`), a bitwise assignment operation or
	* (`AssignArithmeticOperation`), a bitwise assignment operation

	* An assignment operation that corresponds to an operator call, for example `x += y` corresponds to `x = x + y`.
	* An assignment operation that corresponds to an operator call, for example `x += y` corresponds to a call to `+=`.

	override string toString() { result = "... " + this.getOperator() + " ..." }
	override string toString() { result = AssignOperation.super.toString() }

Conversation

michaelnebel commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelnebel commented Mar 20, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelnebel Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aschackmull commented Mar 23, 2026

Uh oh!

hvitved left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

michaelnebel commented Mar 11, 2026 •

edited

Loading

michaelnebel Mar 23, 2026 •

edited

Loading