-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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.
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
base: main
Are you sure you want to change the base?
Conversation
30e2a16 to
15848f3
Compare
438153d to
15e9685
Compare
8877b1e to
3c42988
Compare
|
@hvitved : Would it be possible to get some early feedback on the contents of the PR (in case we should design it differently)? |
…field access in the CFG.
3c42988 to
86198e3
Compare
hvitved
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, I mostly have a concern about CFGs for out arguments.
csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll
Outdated
Show resolved
Hide resolved
csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll
Outdated
Show resolved
Hide resolved
csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll
Outdated
Show resolved
Hide resolved
csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll
Outdated
Show resolved
Hide resolved
| result = | ||
| any(QualifiableExpr qe | | ||
| qe.isConditional() and | ||
| qe.getQualifier() = maybeNullExpr(reason) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll
Outdated
Show resolved
Hide resolved
csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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 pull request adds support for C# 14 null conditional assignments and improves control flow graph handling for qualified write access. The changes update the control flow logic to match how property assignments work, ensuring field/property accesses are included in the control flow graph after evaluating the right-hand side.
Changes:
- Updated control flow graph to include field/property access nodes before assignments
- Added support for null conditional assignments (e.g.,
ca?.Prop = value) - Improved
MaybeNullExprclass to handle null conditional read accesses - Added comprehensive test coverage for the new features
Reviewed changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| csharp/ql/test/library-tests/standalone/controlflow/cfg.expected | Updated expected control flow for array element access assignments |
| csharp/ql/test/library-tests/obinit/ObInit.expected | Updated control flow for object initializer field assignments |
| csharp/ql/test/library-tests/dataflow/signanalysis/SignAnalysis.expected | Added entries for field access in assignments |
| csharp/ql/test/library-tests/dataflow/nullness/options | Added extractor options for nullness test |
| csharp/ql/test/library-tests/dataflow/nullness/maybeNullExpr.ql | New query for testing MaybeNullExpr class |
| csharp/ql/test/library-tests/dataflow/nullness/maybeNullExpr.expected | Expected results for MaybeNullExpr test |
| csharp/ql/test/library-tests/dataflow/nullness/MaybeNullExpr.cs | Test cases for null conditional expressions |
| csharp/ql/test/library-tests/dataflow/local/TaintTrackingStep.expected | Updated taint tracking for array element assignments |
| csharp/ql/test/library-tests/csharp8/*.expected | Updated control flow for various C# 8 features with new assignment handling |
| csharp/ql/test/library-tests/controlflow/graph/*.expected | Comprehensive updates to control flow graph expected results |
| csharp/ql/test/library-tests/controlflow/graph/ConditionalAccess.cs | New test cases for null conditional assignments (M9 method) |
| csharp/ql/test/library-tests/controlflow/graph/Assignments.cs | New test cases for out parameter assignments with fields |
Comments suppressed due to low confidence (3)
csharp/ql/test/library-tests/dataflow/nullness/MaybeNullExpr.cs:13
- This assignment to nullCoalescing is useless, since its value is never read.
var nullCoalescing = o ?? null;
csharp/ql/test/library-tests/dataflow/nullness/MaybeNullExpr.cs:16
- This assignment to c is useless, since its value is never read.
var c = o as C;
csharp/ql/test/library-tests/dataflow/nullness/MaybeNullExpr.cs:19
- This assignment to s1 is useless, since its value is never read.
var s1 = (o as C)?.Prop;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var c = o as C; | ||
|
|
||
| // Conditional access might be null as the qualifier might be null. | ||
| var s1 = (o as C)?.Prop; |
Copilot
AI
Jan 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Local scope variable 's1' shadows C.s1.
| var s1 = (o as C)?.Prop; | |
| var propValue = (o as C)?.Prop; |
| void M(object o, bool b) | ||
| { | ||
| // Conditional expr might be null. | ||
| var conditionalExpr = b ? new object() : null; |
Copilot
AI
Jan 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assignment to conditionalExpr is useless, since its value is never read.
This issue also appears in the following locations of the same file:
- line 13
- line 16
- line 19
| string this[int index] | ||
| { | ||
| get { return null; } | ||
| set { } |
Copilot
AI
Jan 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Value ignored when setting property.
| string this[int index] | |
| { | |
| get { return null; } | |
| set { } | |
| string[] _items = new string[1]; | |
| string this[int index] | |
| { | |
| get { return index == 0 ? _items[0] : null; } | |
| set | |
| { | |
| if (index == 0) | |
| _items[0] = value; | |
| } |
hvitved
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just the one comment in Nullness.qll.
In this PR, we:
MaybeNullExprclass so that it now correctly handles null conditional (read) accesses.Changes to existing control flow
Previously, the control flow for
looked like this:
With this PR, we now include the field access after evaluating the right-hand side, so the control flow becomes:
This matches how property assignments are handled, where it’s important for the property access to be included in the control flow after evaluating the right-hand side, since accessing the property corresponds to invoking the property setter.
Previously, the control flow for
looked like this:
With this PR, we now include the field access before the call, so the control flow becomes:
New control flow.
The changes to the control flow graph implementation mean that for a statement like
the control flow is now built correctly to reflect the sequence of null conditional accesses.

That is, if the qualifier uses a null conditional, we add a null edge from the qualifier to the point after the assignment. This reflects the fact that if the qualifier is null, the right-hand side of the assignment isn’t evaluated.