-
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?
Changes from all commits
9894993
b061c4d
0bf0cba
4ba8923
f0135e9
5942edf
ab432ec
812fdbe
3d988e8
bd1c6e6
86198e3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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 |
|---|---|---|
|
|
@@ -429,7 +429,8 @@ 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 QualifiedAccessorWriteOutParam and | ||
| not this instanceof NoNodeExpr and | ||
| not this instanceof SwitchExpr and | ||
| not this instanceof SwitchCaseExpr and | ||
|
|
@@ -448,6 +449,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). | ||
| * | ||
| * 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 | ||
| { | ||
|
|
@@ -470,25 +475,112 @@ 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| first(getExprChild(this, i + 1), succ) | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| private class StatOrDynAccessorCall_ = | ||
| @dynamic_member_access_expr or @dynamic_element_access_expr or @call_access_expr; | ||
| /** | ||
| * An expression that writes via an accessor in an `out` parameter, for example `s = M(out x.Field)`, | ||
| * where `Field` is a field. | ||
| * | ||
| * Note that `ref` parameters are not included here as they are considered reads before the call. | ||
| * Ideally, we would model `ref` parameters as both reads and writes, but that is not currently supported. | ||
| * | ||
| * Accessor writes need special attention, because we need to model that the | ||
| * access is written after the method call. | ||
| * | ||
| * In the example above, this means we want a CFG that looks like | ||
| * | ||
| * ```csharp | ||
| * x -> call M -> x.Field -> s = M(out x.Field) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, I'm not convinced that this is the CFG we want in this case. Consider this example: void MyTest()
{
if (Set(out this.IntField))
return;
Guarded();
}It gets the following CFG flowchart TD
1["enter MyTest"]
10["return ...;"]
11["call to method Guarded"]
12["this access"]
13["...;"]
2["exit MyTest"]
3["exit MyTest (normal)"]
4["{...}"]
5["if (...) ..."]
6["call to method Set"]
7["this access"]
8["this access"]
9["access to field IntField"]
1 --> 4
3 --> 2
4 --> 5
5 --> 7
6 -- false, true --> 9
7 --> 8
8 --> 6
9 -- false --> 13
9 -- true --> 10
10 -- return --> 3
11 --> 3
12 --> 11
13 --> 12
Note how get the join at flowchart TD
1["enter MyTest"]
10["return ...;"]
11["call to method Guarded"]
12["this access"]
13["...;"]
2["exit MyTest"]
3["exit MyTest (normal)"]
4["{...}"]
5["if (...) ..."]
6["call to method Set"]
7["this access"]
8["this access"]
9["access to field IntField"]
1 --> 4
3 --> 2
4 --> 5
5 --> 7
9 --> 6
7 --> 8
8 --> 9
6 -- false --> 13
6 -- true --> 10
10 -- return --> 3
11 --> 3
12 --> 11
13 --> 12
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we can do it like that. However, then it looks like a field read instead.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It will also significantly simplify the implementation. |
||
| * ``` | ||
| */ | ||
| private class QualifiedAccessorWriteOutParam extends PostOrderTree instanceof Expr { | ||
| QualifiedAccessorWriteOutParam() { | ||
| exists(AssignableDefinitions::OutRefDefinition def | | ||
| def.getExpr() = this and | ||
| def.getTargetAccess() instanceof QualifiableExpr and | ||
| def.getTargetAccess().isOutArgument() | ||
| ) | ||
| } | ||
|
|
||
| private QualifiableExpr getOutAccess(int i) { | ||
| result = | ||
| rank[i + 1](AssignableDefinitions::OutRefDefinition def | | ||
| def.getExpr() = this and | ||
| def.getTargetAccess() instanceof QualifiableExpr and | ||
| def.getTargetAccess().isOutArgument() | ||
| | | ||
| def order by def.getIndex() | ||
| ).getTargetAccess() | ||
| } | ||
|
|
||
| private QualifiableExpr getLastOutAccess() { | ||
| exists(int last | | ||
| result = this.getOutAccess(last) and | ||
| not exists(this.getOutAccess(last + 1)) | ||
| ) | ||
| } | ||
|
|
||
| final override predicate propagatesAbnormal(AstNode child) { child = getExprChild(this, _) } | ||
|
|
||
| final override predicate first(AstNode first) { | ||
| first(getExprChild(this, 0), first) | ||
| or | ||
| not exists(getExprChild(this, 0)) and | ||
| first = this | ||
| } | ||
|
|
||
| final override predicate last(AstNode last, Completion c) { | ||
| // The last ast node is the last out writeaccess. | ||
| // Completion from the call itself is propagated (required for eg. conditions). | ||
| last = this.getLastOutAccess() and | ||
| c.isValidFor(this) | ||
| } | ||
|
|
||
| /** A normal or a (potential) dynamic call to an accessor. */ | ||
| private class StatOrDynAccessorCall extends Expr, StatOrDynAccessorCall_ { } | ||
| final override predicate succ(AstNode pred, AstNode succ, Completion c) { | ||
| exists(int i | | ||
| last(getExprChild(this, i), pred, c) and | ||
| c instanceof NormalCompletion | ||
| | | ||
| // Post-order: flow from last element of last child to element itself | ||
| i = max(int j | exists(getExprChild(this, j))) and | ||
| succ = this | ||
| or | ||
| // Standard left-to-right evaluation | ||
| first(getExprChild(this, i + 1), succ) | ||
| ) | ||
| or | ||
| // Flow from this element to the first write access. | ||
| pred = this and | ||
| succ = this.getOutAccess(0) and | ||
| c.isValidFor(pred) and | ||
| c instanceof NormalCompletion | ||
| or | ||
| // Flow from one access to the next | ||
| exists(int i | pred = this.getOutAccess(i) | | ||
| succ = this.getOutAccess(i + 1) and | ||
| c.isValidFor(pred) and | ||
| c instanceof NormalCompletion | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * 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`, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Accessor means stuff like |
||
| * where `Prop` is a property. | ||
| * | ||
| * Accessor writes need special attention, because we need to model the fact | ||
|
|
@@ -498,24 +590,33 @@ 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 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() | ||
|
|
@@ -528,7 +629,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) } | ||
|
|
@@ -538,24 +645,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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment. |
||
| 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 | ||
|
|
@@ -704,7 +812,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 } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this actually needed? I would think that an expression like |
||
| ) | ||
| } | ||
|
|
||
| /** An expression that may be `null`. */ | ||
|
|
||
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 QL doc is now incorrect. Perhaps just remove the entire
In a qualified write access, the access itself is not evaluated, only the qualifier and the indexer arguments (if any).