-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Swift: Dataflow for keypaths #12807
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
Swift: Dataflow for keypaths #12807
Conversation
…ambdas that perform a sequence of read steps.
9b12f29
to
f32d77b
Compare
DCA looks fine: the new lost result is due to extractor-wobbliness. When I run |
swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll
Outdated
Show resolved
Hide resolved
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.
Results LGTM but I would like to see more diverse test cases. You mentioned writeable key-path expressions and optional handling as planned follow-up. I've also come across:
- keypaths of the form
\.field
where the object can be inferred from context - keypaths into arrays
\[Int][0]
Sure, I'll add more tests. I wasn't aware of the inferred-context version. I'm also sure |
231b0fc adds a few more tests. |
Great, thanks. Happy with the tests now!
Good point. It will be nice when we have ArrayContent working and this comes together. |
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.
Looking good! See my comment though.
let s = S(x: source()) | ||
let s2 = S2_Optional(s: s) | ||
let f = \S2_Optional.s?.x | ||
sink(opt: s2[keyPath: f]) // $ MISSING: flow=611 |
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 you should have all the pieces now for making this work, no? We have content flow for optionals, it just needs to be linked to key paths like you did with field contents.
I'm also fine with leaving this as a follow-up issue though, but if it's not too complicated it would be nice to see this test pass.
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.
Yeah, I think everything should be in place to make this work. I'd like to save it as follow-up, though :)
I'll create an issue with what needs to happen as a follow-up
This PR implements dataflow-support for key-path expressions. This has been made possible thanks for Sasha's recent PR which extracted the right things 🎉.
After this PR, we model a key-path expression as a lambda with a sequence of read steps. For example, consider this snippet:
this is now modelled like we model:
There's still some future work to be done: I've only supported simple field lookups inside those key-path chains. We also need to handle wrapping and unwrapping of optionals, and writeable key-path expressions. All of this should be fairly straightforward once the infrastructure from this PR is merged, though.
Commit-by-commit review recommended.
@d10c I've assigned you as a reviewer because I think you've touched many of these files before (and will probably be touching them again soon). Let me know if you'd like to go through this PR over Zoom or something.