C#: Remove expanded assignments.#21452
Conversation
c94de85 to
1d8d712
Compare
abb75be to
5021ea9
Compare
41fef59 to
07144c2
Compare
|
@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
| * 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 |
There was a problem hiding this comment.
Spurious "or"
| * (`AssignArithmeticOperation`), a bitwise assignment operation or | |
| * (`AssignArithmeticOperation`), a bitwise assignment operation |
…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.
8f725cd to
028f2b8
Compare
028f2b8 to
147ac37
Compare
209771a to
a402ce5
Compare
|
@aschackmull @hvitved : This PR is now ready for review.
|
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'.
| TAssignment(Expr e) or | ||
| TLhs(Expr e) or | ||
| TRhs(Expr e) |
There was a problem hiding this comment.
These could be CompoundAssignmentExpr instead of Expr, right? I don't know if it matters - it probably doesn't.
There was a problem hiding this comment.
Yes! I will change it.
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.