Skip to content

C#: Remove expanded assignments. - #21452

Merged
michaelnebel merged 18 commits into
github:mainfrom
michaelnebel:csharp/expandedassignment
Mar 27, 2026
Merged

C#: Remove expanded assignments.#21452
michaelnebel merged 18 commits into
github:mainfrom
michaelnebel:csharp/expandedassignment

Conversation

@michaelnebel

@michaelnebelmichaelnebel commented Mar 11, 2026

Copy link
Copy Markdown
Contributor

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 operatorcall, since it implicitly invokes the user-definedstaticoperator 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.

@michaelnebel
michaelnebelforce-pushed the csharp/expandedassignment branch from c94de85 to 1d8d712CompareMarch 11, 2026 12:47
Comment threadcsharp/ql/lib/semmle/code/csharp/ExprOrStmtParent.qll Fixed
@michaelnebel
michaelnebelforce-pushed the csharp/expandedassignment branch 9 times, most recently from abb75be to 5021ea9CompareMarch 16, 2026 10:00
@michaelnebelmichaelnebel changed the title C#: Cleanup expanded assignments.C#: Remove expanded assignments.Mar 16, 2026
@michaelnebel
michaelnebelforce-pushed the csharp/expandedassignment branch 13 times, most recently from 41fef59 to 07144c2CompareMarch 20, 2026 12:09
@michaelnebel

Copy link
Copy Markdown
ContributorAuthor

@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
michaelnebelforce-pushed the csharp/expandedassignment branch from 07144c2 to d2188ddCompareMarch 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
Copy Markdown
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 operationor
*(`AssignArithmeticOperation`),a bitwise assignment operation

@michaelnebel
michaelnebelforce-pushed the csharp/expandedassignment branch from 8f725cd to 028f2b8CompareMarch 24, 2026 14:32
@michaelnebel
michaelnebelforce-pushed the csharp/expandedassignment branch from 028f2b8 to 147ac37CompareMarch 25, 2026 15:16
@michaelnebel
michaelnebelforce-pushed the csharp/expandedassignment branch from 209771a to a402ce5CompareMarch 26, 2026 09:46
@michaelnebel

michaelnebel commented Mar 26, 2026

Copy link
Copy Markdown
ContributorAuthor

@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 ownerMarch 26, 2026 12:18

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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

@aschackmullaschackmull left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@michaelnebel

Copy link
Copy Markdown
ContributorAuthor

DCA looks good.

@michaelnebel
michaelnebel merged commit 73360ee into github:mainMar 27, 2026
26 of 29 checks passed
@michaelnebel
michaelnebel deleted the csharp/expandedassignment branch March 27, 2026 09:29
Sign up for freeto 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.

5 participants

@michaelnebel@aschackmull@hvitved@github-advanced-security