-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Swift: add new TypeValueExpr
to CFG
#19490
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
Conversation
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.
Pull Request Overview
This PR extends CFG support for the new experimental TypeValueExpr
, adds ValueGenerics tests in both AST and CFG, and relaxes how extractor options/env markers are picked up in qltest.sh
.
- Relax sed usage in
qltest.sh
to capture extractor options and env from anywhere in the source file - Update control‐flow and AST tests (and their expected outputs) to account for
TypeValueExpr
,next(isolation:)
, and ValueGenerics support - Introduce a
TypeValueTree
class stub inControlFlowGraphImpl.qll
for the new expression kind
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
swift/tools/qltest.sh | Allow //codeql-extractor-*- markers on any line (not just line 1) |
swift/ql/test/library-tests/controlflow/graph/cfg.swift | Reorder member‐ref tests and add ValueGenerics snippet |
swift/ql/test/library-tests/controlflow/graph/Cfg.expected | Update expected CFG output for TypeValueExpr, next(isolation:), and ValueGenerics |
swift/ql/test/library-tests/ast/cfg.swift | Mirror CFG changes in the AST test and add ValueGenerics snippet |
swift/ql/test/library-tests/ast/PrintAst.expected | Adjust variable indices and add missing‐type entries for ValueGenerics |
swift/ql/test/library-tests/ast/Missing.expected | Add missing-type entries for TypeValueExpr in ValueGenerics |
swift/ql/test/library-tests/ast/Errors.expected | Add corresponding UnspecifiedElement errors |
swift/ql/lib/codeql/swift/controlflow/internal/ControlFlowGraphImpl.qll | Add TypeValueTree stub to handle TypeValueExpr in the CFG builder |
Comments suppressed due to low confidence (1)
swift/ql/lib/codeql/swift/controlflow/internal/ControlFlowGraphImpl.qll:1887
- [nitpick] The new
TypeValueTree
stub is declared but has no integration logic or registered use. Ensure it’s hooked into the tree‐builder dispatch soTypeValueExpr
nodes actually produce this tree in the CFG.
private class TypeValueTree extends AstLeafTree {
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.
LGTM. I assume we generally want to do this for every new expression and/or statement that is introduced?
yes, I entirely forgot to mention it on your PR, and it only occurred again to me when rereading the ticket about it that was mentioning CFG 😅 |
We do see errors about missing types from type reprs, which is not ideal. However I propose to postpone looking at that when type value expressions get out from being experimental.
Also, changed
qltest.sh
to be more liberal about the location of special option comments in the test source code.