-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Shared: Skip non-CFG children in StandardTree
#20230
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
Shared: Skip non-CFG children in StandardTree
#20230
Conversation
41b8124 to
7501e62
Compare
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 updates the StandardTree class in the control flow graph implementation to properly handle CFG children by changing method signatures and calls from AstNode to ControlFlowTree. The changes ensure that only CFG-relevant children are processed, addressing CFG inconsistencies in Rust's Data Control Analysis (DCA).
Key changes:
- Replace
AstNodereturn types withControlFlowTreein child node methods - Add deprecated versions of old methods for backward compatibility
- Update all references to use the new CFG-aware child methods
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| shared/controlflow/codeql/controlflow/Cfg.qll | Core changes to StandardTree class, replacing AstNode with ControlFlowTree in child node methods and adding deprecated alternatives |
| rust/ql/lib/codeql/rust/controlflow/internal/Scope.qll | Updates scope classes to use new CFG-aware child methods |
| rust/ql/lib/codeql/rust/controlflow/internal/ControlFlowGraphImpl.qll | Updates pattern tree implementation to use new child method |
| rust/ql/lib/codeql/rust/controlflow/internal/CfgConsistency.qll | Adds file extraction check to deadEnd query predicate |
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
|
|
||
| /** Gets the first child node of this element. */ | ||
| final AstNode getFirstChildNode() { result = this.getChildNodeRanked(0) } | ||
| deprecated final AstNode getFirstChildNode() { result = this.getChildTreeRanked(0) } |
Copilot
AI
Aug 17, 2025
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 deprecated getFirstChildNode() method returns AstNode but calls getChildTreeRanked(0) which returns ControlFlowTree. This type mismatch will cause compilation errors since ControlFlowTree cannot be implicitly converted to AstNode.
| deprecated final AstNode getFirstChildNode() { result = this.getChildTreeRanked(0) } | |
| deprecated final AstNode getFirstChildNode() { result = this.getChildNode(0) } |
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 it can do that conversion just fine. This change doesn't appear to preserve the existing behaviour of getFirstChildNode perfectly, but as it's being deprecated I'm not overly concerned about that.
| deprecated final AstNode getLastChildElement() { | ||
| exists(int last | | ||
| result = this.getChildTreeRanked(last) and | ||
| not exists(this.getChildTreeRanked(last + 1)) |
Copilot
AI
Aug 17, 2025
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 deprecated getLastChildElement() method has the same type mismatch issue - it returns AstNode but assigns from getChildTreeRanked(last) which returns ControlFlowTree. This will cause compilation errors.
| not exists(this.getChildTreeRanked(last + 1)) | |
| result = this.getChildNode(last) and | |
| not exists(this.getChildNode(last + 1)) |
geoffw0
left a comment
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.
Code changes and Rust DCA run LGTM.
(I can review the other language DCA runs later if nobody else does it)
The other DCA runs are uneventful. |
Removes the remaining CFG inconsistencies for Rust in DCA.