C#: Remove expanded assignments.#21452
Merged
michaelnebel merged 18 commits intoMar 27, 2026
Merged
Conversation
c94de85 to
1d8d712
Compare
abb75be to
5021ea9
Compare
41fef59 to
07144c2
Compare
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. |
07144c2 to
d2188dd
Compare
aschackmull
reviewed
Mar 23, 2026
| * 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 |
Contributor
There was a problem hiding this comment.
Spurious "or"
Suggested change
| * (`AssignArithmeticOperation`), a bitwise assignment operation or | |
| * (`AssignArithmeticOperation`), a bitwise assignment operation |
8f725cd to
028f2b8
Compare
028f2b8 to
147ac37
Compare
209771a to
a402ce5
Compare
Contributor
Author
|
@aschackmull @hvitved : This PR is now ready for review.
|
Contributor
There was a problem hiding this comment.
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 += b→a = 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 yandx OP= ymodeling (plus??/??=viaNullCoalescingOperation).
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_expris 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 logicalParentis typed toRelevantOperations, but the transitive closure is invoked withRelevantBinaryOperations op. SinceRelevantBinaryOperationsis not a subtype ofRelevantOperations, this will not type-check (or will fail to traverse as intended). Consider makingRelevantBinaryOperationsextendRelevantOperations, or changelogicalParent/thecountto use a single consistent type hierarchy for traversal and counting.
csharp/ql/src/Complexity/ComplexCondition.ql:1logicalParentis typed toRelevantOperations, but the transitive closure is invoked withRelevantBinaryOperations op. SinceRelevantBinaryOperationsis not a subtype ofRelevantOperations, this will not type-check (or will fail to traverse as intended). Consider makingRelevantBinaryOperationsextendRelevantOperations, or changelogicalParent/thecountto use a single consistent type hierarchy for traversal and counting.
csharp/ql/src/Complexity/ComplexCondition.ql:1logicalParentis typed toRelevantOperations, but the transitive closure is invoked withRelevantBinaryOperations op. SinceRelevantBinaryOperationsis not a subtype ofRelevantOperations, this will not type-check (or will fail to traverse as intended). Consider makingRelevantBinaryOperationsextendRelevantOperations, or changelogicalParent/thecountto use a single consistent type hierarchy for traversal and counting.
csharp/ql/src/Complexity/ComplexCondition.ql:1logicalParentis typed toRelevantOperations, but the transitive closure is invoked withRelevantBinaryOperations op. SinceRelevantBinaryOperationsis not a subtype ofRelevantOperations, this will not type-check (or will fail to traverse as intended). Consider makingRelevantBinaryOperationsextendRelevantOperations, or changelogicalParent/thecountto use a single consistent type hierarchy for traversal and counting.
csharp/ql/src/Security Features/InsecureRandomness.ql:1- The
existsclause uses multiple|separators, which is easy to misread and makes operator precedence less obvious. Refactor into a single-condition form (for example usingandplus a parenthesizedorblock) 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'.
aschackmull
reviewed
Mar 26, 2026
Contributor
Author
|
DCA looks good. |
aschackmull
approved these changes
Mar 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR has two goals, primarily to avoid making multiple upgrade/downgrade scripts:
a += b, it would synthesizea = a + b.Review on a commit-by-commit basis is encouraged.
Implementation notes
a += b(e.g., whenaandbare int) as an operator call, since it implicitly invokes the user-defined static operator onInt32.??=as the null-coalescing operator is a built-in short-circuit like operation.AddOperation, which can represent eithera + bora += b).DCA notes
cs/unsynchronized-getteris due to removal of a false positive: the query did not properly account for expanded assignments.cs/web/xsslooks acceptable: one alert is removed, but it is essentially a shorter path to the same underlying alert (which is here)cs/useless-upcastlook acceptable.