Skip to content

C#: Remove expanded assignments.#21452

Open
michaelnebel wants to merge 18 commits intogithub:mainfrom
michaelnebel:csharp/expandedassignment
Open

C#: Remove expanded assignments.#21452
michaelnebel wants to merge 18 commits intogithub:mainfrom
michaelnebel:csharp/expandedassignment

Conversation

@michaelnebel
Copy link
Contributor

@michaelnebel michaelnebel commented Mar 11, 2026

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.

@github-actions github-actions bot added the C# label Mar 11, 2026
@michaelnebel michaelnebel force-pushed the csharp/expandedassignment branch from c94de85 to 1d8d712 Compare March 11, 2026 12:47
@michaelnebel michaelnebel force-pushed the csharp/expandedassignment branch 9 times, most recently from abb75be to 5021ea9 Compare March 16, 2026 10:00
@michaelnebel michaelnebel changed the title C#: Cleanup expanded assignments. C#: Remove expanded assignments. Mar 16, 2026
@michaelnebel michaelnebel force-pushed the csharp/expandedassignment branch 13 times, most recently from 41fef59 to 07144c2 Compare March 20, 2026 12:09
@michaelnebel
Copy link
Contributor Author

@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.

@michaelnebel michaelnebel requested a review from hvitved March 20, 2026 13:18
@michaelnebel michaelnebel force-pushed the csharp/expandedassignment branch from 07144c2 to d2188dd Compare March 23, 2026 09:09
* 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spurious "or"

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

@michaelnebel michaelnebel force-pushed the csharp/expandedassignment branch from 8f725cd to 028f2b8 Compare March 24, 2026 14:32
@michaelnebel michaelnebel force-pushed the csharp/expandedassignment branch from 028f2b8 to 147ac37 Compare March 25, 2026 15:16
@michaelnebel michaelnebel force-pushed the csharp/expandedassignment branch from 209771a to a402ce5 Compare March 26, 2026 09:46
@michaelnebel
Copy link
Contributor Author

michaelnebel commented Mar 26, 2026

@aschackmull @hvitved : This PR is now ready for review.

  • The review comments have been addressed.
  • Upgrade/downgrade scripts have been added. I have decided to only "claim" partial correctness for these scripts. The primary focus has been to ensure that the expressions and parent/child relation are correct - there might be dangling/missing references in other relations. I have done some minor testing locally.
    • The upgrade script contracts the parent/child relation for expressions and deletes the synthetic (expanded assignment related) expressions.
    • The downgrade script re-introduce the synthetic expanded assignment, by adding new expressions entities and adjust the parent/child relation accordingly.
  • DCA identified a performance issue in the cs/coalesce-of-identical-expressions, which has been fixed.

@michaelnebel michaelnebel marked this pull request as ready for review March 26, 2026 12:18
@michaelnebel michaelnebel requested a review from a team as a code owner March 26, 2026 12:18
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR removes extractor-synthesized expanded forms of compound assignments and rotates assignment child indices, introducing new “operation” classes (e.g., AddOperation, NullCoalescingOperation) to represent both regular and compound assignment operator forms consistently.

Changes:

  • Stop synthesizing expanded assignments (e.g., a += ba = a + b) and update CFG/dataflow expectations accordingly.
  • Swap assignment child indices (LHS/RHS) and update extractor + QL libraries/queries to match.
  • Add new operation kinds/classes to unify x OP y and x OP= y modeling (plus ??/??= via NullCoalescingOperation).

Reviewed changes

Copilot reviewed 99 out of 99 changed files in this pull request and generated no comments.

