Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* C# 14: Support for null-conditional assignments (such as `c?.Prop = p`). Furthermore, the `MaybeNullExpr` class now takes null-conditional access (such as `?.`) into account when modeling potential null values.
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 @@ -446,21 +446,29 @@ 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).
* A qualified write access.
*
* 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.
*
* When a qualified write access is used as an `out/ref` argument, the access itself is evaluated immediately.
*/
private class QualifiedWriteAccess extends ControlFlowTree instanceof WriteAccess, QualifiableExpr
{
QualifiedWriteAccess() {
this.hasQualifier()
or
// Member initializers like
// ```csharp
// new Dictionary<int, string>() { [0] = "Zero", [1] = "One", [2] = "Two" }
// ```
// need special treatment, because the accesses `[0]`, `[1]`, and `[2]`
// have no qualifier.
this = any(MemberInitializer mi).getLValue()
(
this.hasQualifier()
or
// Member initializers like
// ```csharp
// new Dictionary<int, string>() { [0] = "Zero", [1] = "One", [2] = "Two" }
// ```
// need special treatment, because the accesses `[0]`, `[1]`, and `[2]`
// have no qualifier.
this = any(MemberInitializer mi).getLValue()
) and
not exists(AssignableDefinitions::OutRefDefinition def | def.getTargetAccess() = this)
}

final override predicate propagatesAbnormal(AstNode child) { child = getExprChild(this, _) }
Expand All @@ -470,25 +478,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
(if i = 0 then not c.(NullnessCompletion).isNull() else any()) and
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 a qualifiable expression, for example `x.Prop = 0`,
* where `Prop` is a property.
*
* Accessor writes need special attention, because we need to model the fact
Expand All @@ -498,24 +506,33 @@ module Expressions {
* ```csharp
* x -> 0 -> set_Prop -> x.Prop = 0
* ```
*
* For consistency, control flow is implemented the same way for other qualified 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 def instanceof AssignableDefinitions::OutRefDefinition 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 +545,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 +561,25 @@ module Expressions {
exists(int i |
last(getExprChild(this, i), pred, c) and
c instanceof NormalCompletion and
(if i = 0 then not c.(NullnessCompletion).isNull() else any()) and
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 +728,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
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
Contributor

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
Contributor Author

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
Contributor

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.

)
}

/** An expression that may be `null`. */
Expand Down
22 changes: 22 additions & 0 deletions csharp/ql/test/library-tests/controlflow/graph/Assignments.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,26 @@ void M()

delegate void EventHandler(object sender, object e);
event EventHandler Event;
int IntField;
string StringField;

void SetParamSingle(out int x)
{
x = 42;
}

void SetParamMulti(out int x, object o, out string y)
{
x = 42;
y = "Hello";
}

void M2()
{
int x1;
SetParamSingle(out x1);
SetParamSingle(out IntField);
SetParamMulti(out var y, null, out StringField);
SetParamMulti(out IntField, null, out StringField);
}
}
Loading
Loading