Skip to content

Swift: flow through writeable keypaths #14165

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
merged 7 commits into from
Sep 12, 2023

Conversation

rdmarsh2
Copy link
Contributor

@rdmarsh2 rdmarsh2 commented Sep 7, 2023

No description provided.

@rdmarsh2 rdmarsh2 requested a review from a team as a code owner September 7, 2023 15:57
@github-actions github-actions bot added the Swift label Sep 7, 2023
Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of comments, but generally this LGTM. We should also make sure to run DCA on this before we merge it.

Comment on lines +256 to +260
exists(KeyPathApplicationExpr apply, AssignExpr assign |
apply = assign.getDest() and
nodeTo.asExpr() = apply and
nodeFrom.asExpr() = assign.getSource()
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see why you need to do this. So a write through a key path looks like:

myObject[keyPath: kp] = x

and this is stepping from x to myObject[keyPath: kp], right?

Maybe the right way to do this would be to say that myObject[keyPath: kp] is an SSA definition of myObject. That would make this write an instance of Ssa::WriteDefinition and this case would then be covered by the first case in this very predicate: https://github.com/github/codeql/blob/main/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll#L144-L146

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that doesn't quite seem to do what I want here - the current implementation steps from the source to the preupdate node, then there's interprocedural flow through the keypath to the postupdate node. With the WriteDefinition that first step goes to the WriteDefinition and then skips the postupdate node entirely, from what I can tell....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see. Yeah, I can see how this is different. Maybe this is simply how we need to do things then 🤔


KeyPathAssignmentArgumentNode() {
keyPath = this.getCfgNode() and
exists(AssignExpr assign | assign.getDest() = keyPath.getNode().asAstNode())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do the changes I suggested above (i.e., making myObject[keyPath: kp] = x an SSA definition of myObject) then this could also be prettified using Ssa::WriteDefinition.

Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (except for the formatting issue) 👍

@rdmarsh2 rdmarsh2 merged commit ecf1d98 into github:main Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants