-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Rust: Improve CFG #17498
Rust: Improve CFG #17498
Conversation
} | ||
} | ||
|
||
class LogicalOrBinaryOpExprTree extends PreOrderTree instanceof BinaryExpr { |
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.
The short-circuiting operators are pre-ordered s.t. nested operators can be handled in a way where impossible paths are eliminated.
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.
As discussed offline, this is fine for now, but we want to change this later to use splitting.
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.
Looks very promising!
there probably are some bugs in there :)
Hopefully when we have consistency checks and actual queries depending on this we'll discover any issues quite rapidly.
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.
Really great work, Simon!
rust/ql/lib/codeql/rust/controlflow/internal/ControlFlowGraphImpl.qll
Outdated
Show resolved
Hide resolved
rust/ql/lib/codeql/rust/controlflow/internal/ControlFlowGraphImpl.qll
Outdated
Show resolved
Hide resolved
rust/ql/lib/codeql/rust/controlflow/internal/ControlFlowGraphImpl.qll
Outdated
Show resolved
Hide resolved
rust/ql/lib/codeql/rust/controlflow/internal/ControlFlowGraphImpl.qll
Outdated
Show resolved
Hide resolved
rust/ql/lib/codeql/rust/controlflow/internal/ControlFlowGraphImpl.qll
Outdated
Show resolved
Hide resolved
The PR is ready for re-review. I've addressed most of the comments. I didn't fix the |
I don't understand why the CFGs for |
You're right, I didn't notice that. It could be due to the changes in the extractor or maybe some of the last changes I made broke it. Let's investigate after merging :) |
With this PR CFG creation is (attempted) to cover every relevant node in the AST. Some noteworthy additions are
match
expressions, closures,continue
andbreak
in loops (including with labels),if let
expressions and short-circuiting binary operators.I'm not aware of any syntax constructions that are not supported, but there probably are some bugs in there :)