Show a summary per file
File Description
csharp/ql/test/query-tests/WriteOnlyContainer/WriteOnlyContainer.cs Adds coverage for ??= returning a non-write-only container.
csharp/ql/test/query-tests/Language Abuse/UselessNullCoalescingExpression/UselessNullCoalescingExpression.cs Adds ??= cases (bad/good) for the query test.
csharp/ql/test/query-tests/Language Abuse/UselessNullCoalescingExpression/UselessNullCoalescingExpression.expected Updates expected results to include ??= findings.
csharp/ql/test/query-tests/Dead Code/DeadStoreOfLocal/DeadStoreOfLocal.expected Updates expected output to reflect compound assignment nodes (... += ...).
csharp/ql/test/query-tests/Concurrency/SynchSetUnsynchGet/SynchSetUnsynchGet.cs Adds locked getter/setter example using ??=.
csharp/ql/test/library-tests/structuralcomparison/structuralComparison.expected Updates structural comparison expectations for rotated assignment child ordering.
csharp/ql/test/library-tests/dynamic/PrintAst.expected Updates AST print expectations to represent dynamic operator calls as OperatorCall.
csharp/ql/test/library-tests/dynamic/DynamicOperatorCall.expected Updates expectations for compound assignment operator calls on dynamic.
csharp/ql/test/library-tests/dispatch/viableCallable.expected Updates expected callability results; includes ... += ... viable callable.
csharp/ql/test/library-tests/dispatch/ViableCallable.cs Adds comment clarifying viable callable for x += 42.
csharp/ql/test/library-tests/dispatch/GetADynamicTarget.expected Adjusts dynamic target expectations due to line/shape changes.
csharp/ql/test/library-tests/dispatch/CallGraph.expected Updates call graph expectations after CFG/assignment modeling changes.
csharp/ql/test/library-tests/dispatch/CallContext.expected Updates call-context expectations after node/line shifts.
csharp/ql/test/library-tests/dataflow/ssa/SsaExplicitDef.expected Updates SSA expectations for compound assignment definitions.
csharp/ql/test/library-tests/dataflow/ssa/SsaDefElement.expected Updates SSA def element expectations for compound assignments.
csharp/ql/test/library-tests/dataflow/ssa/DefAdjacentRead.expected Updates adjacent def/read expectations for compound assignments.
csharp/ql/test/library-tests/dataflow/signanalysis/SignAnalysis.expected Updates sign analysis expectations to use ... += ... nodes.
csharp/ql/test/library-tests/dataflow/nullcoalescing/nullCoalescingFlow.ql Adds a new path-problem test query for null-coalescing flows.
csharp/ql/test/library-tests/dataflow/nullcoalescing/nullCoalescingFlow.expected Adds expected path-graph output for null-coalescing flow tests.
csharp/ql/test/library-tests/dataflow/nullcoalescing/NullCoalescing.cs Introduces test code covering both ?? and ??= flows.
csharp/ql/test/library-tests/dataflow/modulusanalysis/ModulusAnalysis.expected Updates modulus analysis expectations for compound assignment operations.
csharp/ql/test/library-tests/dataflow/local/TaintTrackingStep.expected Updates taint-tracking step expectations around compound assignments.
csharp/ql/test/library-tests/dataflow/local/DataFlowStep.expected Updates local dataflow step expectations around compound assignments.
csharp/ql/test/library-tests/csharp8/NullCoalescingControlFlow.expected Updates CFG expectations for ??= as a branching expression.
csharp/ql/test/library-tests/csharp8/NullCoalescingAssignment.ql Removes dependency on expanded assignment API from the test.
csharp/ql/test/library-tests/csharp8/NullCoalescingAssignment.expected Updates expected output after removing expanded assignment.
csharp/ql/test/library-tests/csharp11/operators.expected Fixes class name for unsigned right shift assign expr in expectations.
csharp/ql/test/library-tests/csharp11/PrintAst.expected Updates AST print expectation for AssignUnsignedRightShiftExpr.
csharp/ql/test/library-tests/controlflow/graph/NodeGraph.expected Updates CFG node graph expectations after assignment modeling changes.
csharp/ql/test/library-tests/controlflow/graph/EntryElement.expected Updates entry-element expectations after assignment modeling changes.
csharp/ql/test/library-tests/controlflow/graph/EnclosingCallable.expected Updates enclosing callable expectations after CFG/assignment changes.
csharp/ql/test/library-tests/controlflow/graph/Condition.expected Updates condition expectations after CFG/assignment changes.
csharp/ql/test/library-tests/controlflow/graph/BasicBlock.expected Updates basic block expectations after CFG/assignment changes.
csharp/ql/test/library-tests/assignments/AssignOperationExpanded.ql Removes a test relying on expanded assignments.
csharp/ql/test/library-tests/assignments/AssignOperationExpanded.expected Removes expected output for expanded assignment test.
csharp/ql/test/library-tests/arguments/parameterGetArguments.expected Updates expected argument extraction for compound assignments.
csharp/ql/test/library-tests/arguments/argumentType.expected Updates expected argument typing results after AST changes.
csharp/ql/test/library-tests/arguments/argumentByParameter.expected Updates expected parameter-to-argument mapping for compound assigns.
csharp/ql/test/library-tests/arguments/argumentByName.expected Updates expected named-argument mapping for compound assignments.
csharp/ql/src/experimental/Security Features/CWE-759/HashWithoutSalt.ql Switches string concat step to AddOperation.
csharp/ql/src/experimental/CWE-918/RequestForgery.qll Switches string concat step to AddOperation.
csharp/ql/src/Security Features/InsecureRandomness.ql Updates assignment-operation handling after removal of expanded assignments.
csharp/ql/src/Security Features/CWE-119/LocalUnvalidatedArithmetic.ql Switches pointer arithmetic matching to AddOperation.
csharp/ql/src/Performance/StringConcatenationInLoop.ql Removes workaround for expanded assignments.
csharp/ql/src/Likely Bugs/PossibleLossOfPrecision.ql Generalizes arithmetic conversions to new *Operation classes.
csharp/ql/src/Likely Bugs/DangerousNonShortCircuitLogic.ql Removes expanded-assignment special casing.
csharp/ql/src/Likely Bugs/Collections/WriteOnlyContainer.ql Broadens initialization recognition to rotated assignment/decl shapes.
csharp/ql/src/Language Abuse/UselessNullCoalescingExpression.ql Refactors to use NullCoalescingOperation (covers ?? and ??=).
csharp/ql/src/Dead Code/DeadStoreOfLocal.ql Treats compound assignment definitions as relevant definitions.
csharp/ql/src/Complexity/ComplexCondition.ql Attempts to include ??= in logical-operation counting.
csharp/ql/src/Bad Practices/Control-Flow/ConstantCondition.ql Switches null-check/whitelist logic to NullCoalescingOperation.
csharp/ql/lib/upgrades/e73ca2c93df8aae162f1704edc4817a5cb330529/upgrade.properties Adds upgrade scripts for new operation kinds + assignment rotation.
csharp/ql/lib/upgrades/e73ca2c93df8aae162f1704edc4817a5cb330529/assignments.ql Upgrade logic to remove expanded assignment expressions + rotate children.
csharp/ql/lib/semmlecode.csharp.dbscheme Introduces operation kinds and treats assign-op-call expressions as calls.
csharp/ql/lib/semmle/code/csharp/metrics/Complexity.qll Counts NullCoalescingOperation as a branching expression.
csharp/ql/lib/semmle/code/csharp/frameworks/system/Xml.qll Switches bitwise-or operand matching to BitwiseOrOperation.
csharp/ql/lib/semmle/code/csharp/exprs/Operation.qll Adds new operation classes (AddOperation, NullCoalescingOperation, etc.).
csharp/ql/lib/semmle/code/csharp/exprs/LogicalOperation.qll Makes NullCoalescingExpr also a NullCoalescingOperation.
csharp/ql/lib/semmle/code/csharp/exprs/Expr.qll Removes expanded-assignment implicitness; updates local decl child indices.
csharp/ql/lib/semmle/code/csharp/exprs/Dynamic.qll Simplifies DynamicOperatorCall (behavior via OperatorCall.toString).
csharp/ql/lib/semmle/code/csharp/exprs/Call.qll Updates OperatorCall to cover assignment-operator calls via dbscheme union.
csharp/ql/lib/semmle/code/csharp/exprs/BitwiseOperation.qll Connects bitwise expr classes to new *Operation superclasses.
csharp/ql/lib/semmle/code/csharp/exprs/Assignment.qll Deprecates expanded assignment API; models assign-op expressions as calls.
csharp/ql/lib/semmle/code/csharp/exprs/ArithmeticOperation.qll Connects arithmetic expr classes to new *Operation superclasses.
csharp/ql/lib/semmle/code/csharp/dispatch/Dispatch.qll Updates dynamic event-call heuristic for new assignment modeling.
csharp/ql/lib/semmle/code/csharp/dataflow/internal/rangeanalysis/SsaUtils.qll Switches SSA delta logic to AddOperation/SubOperation.
csharp/ql/lib/semmle/code/csharp/dataflow/internal/rangeanalysis/SignAnalysisSpecific.qll Aligns expression-node types + supports assign-operation definitions.
csharp/ql/lib/semmle/code/csharp/dataflow/internal/rangeanalysis/RangeUtils.qll Updates SSA update steps and renames ExprNode operation classes.
csharp/ql/lib/semmle/code/csharp/dataflow/internal/rangeanalysis/ModulusAnalysisSpecific.qll Switches operation-node classes to new *Operation names.
csharp/ql/lib/semmle/code/csharp/dataflow/internal/TaintTrackingPrivate.qll Taint propagation now treats AddOperation as the concat step.
csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll Uses NullCoalescingOperation for local flow steps / barriers.
csharp/ql/lib/semmle/code/csharp/dataflow/Nullness.qll Uses NullCoalescingOperation for maybe-null expression modeling.
csharp/ql/lib/semmle/code/csharp/controlflow/internal/Splitting.qll Uses NullCoalescingOperation in CFG completion splitting.
csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll Removes expanded-assignment CFG hack; adapts to new assignment ordering.
csharp/ql/lib/semmle/code/csharp/controlflow/internal/Completion.qll Switches exception completion triggers to *Operation classes.
csharp/ql/lib/semmle/code/csharp/controlflow/Guards.qll Updates guard logic to account for *Operation (incl. compound assigns).
csharp/ql/lib/semmle/code/csharp/commons/Strings.qll Adds implicit-to-string handling for AssignAddExpr string concatenation.
csharp/ql/lib/semmle/code/csharp/Variable.qll Updates field initializer indexing due to assignment child rotation.
csharp/ql/lib/semmle/code/csharp/Property.qll Updates property initializer indexing due to assignment child rotation.
csharp/ql/lib/semmle/code/csharp/ExprOrStmtParent.qll Removes expanded-assignment adjusted parenting; uses rotated expr_parent.
csharp/ql/lib/semmle/code/csharp/Assignable.qll Adds assignment-operation definitions for correct def/use modeling.
csharp/ql/lib/experimental/code/csharp/Cryptography/NonCryptographicHashes.qll Uses AddOperation for hash-shape detection logic.
csharp/ql/lib/change-notes/2026-03-26-expanded-assignments.md Adds change note about removal of expanded compound assignments.
csharp/ql/campaigns/Solorigate/src/ModifiedFnvFunctionDetection.ql Simplifies XOR detection using BitwiseXorOperation.
csharp/extractor/Semmle.Extraction.CSharp/Entities/Property.cs Rotates property initializer assignment children to match new ordering.
csharp/extractor/Semmle.Extraction.CSharp/Entities/Field.cs Rotates field initializer assignment children to match new ordering.
csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/VariableDeclaration.cs Rotates local variable decl/access/initializer child indices.
csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/Query.cs Rotates query range-variable decl child indices.
csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/ObjectCreation/AnonymousObjectCreation.cs Rotates anonymous object init assignment children.
csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/Initializer.cs Rotates initializer assignment children; reorders RHS extraction accordingly.
csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/Assignment.cs Removes expanded assignment extraction and rotates assignment children.
csharp/downgrades/19b8cc3e2dc768d4cbc03d6e3773b709bbebd036/upgrade.properties Adds downgrade scripts to re-introduce expanded assignments + rotate back.
csharp/downgrades/19b8cc3e2dc768d4cbc03d6e3773b709bbebd036/assignments.ql Downgrade logic to synthesize expanded nodes and restore old parenting.
Comments suppressed due to low confidence (7)

csharp/ql/lib/semmlecode.csharp.dbscheme:1

  • The dbscheme definition for @assign_op_call_expr is missing a terminating semicolon, which will make the dbscheme invalid. Add ; at the end of the @assign_op_call_expr = ... line (and ensure formatting matches surrounding productions).
    csharp/ql/src/Complexity/ComplexCondition.ql:1
  • logicalParent is typed to RelevantOperations, but the transitive closure is invoked with RelevantBinaryOperations op. Since RelevantBinaryOperations is not a subtype of RelevantOperations, this will not type-check (or will fail to traverse as intended). Consider making RelevantBinaryOperations extend RelevantOperations, or change logicalParent/the count to use a single consistent type hierarchy for traversal and counting.
    csharp/ql/src/Complexity/ComplexCondition.ql:1
  • logicalParent is typed to RelevantOperations, but the transitive closure is invoked with RelevantBinaryOperations op. Since RelevantBinaryOperations is not a subtype of RelevantOperations, this will not type-check (or will fail to traverse as intended). Consider making RelevantBinaryOperations extend RelevantOperations, or change logicalParent/the count to use a single consistent type hierarchy for traversal and counting.
    csharp/ql/src/Complexity/ComplexCondition.ql:1
  • logicalParent is typed to RelevantOperations, but the transitive closure is invoked with RelevantBinaryOperations op. Since RelevantBinaryOperations is not a subtype of RelevantOperations, this will not type-check (or will fail to traverse as intended). Consider making RelevantBinaryOperations extend RelevantOperations, or change logicalParent/the count to use a single consistent type hierarchy for traversal and counting.
    csharp/ql/src/Complexity/ComplexCondition.ql:1
  • logicalParent is typed to RelevantOperations, but the transitive closure is invoked with RelevantBinaryOperations op. Since RelevantBinaryOperations is not a subtype of RelevantOperations, this will not type-check (or will fail to traverse as intended). Consider making RelevantBinaryOperations extend RelevantOperations, or change logicalParent/the count to use a single consistent type hierarchy for traversal and counting.
    csharp/ql/src/Security Features/InsecureRandomness.ql:1
  • The exists clause uses multiple | separators, which is easy to misread and makes operator precedence less obvious. Refactor into a single-condition form (for example using and plus a parenthesized or block) to reduce the risk of subtle logic mistakes during future edits.
    csharp/ql/lib/upgrades/e73ca2c93df8aae162f1704edc4817a5cb330529/assignments.ql:1
  • Correct spelling of 'seperatly' to 'separately'.

Comment on lines +10 to +12
TAssignment(Expr e) or
TLhs(Expr e) or
TRhs(Expr e)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These could be CompoundAssignmentExpr instead of Expr, right? I don't know if it matters - it probably doesn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! I will change it.

aschackmull
aschackmull previously approved these changes Mar 26, 2026
Copy link
Contributor

@aschackmull aschackmull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants