-
Notifications
You must be signed in to change notification settings - Fork 1.7k
C# 12: Primary constructors. #15474
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
Merged
michaelnebel
merged 16 commits into
github:main
from
michaelnebel:csharp/primaryconstructors
Feb 14, 2024
Merged
C# 12: Primary constructors. #15474
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
afe3c5e
C#: Re-arrange the code in constructor data flow test and update expe…
michaelnebel 86212b2
C#: Move constructor data flow tests to a separate folder.
michaelnebel f5d4c49
C#: Add some more constructor dataflow tests.
michaelnebel 42f4656
C#: Data flow for primary constructors.
michaelnebel ff29679
C#: Update expected test output.
michaelnebel 4083348
C#: Add a primary constructor QL library test.
michaelnebel aed5080
C#: Add primary constructor change note.
michaelnebel 68b920f
C#: Update other tests expected output.
michaelnebel 69c0f0c
C#: Address review comments.
michaelnebel eaf129d
C#: Update expected test output.
michaelnebel 8efe349
C#: Add indirect assignment example.
michaelnebel 91bbbe2
C#: Address more review comments.
michaelnebel ebd6853
C#: Avoid overlapping output in data flow test
hvitved 3f43f45
C#: Assume captured variables are live at exit in SSA construction
hvitved 7bdc2c5
C#: Simplify `primaryConstructorParameterStore`
hvitved 7c59c7b
C#: Update QLdoc
hvitved File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
4 changes: 4 additions & 0 deletions
4
csharp/ql/lib/change-notes/2024-02-12-primary-constructors.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
--- | ||
category: minorAnalysis | ||
--- | ||
* C# 12: The QL and data flow library now support primary constructors. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,7 @@ private import semmle.code.csharp.frameworks.system.threading.Tasks | |
private import semmle.code.cil.Ssa::Ssa as CilSsa | ||
private import semmle.code.cil.internal.SsaImpl as CilSsaImpl | ||
private import codeql.util.Unit | ||
private import codeql.util.Boolean | ||
|
||
/** Gets the callable in which this node occurs. */ | ||
DataFlowCallable nodeGetEnclosingCallable(Node n) { | ||
|
@@ -37,6 +38,19 @@ predicate isArgumentNode(ArgumentNode arg, DataFlowCall c, ArgumentPosition pos) | |
arg.argumentOf(c, pos) | ||
} | ||
|
||
/** | ||
* Gets a control flow node used for data flow purposes for the primary constructor | ||
* parameter access `pa`. | ||
*/ | ||
private ControlFlow::Node getAPrimaryConstructorParameterCfn(ParameterAccess pa) { | ||
pa.getTarget().getCallable() instanceof PrimaryConstructor and | ||
( | ||
result = pa.(ParameterRead).getAControlFlowNode() | ||
or | ||
pa = any(AssignableDefinition def | result = def.getAControlFlowNode()).getTargetAccess() | ||
) | ||
} | ||
|
||
abstract class NodeImpl extends Node { | ||
/** Do not call: use `getEnclosingCallable()` instead. */ | ||
abstract DataFlowCallable getEnclosingCallableImpl(); | ||
|
@@ -124,9 +138,22 @@ private module ThisFlow { | |
n.(InstanceParameterNode).getCallable() = cfn.(ControlFlow::Nodes::EntryNode).getCallable() | ||
or | ||
n.asExprAtNode(cfn) = any(Expr e | e instanceof ThisAccess or e instanceof BaseAccess) | ||
or | ||
n = | ||
any(InstanceParameterAccessNode pan | | ||
pan.getUnderlyingControlFlowNode() = cfn and pan.isPreUpdate() | ||
) | ||
} | ||
|
||
private predicate thisAccess(Node n, BasicBlock bb, int i) { thisAccess(n, bb.getNode(i)) } | ||
private predicate thisAccess(Node n, BasicBlock bb, int i) { | ||
thisAccess(n, bb.getNode(i)) | ||
or | ||
exists(Parameter p | n.(PrimaryConstructorThisAccessNode).getParameter() = p | | ||
bb.getCallable() = p.getCallable() and | ||
i = p.getPosition() + 1 and | ||
not n instanceof PostUpdateNode | ||
) | ||
} | ||
|
||
private predicate thisRank(Node n, BasicBlock bb, int rankix) { | ||
exists(int i | | ||
|
@@ -202,7 +229,7 @@ CIL::DataFlowNode asCilDataFlowNode(Node node) { | |
|
||
/** Provides predicates related to local data flow. */ | ||
module LocalFlow { | ||
private class LocalExprStepConfiguration extends ControlFlowReachabilityConfiguration { | ||
class LocalExprStepConfiguration extends ControlFlowReachabilityConfiguration { | ||
LocalExprStepConfiguration() { this = "LocalExprStepConfiguration" } | ||
|
||
override predicate candidate( | ||
|
@@ -925,7 +952,17 @@ private module Cached { | |
TParamsArgumentNode(ControlFlow::Node callCfn) { | ||
callCfn = any(Call c | isParamsArg(c, _, _)).getAControlFlowNode() | ||
} or | ||
TFlowInsensitiveFieldNode(FieldOrProperty f) { f.isFieldLike() } | ||
TFlowInsensitiveFieldNode(FieldOrProperty f) { f.isFieldLike() } or | ||
TInstanceParameterAccessNode(ControlFlow::Node cfn, boolean isPostUpdate) { | ||
exists(ParameterAccess pa | cfn = getAPrimaryConstructorParameterCfn(pa) | | ||
isPostUpdate = false | ||
or | ||
pa instanceof ParameterWrite and isPostUpdate = true | ||
) | ||
} or | ||
TPrimaryConstructorThisAccessNode(Parameter p, Boolean isPostUpdate) { | ||
p.getCallable() instanceof PrimaryConstructor | ||
} | ||
|
||
/** | ||
* Holds if data flows from `nodeFrom` to `nodeTo` in exactly one local | ||
|
@@ -961,14 +998,20 @@ private module Cached { | |
TFieldContent(Field f) { f.isUnboundDeclaration() } or | ||
TPropertyContent(Property p) { p.isUnboundDeclaration() } or | ||
TElementContent() or | ||
TSyntheticFieldContent(SyntheticField f) | ||
TSyntheticFieldContent(SyntheticField f) or | ||
TPrimaryConstructorParameterContent(Parameter p) { | ||
p.getCallable() instanceof PrimaryConstructor | ||
} | ||
|
||
cached | ||
newtype TContentApprox = | ||
TFieldApproxContent(string firstChar) { firstChar = approximateFieldContent(_) } or | ||
TPropertyApproxContent(string firstChar) { firstChar = approximatePropertyContent(_) } or | ||
TElementApproxContent() or | ||
TSyntheticFieldApproxContent() | ||
TSyntheticFieldApproxContent() or | ||
TPrimaryConstructorParameterApproxContent(string firstChar) { | ||
firstChar = approximatePrimaryConstructorParameterContent(_) | ||
} | ||
|
||
pragma[nomagic] | ||
private predicate commonSubTypeGeneral(DataFlowTypeOrUnifiable t1, RelevantGvnType t2) { | ||
|
@@ -1037,6 +1080,10 @@ predicate nodeIsHidden(Node n) { | |
n.asExpr() = any(WithExpr we).getInitializer() | ||
or | ||
n instanceof FlowInsensitiveFieldNode | ||
or | ||
n instanceof InstanceParameterAccessNode | ||
or | ||
n instanceof PrimaryConstructorThisAccessNode | ||
} | ||
|
||
/** A CIL SSA definition, viewed as a node in a data flow graph. */ | ||
|
@@ -1745,6 +1792,100 @@ class FlowSummaryNode extends NodeImpl, TFlowSummaryNode { | |
override string toStringImpl() { result = this.getSummaryNode().toString() } | ||
} | ||
|
||
/** | ||
* A data-flow node used to model reading and writing of primary constructor parameters. | ||
* For example, in | ||
* ```csharp | ||
* public class C(object o) | ||
* { | ||
* public object GetParam() => o; // (1) | ||
* | ||
* public void SetParam(object value) => o = value; // (2) | ||
* } | ||
* ``` | ||
* the first access to `o` (1) is modeled as `this.o_backing_field` and | ||
* the second access to `o` (2) is modeled as `this.o_backing_field = value`. | ||
* Both models need a pre-update this node, and the latter need an additional post-update this access, | ||
* all of which are represented by an `InstanceParameterAccessNode` node. | ||
*/ | ||
class InstanceParameterAccessNode extends NodeImpl, TInstanceParameterAccessNode { | ||
private ControlFlow::Node cfn; | ||
private boolean isPostUpdate; | ||
private Parameter p; | ||
|
||
InstanceParameterAccessNode() { | ||
this = TInstanceParameterAccessNode(cfn, isPostUpdate) and | ||
exists(ParameterAccess pa | cfn = getAPrimaryConstructorParameterCfn(pa) and pa.getTarget() = p) | ||
} | ||
|
||
override DataFlowCallable getEnclosingCallableImpl() { | ||
result.asCallable() = cfn.getEnclosingCallable() | ||
} | ||
|
||
override Type getTypeImpl() { result = cfn.getEnclosingCallable().getDeclaringType() } | ||
|
||
override ControlFlow::Nodes::ElementNode getControlFlowNodeImpl() { none() } | ||
|
||
override Location getLocationImpl() { result = cfn.getLocation() } | ||
|
||
override string toStringImpl() { result = "this" } | ||
|
||
/** | ||
* Gets the underlying control flow node. | ||
*/ | ||
ControlFlow::Node getUnderlyingControlFlowNode() { result = cfn } | ||
|
||
/** | ||
* Gets the primary constructor parameter that this is a this access to. | ||
*/ | ||
Parameter getParameter() { result = p } | ||
|
||
/** | ||
* Holds if the this parameter access node corresponds to a pre-update node. | ||
*/ | ||
predicate isPreUpdate() { isPostUpdate = false } | ||
} | ||
|
||
/** | ||
* A data-flow node used to synthesize the body of a primary constructor. | ||
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. Again, I think we should elaborate. Something like For example, in public class C(object o) { } we synthesize the primary constructor for public C(object o) => this.o_backing_field = o; The synthesized (pre/post-update) |
||
* | ||
* For example, in | ||
* ```csharp | ||
* public class C(object o) { } | ||
* ``` | ||
* we synthesize the primary constructor for `C` as | ||
* ```csharp | ||
* public C(object o) => this.o_backing_field = o; | ||
* ``` | ||
* The synthesized (pre/post-update) this access is represented an `PrimaryConstructorThisAccessNode` node. | ||
*/ | ||
class PrimaryConstructorThisAccessNode extends NodeImpl, TPrimaryConstructorThisAccessNode { | ||
private Parameter p; | ||
private boolean isPostUpdate; | ||
|
||
PrimaryConstructorThisAccessNode() { this = TPrimaryConstructorThisAccessNode(p, isPostUpdate) } | ||
|
||
override DataFlowCallable getEnclosingCallableImpl() { result.asCallable() = p.getCallable() } | ||
|
||
override Type getTypeImpl() { result = p.getCallable().getDeclaringType() } | ||
|
||
override ControlFlow::Nodes::ElementNode getControlFlowNodeImpl() { none() } | ||
|
||
override Location getLocationImpl() { result = p.getLocation() } | ||
|
||
override string toStringImpl() { result = "this" } | ||
|
||
/** | ||
* Gets the primary constructor parameter that this is a this access to. | ||
*/ | ||
Parameter getParameter() { result = p } | ||
|
||
/** | ||
* Holds if this is a this access for a primary constructor parameter write. | ||
*/ | ||
predicate isPostUpdate() { isPostUpdate = true } | ||
} | ||
|
||
/** A field or a property. */ | ||
class FieldOrProperty extends Assignable, Modifiable { | ||
FieldOrProperty() { | ||
|
@@ -1881,6 +2022,18 @@ private PropertyContent getResultContent() { | |
result.getProperty() = any(SystemThreadingTasksTaskTClass c_).getResultProperty() | ||
} | ||
|
||
private predicate primaryConstructorParameterStore( | ||
SsaDefinitionExtNode node1, PrimaryConstructorParameterContent c, Node node2 | ||
) { | ||
exists(Ssa::ExplicitDefinition def, ControlFlow::Node cfn, Parameter p | | ||
def = node1.getDefinitionExt() and | ||
p = def.getSourceVariable().getAssignable() and | ||
cfn = def.getControlFlowNode() and | ||
node2 = TInstanceParameterAccessNode(cfn, true) and | ||
c.getParameter() = p | ||
) | ||
} | ||
|
||
/** | ||
* Holds if data can flow from `node1` to `node2` via an assignment to | ||
* content `c`. | ||
|
@@ -1918,6 +2071,14 @@ predicate storeStep(Node node1, ContentSet c, Node node2) { | |
c = getResultContent() | ||
) | ||
or | ||
primaryConstructorParameterStore(node1, c, node2) | ||
or | ||
exists(Parameter p | | ||
node1 = TExplicitParameterNode(p) and | ||
node2 = TPrimaryConstructorThisAccessNode(p, true) and | ||
c.(PrimaryConstructorParameterContent).getParameter() = p | ||
) | ||
or | ||
FlowSummaryImpl::Private::Steps::summaryStoreStep(node1.(FlowSummaryNode).getSummaryNode(), c, | ||
node2.(FlowSummaryNode).getSummaryNode()) | ||
} | ||
|
@@ -2010,6 +2171,14 @@ predicate readStep(Node node1, ContentSet c, Node node2) { | |
node2.asExpr().(AwaitExpr).getExpr() = node1.asExpr() and | ||
c = getResultContent() | ||
or | ||
node1 = | ||
any(InstanceParameterAccessNode n | | ||
n.getUnderlyingControlFlowNode() = node2.(ExprNode).getControlFlowNode() and | ||
n.getParameter() = c.(PrimaryConstructorParameterContent).getParameter() and | ||
n.isPreUpdate() | ||
) and | ||
node2.asExpr() instanceof ParameterRead | ||
or | ||
// node1 = (..., node2, ...) | ||
// node1.ItemX flows to node2 | ||
exists(TupleExpr te, int i, Expr item | | ||
|
@@ -2072,6 +2241,8 @@ predicate clearsContent(Node n, ContentSet c) { | |
c.(FieldContent).getField() = f.getUnboundDeclaration() and | ||
not f.isRef() | ||
) | ||
or | ||
n = any(PostUpdateNode n1 | primaryConstructorParameterStore(_, c, n1)).getPreUpdateNode() | ||
} | ||
|
||
/** | ||
|
@@ -2361,6 +2532,32 @@ module PostUpdateNodes { | |
|
||
override Node getPreUpdateNode() { result.(FlowSummaryNode).getSummaryNode() = preUpdateNode } | ||
} | ||
|
||
private class InstanceParameterAccessPostUpdateNode extends PostUpdateNode, | ||
InstanceParameterAccessNode | ||
{ | ||
private ControlFlow::Node cfn; | ||
|
||
InstanceParameterAccessPostUpdateNode() { this = TInstanceParameterAccessNode(cfn, true) } | ||
|
||
override Node getPreUpdateNode() { result = TInstanceParameterAccessNode(cfn, false) } | ||
|
||
override string toStringImpl() { result = "[post] this" } | ||
} | ||
|
||
private class PrimaryConstructorThisAccessPostUpdateNode extends PostUpdateNode, | ||
PrimaryConstructorThisAccessNode | ||
{ | ||
private Parameter p; | ||
|
||
PrimaryConstructorThisAccessPostUpdateNode() { | ||
this = TPrimaryConstructorThisAccessNode(p, true) | ||
} | ||
|
||
override Node getPreUpdateNode() { result = TPrimaryConstructorThisAccessNode(p, false) } | ||
|
||
override string toStringImpl() { result = "[post] this" } | ||
} | ||
} | ||
|
||
private import PostUpdateNodes | ||
|
@@ -2537,6 +2734,11 @@ class ContentApprox extends TContentApprox { | |
this = TElementApproxContent() and result = "element" | ||
or | ||
this = TSyntheticFieldApproxContent() and result = "approximated synthetic field" | ||
or | ||
exists(string firstChar | | ||
this = TPrimaryConstructorParameterApproxContent(firstChar) and | ||
result = "approximated parameter field " + firstChar | ||
) | ||
} | ||
} | ||
|
||
|
@@ -2550,6 +2752,14 @@ private string approximatePropertyContent(PropertyContent pc) { | |
result = pc.getProperty().getName().prefix(1) | ||
} | ||
|
||
/** | ||
* Gets a string for approximating the name of a synthetic field corresponding | ||
* to a primary constructor parameter. | ||
*/ | ||
private string approximatePrimaryConstructorParameterContent(PrimaryConstructorParameterContent pc) { | ||
result = pc.getParameter().getName().prefix(1) | ||
} | ||
|
||
/** Gets an approximated value for content `c`. */ | ||
pragma[nomagic] | ||
ContentApprox getContentApprox(Content c) { | ||
|
@@ -2560,6 +2770,9 @@ ContentApprox getContentApprox(Content c) { | |
c instanceof ElementContent and result = TElementApproxContent() | ||
or | ||
c instanceof SyntheticFieldContent and result = TSyntheticFieldApproxContent() | ||
or | ||
result = | ||
TPrimaryConstructorParameterApproxContent(approximatePrimaryConstructorParameterContent(c)) | ||
} | ||
|
||
/** | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I think we should elaborate this. Something like
For example, in
the first access to
o
(1) is modeled asthis.o_backing_field
and the second access too
(2) is modeled asthis.o_backing_field = value
. Both models need a pre-updatethis
node, and the latter need an additional post-updatethis
access, all of which are represented by anInstanceParameterAccessNode
node.