CFG: Add a new shared inline CFG test and apply it to unified/swift - #22557
aschackmull wants to merge 4 commits into
Conversation
aschackmull
added
the
no-change-note-required
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if Input::getParent+(a2) = a1 | ||
| then dir = Down() | ||
| else none() |
d30a142 to
3dba012
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Multiline CFG nodes are incorrectly excluded from slice-link and noCfg existence checks.
Get a fresh assessment by requesting another Copilot review.
Review tier: Balanced
Findings: 1
Open findings (1)
What changed in this PR
Adds shared inline CFG-test utilities and applies them to Unified Swift control-flow testing.
Changes:
- Introduces reusable basic-block slice and CFG expectation predicates.
- Exposes the test module through the shared CFG implementation.
- Adds Swift fixtures, inline expectations, and generated outputs.
| File | Description |
|---|---|
shared/controlflow/codeql/controlflow/test/TestCfg.qll |
Implements shared CFG-test predicates. |
shared/controlflow/codeql/controlflow/ControlFlowGraph.qll |
Exposes the inline test module. |
unified/ql/test/library-tests/controlflow/cfg.swift |
Adds Swift CFG fixtures and expectations. |
unified/ql/test/library-tests/controlflow/cfg.ql |
Imports the inline CFG test. |
unified/ql/test/library-tests/controlflow/cfg.qlref |
Configures expectation post-processing. |
unified/ql/test/library-tests/controlflow/cfg.expected |
Records generated inline-test output. |
unified/ql/test/library-tests/controlflow/basicblock-slices.ql |
Dumps block slices. |
unified/ql/test/library-tests/controlflow/basicblock-slices.expected |
Records generated slice output. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
3dba012 to
b711a34
Compare
b711a34 to
e09fa64
Compare
Should be fixed now (🤞 ) |
asgerf
left a comment
asgerf
left a comment
There was a problem hiding this comment.
Looks good to me in the sense that I've read through the whole thing and nothing stood out, though the code was not that easy to follow. I'll take another look tomorrow.
I suspect it could be simplified by adding a LineBlock newtype early on? Probably not worth blocking the PR but might be worth considering if it starts to get hard to maintain.
Co-authored-by: Anders Schack-Mulligen <aschackmull@users.noreply.github.com>
27437db to
ef5067a
Compare
asgerf
left a comment
asgerf
left a comment
There was a problem hiding this comment.
Thanks! I found it much easier to follow now. LGTM assuming CI turns green
The test is structured around "basic block slices", i.e. basic blocks cut by line boundaries. Ordinary intra-block control flow is elided and so is flow from/to non-AST CFG nodes, but everything else is reported.
Basic block slices are reported by
nonSimpleif they aren't left-to-right in the location-induced syntax tree.If the basic block cuts that made the slices aren't trivial (i.e. the block continues on the next line), then the link is reported by
bbContinues.All basic block steps are reported by
bbStep(with purely synthetic blocks being skipped over).Lines with AST nodes, but no CFG nodes are reported by
noCfg.Additionally, a separate query dumps all basic block slices, but that's not suitable for inline expectation.