Shared: Skip non-CFG children in StandardTree
#20230
Conversation
hvitved
force-pushed
the
cfg/standard-tree-skip-non-tree-children
branch
from
41b8124 to
7501e62
Compare
There was a problem hiding this comment.
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) } |
There was a problem hiding this comment.
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) } |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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)) |
Copilot uses AI. Check for mistakes.
The other DCA runs are uneventful. |
Removes the remaining CFG inconsistencies for Rust in DCA.