-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Rust: Data flow through tuple and struct fields #18131
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
Conversation
4e4e27d
to
d89678f
Compare
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, some minor things.
} | ||
|
||
/** Content stored in a field on a struct. */ | ||
private class StructFieldContent extends VariantContent, TStructFieldContent { |
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 this should extend Content
instead of VariantContent
.
* NOTE: Unlike `struct`s and `enum`s tuples are structural and not nominal, | ||
* hence we don't store a canonical path for them. | ||
*/ | ||
private class TuplePositionContent extends VariantContent, TTuplePositionContent { |
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.
Same
@@ -636,6 +687,12 @@ module RustDataFlow implements InputSig<Location> { | |||
pathResolveToVariantCanonicalPath(p.getPath(), v) | |||
} | |||
|
|||
/** Holds if `p` destructs an struct `s`. */ |
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.
a struct
@@ -671,30 +742,44 @@ module RustDataFlow implements InputSig<Location> { | |||
pathResolveToVariantCanonicalPath(re.getPath(), v) | |||
} | |||
|
|||
/** Holds if `re` constructs a struct value of type `v`. */ |
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.
v -> s
exists(AssignmentExprCfgNode assignment, FieldExprCfgNode access | | ||
assignment.getLhs() = access and | ||
n.asExpr() = access.getExpr() and | ||
access.getNameRef().getText().toInt() = | ||
c.(SingletonContentSet).getContent().(TuplePositionContent).getPosition() | ||
) |
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 part should also give rise to a store step (but where the toNode
is the post-update node of n
). That should remove the false negative inside tuple_mutation
.
exists(AssignmentExprCfgNode assignment, FieldExprCfgNode access | | ||
assignment.getLhs() = access and | ||
fieldTuplePositionContent(access, c) and | ||
node1.asExpr() = assignment.getRhs() and | ||
node2.asExpr() = access.getExpr() | ||
) |
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.
Should be node2.(PostUpdateNode).getPreUpdateNode().asExpr()
. But I think we should make a helper predicate
private predicate tupleAssignment(Node node1, Node node2, TuplePositionContent c) {
exists(AssignmentExprCfgNode assignment, FieldExprCfgNode access |
assignment.getLhs() = access and
fieldTuplePositionContent(access, c) and
node1.asExpr() = assignment.getRhs() and
node2.asExpr() = access.getExpr()
)
}
Then the above becomes tupleAssignment(node1, node2.(PostUpdateNode).getPreUpdateNode(), c)
and clearsContent
becomes tupleAssignment(_, n, cs.(SingletonContentSet).getContent())
@@ -260,11 +260,13 @@ module Node { | |||
/** Gets the node before the state update. */ | |||
Node getPreUpdateNode() { result = TExprNode(n) } | |||
|
|||
override ExprCfgNode asExpr() { result = n } |
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.
We don't want this; this predicate should only hold for TExprNode
.
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, I realised that 😅
99c1c36
to
e1c65aa
Compare
I think the DCA looks fine. Alacritty went from 19564 data flow inconsistencies to 20356 but this is before #18144. |
Adds some amount of data flow through tuples and structs.
One notable omission is that field access to
struct
s is not handled. This is because the field in aFieldExpression
is aNameRef
which is not a subclass ofResolvable
. Hence we can not lookup whichstruct
the field is actually for. I think we'll want to have the extractor resolve theseNameRef
s as well, so we can do the same thing that we do for struct-like variants.