Skip to content

C# 14: Null conditional assignments. #21158

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
michaelnebel merged 15 commits into from
Jan 19, 2026
Merged

C# 14: Null conditional assignments. #21158

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
9894993
C#: Add CFG support for null conditional assignments and include eg. …
michaelnebel Jan 13, 2026
b061c4d
C#: Update test expected output.
michaelnebel Jan 14, 2026
0bf0cba
C#: Add some null conditional assignments CFG tests.
michaelnebel Jan 14, 2026
4ba8923
C#: Update test expected output.
michaelnebel Jan 14, 2026
f0135e9
C#: Add a tests for MaybeNullExpr.
michaelnebel Jan 14, 2026
5942edf
C#: Take null conditional assignments into account in MaybeNullExpr.
michaelnebel Jan 7, 2026
ab432ec
C#: Update test expected output.
michaelnebel Jan 14, 2026
812fdbe
C#: Add change-note.
michaelnebel Jan 14, 2026
3d988e8
C#: Add field access for out assignments in the CFG.
michaelnebel Jan 15, 2026
bd1c6e6
C#: Exclude the field access cases from missing argument call.
michaelnebel Jan 15, 2026
86198e3
C#: Add tests for out writeacceses.
michaelnebel Jan 16, 2026
beb7750
C#: Address review comments.
michaelnebel Jan 19, 2026
33fc2ba
C#: Update test expected output.
michaelnebel Jan 19, 2026
7ae2b76
C#: Relax the condition for when a qualified expression might be null.
michaelnebel Jan 19, 2026
7ff1c12
C#: Add some more tests.
michaelnebel Jan 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 41 additions & 20 deletions csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,7 @@ module Expressions {
not this instanceof ObjectCreation and
not this instanceof ArrayCreation and
not this instanceof QualifiedWriteAccess and
not this instanceof AccessorWrite and
not this instanceof QualifiedAccessorWrite and
not this instanceof NoNodeExpr and
not this instanceof SwitchExpr and
not this instanceof SwitchCaseExpr and
Expand All @@ -448,6 +448,10 @@ module Expressions {
/**
* A qualified write access. In a qualified write access, the access itself is
* not evaluated, only the qualifier and the indexer arguments (if any).
Comment thread
hvitved marked this conversation as resolved.
Outdated
*
* Note that the successor declaration in `QualifiedAccessorWrite` ensures that the access itself
* is evaluated after the qualifier and the indexer arguments (if any)
* and the right hand side of the assignment.
*/
private class QualifiedWriteAccess extends ControlFlowTree instanceof WriteAccess, QualifiableExpr
{
Expand All @@ -470,25 +474,25 @@ module Expressions {
final override predicate last(AstNode last, Completion c) {
// Skip the access in a qualified write access
last(getLastExprChild(this), last, c)
or
// Qualifier exits with a null completion
super.isConditional() and
last(super.getQualifier(), last, c) and
c.(NullnessCompletion).isNull()
}

final override predicate succ(AstNode pred, AstNode succ, Completion c) {
exists(int i |
last(getExprChild(this, i), pred, c) and
c instanceof NormalCompletion and
(i != 0 or not c.(NullnessCompletion).isNull()) and
Comment thread
hvitved marked this conversation as resolved.
Outdated
first(getExprChild(this, i + 1), succ)
)
}
}

private class StatOrDynAccessorCall_ =
@dynamic_member_access_expr or @dynamic_element_access_expr or @call_access_expr;

/** A normal or a (potential) dynamic call to an accessor. */
private class StatOrDynAccessorCall extends Expr, StatOrDynAccessorCall_ { }

/**
* An expression that writes via an accessor call, for example `x.Prop = 0`,
* An expression that writes via an accessor, for example `x.Prop = 0`,
Comment thread
hvitved marked this conversation as resolved.
Outdated
* where `Prop` is a property.
*
* Accessor writes need special attention, because we need to model the fact
Expand All @@ -498,24 +502,32 @@ module Expressions {
* ```csharp
* x -> 0 -> set_Prop -> x.Prop = 0
* ```
*
* For consistency, control flow is implemented this way for all accessor writes.
* For example, `x.Field = 0`, where `Field` is a field, we want a CFG that looks like
*
* ```csharp
* x -> 0 -> x.Field -> x.Field = 0
* ```
*/
class AccessorWrite extends PostOrderTree instanceof Expr {
private class QualifiedAccessorWrite extends PostOrderTree instanceof Expr {
AssignableDefinition def;

AccessorWrite() {
QualifiedAccessorWrite() {
def.getExpr() = this and
def.getTargetAccess().(WriteAccess) instanceof StatOrDynAccessorCall and
def.getTargetAccess().(WriteAccess) instanceof QualifiableExpr and
not this instanceof AssignOperationWithExpandedAssignment
}

/**
* Gets the `i`th accessor being called in this write. More than one call
* can happen in tuple assignments.
*/
StatOrDynAccessorCall getCall(int i) {
QualifiableExpr getAccess(int i) {
result =
rank[i + 1](AssignableDefinitions::TupleAssignmentDefinition tdef |
tdef.getExpr() = this and tdef.getTargetAccess() instanceof StatOrDynAccessorCall
tdef.getExpr() = this and
tdef.getTargetAccess() instanceof QualifiableExpr
|
tdef order by tdef.getEvaluationOrder()
).getTargetAccess()
Expand All @@ -528,7 +540,13 @@ module Expressions {
final override predicate propagatesAbnormal(AstNode child) {
child = getExprChild(this, _)
or
child = this.getCall(_)
child = this.getAccess(_)
}

final override predicate last(AstNode last, Completion c) {
PostOrderTree.super.last(last, c)
or
last(getExprChild(this, 0), last, c) and c.(NullnessCompletion).isNull()
}

final override predicate first(AstNode first) { first(getExprChild(this, 0), first) }
Expand All @@ -538,24 +556,25 @@ module Expressions {
exists(int i |
last(getExprChild(this, i), pred, c) and
c instanceof NormalCompletion and
(i != 0 or not c.(NullnessCompletion).isNull()) and
Comment thread
hvitved marked this conversation as resolved.
Outdated
first(getExprChild(this, i + 1), succ)
)
or
// Flow from last element of last child to first accessor call
last(getLastExprChild(this), pred, c) and
succ = this.getCall(0) and
succ = this.getAccess(0) and
c instanceof NormalCompletion
or
// Flow from one call to the next
exists(int i | pred = this.getCall(i) |
succ = this.getCall(i + 1) and
exists(int i | pred = this.getAccess(i) |
succ = this.getAccess(i + 1) and
c.isValidFor(pred) and
c instanceof NormalCompletion
)
or
// Post-order: flow from last call to element itself
exists(int last | last = max(int i | exists(this.getCall(i))) |
pred = this.getCall(last) and
exists(int last | last = max(int i | exists(this.getAccess(i))) |
pred = this.getAccess(last) and
succ = this and
c.isValidFor(pred) and
c instanceof NormalCompletion
Expand Down Expand Up @@ -704,7 +723,9 @@ module Expressions {
private class ConditionallyQualifiedExpr extends PostOrderTree instanceof QualifiableExpr {
private Expr qualifier;

ConditionallyQualifiedExpr() { this.isConditional() and qualifier = getExprChild(this, 0) }
ConditionallyQualifiedExpr() {
this.isConditional() and qualifier = getExprChild(this, 0) and not this instanceof WriteAccess
}

final override predicate propagatesAbnormal(AstNode child) { child = qualifier }

Expand Down
6 changes: 6 additions & 0 deletions csharp/ql/lib/semmle/code/csharp/dataflow/Nullness.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,12 @@ private Expr maybeNullExpr(Expr reason) {
)
or
result.(NullCoalescingExpr).getRightOperand() = maybeNullExpr(reason)
or
result =
any(QualifiableExpr qe |
qe.isConditional() and
qe.getQualifier() = maybeNullExpr(reason)
Copy link
Copy Markdown
Contributor

@hvitved hvitved Jan 16, 2026

Choose a reason for hiding this comment

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

Is this actually needed? I would think that an expression like x?.M() can always potentially be null, regardless of what we know about x.

Copy link
Copy Markdown
Contributor Author

@michaelnebel michaelnebel Jan 19, 2026

Choose a reason for hiding this comment

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

Yes, if x is variable (or other stuff that is not special cased in the MaybeNullExpr class), but it appears we need to propgate the maybe null information for casts and conditionals (stuff that is explicitly handled in the MaybeNullExpr class itself). That is, to properly handle (x as C).GetInt().
An example of an extra finding: https://github.com/dotnet/roslyn/blob/6afbfb45ccc9691167206bf29482a99b1d6d469c/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTupleTest.cs#L24398

Copy link
Copy Markdown
Contributor

@hvitved hvitved Jan 19, 2026

Choose a reason for hiding this comment

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

What I meant was why not

result = any(QualifiableExpr qe | qe.isConditional() and reason = qe.getQualifier())

That should give us strictly more results, and I would assume that any x?.M() expression can be potentially null because it is conditionally qualified.

Copy link
Copy Markdown
Contributor Author

@michaelnebel michaelnebel Jan 19, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

That is somewhat less conservative. It means that stuff like

void M1(object o)
{
    var t = o?.GetType();
    Console.WriteLine(t.FullName);
}

gets flagged (which is good), but it also means that we introduce false positives in cases like

void M2()
{
    var o = new object();
    var t = o?.GetType();
    Console.WriteLine(t.FullName); // GOOD
}

Maybe that is acceptable for a "maybe" query (to consider the intent of the use of ?). I will try and make the change and see what results DCA produce.

Copy link
Copy Markdown
Contributor

@hvitved hvitved Jan 19, 2026

Choose a reason for hiding this comment

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

I would think that was OK; if the qualifier cannot be null then there is no need to use ?.

)
}

/** An expression that may be `null`. */
Expand Down
Loading