Repository navigation
Unified: Add CFG support for loop guards and defer - #22815
aschackmull wants to merge 3 commits into
Conversation
aschackmull
added
the
no-change-note-required
There was a problem hiding this comment.
🟡 Changes recommended
The test encodes incorrect fallthrough control flow, and abrupt defer completion remains untested.
2 open findings
What changed in this PR
Adds Unified CFG modeling for Swift loop guards and defer statements.
Changes:
- Adds guarded
for-incontrol-flow edges. - Models
deferusing nested try/finally scopes. - Expands Swift CFG tests and generated expectations.
| File | Description |
|---|---|
shared/controlflow/codeql/controlflow/ControlFlowGraph.qll |
Adds loop-guard CFG support. |
unified/ql/lib/codeql/unified/internal/ControlFlowGraph.qll |
Adapts Swift guards and defers. |
unified/ql/test/library-tests/controlflow/cfg.swift |
Adds CFG test scenarios. |
unified/ql/test/library-tests/controlflow/cfg.expected |
Updates generated CFG expectations. |
unified/ql/test/library-tests/controlflow/basicblock-slices.expected |
Updates generated block slices. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| return true | ||
| case 42: // $ bbStep='42 : match -> Block(+1)' bbStep='42 : no-match -> SwitchCase(+3)' | ||
| print("42!") | ||
| fallthrough // $ bbStep=' : successor -> var obj(+5)' |
There was a problem hiding this comment.
This is expected, fallthrough is not supported by the extractor yet.
| class TryStmt extends Stmt { | ||
| TryStmt() { this instanceof U::TryExpr or this instanceof U::DeferStmt } |
paldepind
left a comment
paldepind
left a comment
There was a problem hiding this comment.
This is a really neat way of handling defer 😊. I've left some small comments, otherwise this looks good to me.
| deferScope(defer, n - 1, mid) and | ||
| not mid instanceof U::DeferStmt and | ||
| block.getStmt(deferIdx + 1 + n) = s | ||
| ) |
There was a problem hiding this comment.
This disjunct is not covered by the tests. I.e., all the tests still pass when deleting it.
It would get exercised by expanding testDefer to the following (or something similar):
func testDefer(x : inout Int) {
// Will print 1, 2, 3, 4, 5, 6
defer {
print("6")
}
print("1")
print("2")
defer {
print("5")
}
defer {
print("3")
defer {
print("4")
}
}
}| class TryStmt extends U::TryExpr { | ||
| AstNode getBody(int index) { index = 0 and result = super.getBody() } | ||
| class TryStmt extends Stmt { | ||
| TryStmt() { this instanceof U::TryExpr or this instanceof U::DeferStmt } |
There was a problem hiding this comment.
I think it would be worth having a comment somewhere with a small example showing how defer is mapped into try-catch?
| class Expr = U::Expr; | ||
|
|
||
| class BlockStmt = U::Block; | ||
| private predicate deferScope(U::DeferStmt defer, int n, Stmt s) { |
There was a problem hiding this comment.
Maybe this qldoc, or something along those lines, would be worth having?
| private predicate deferScope(U::DeferStmt defer, int n, Stmt s) { | |
| /** | |
| * Holds if `s` is the `n`th statement in the constructed `try` block of | |
| * `defer` | |
| */ | |
| private predicate deferScope(U::DeferStmt defer, int n, Stmt s) { |
asgerf
left a comment
asgerf
left a comment
There was a problem hiding this comment.
Looks good to me, though I agree with @paldepind that it would be nice with some qldoc to clarify the interpretation of defer as a try/finally
Loop guard support is added in the shared CFG lib.
Defer statements are handled as try-finally by nesting subsequent statements under the defer and representing them as the try body.