Skip to content

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ private predicate letElsePanic(BlockExpr be) {
*/
query predicate deadEnd(CfgImpl::Node node) {
Consistency::deadEnd(node) and
successfullyExtractedFile(node.getLocation().getFile()) and
not letElsePanic(node.getAstNode())
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -619,7 +619,7 @@ module PatternTrees {
(
StandardPatTree.super.succ(pred, succ, c)
or
pred = this and first(this.getFirstChildNode(), succ) and completionIsValidFor(c, this)
pred = this and first(this.getFirstChildTree(), succ) and completionIsValidFor(c, this)
)
}

Expand Down
6 changes: 3 additions & 3 deletions rust/ql/lib/codeql/rust/controlflow/internal/Scope.qll
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,10 @@ final class CfgScope = CfgScopeImpl;

final class AsyncBlockScope extends CfgScopeImpl, AsyncBlockExpr instanceof ExprTrees::AsyncBlockExprTree
{
override predicate scopeFirst(AstNode first) { first(super.getFirstChildNode(), first) }
override predicate scopeFirst(AstNode first) { first(super.getFirstChildTree(), first) }

override predicate scopeLast(AstNode last, Completion c) {
last(super.getLastChildElement(), last, c)
last(super.getLastChildTree(), last, c)
or
last(super.getChildNode(_), last, c) and
not c instanceof NormalCompletion
Expand All @@ -48,7 +48,7 @@ final class CallableScope extends CfgScopeImpl, Callable {
}

override predicate scopeFirst(AstNode first) {
first(this.(CallableScopeTree).getFirstChildNode(), first)
first(this.(CallableScopeTree).getFirstChildTree(), first)
}

/** Holds if `scope` is exited when `last` finishes with completion `c`. */
Expand Down
40 changes: 26 additions & 14 deletions shared/controlflow/codeql/controlflow/Cfg.qll
Original file line number Diff line number Diff line change
Expand Up @@ -261,40 +261,52 @@ module MakeWithSplitting<
/** Gets the `i`th child element, in order of evaluation. */
abstract AstNode getChildNode(int i);

private AstNode getChildNodeRanked(int i) {
result = rank[i + 1](AstNode child, int j | child = this.getChildNode(j) | child order by j)
private ControlFlowTree getChildTreeRanked(int i) {
result =
rank[i + 1](ControlFlowTree child, int j | child = this.getChildNode(j) | child order by j)
}

/** Gets the first child node of this element. */
final AstNode getFirstChildNode() { result = this.getChildNodeRanked(0) }
deprecated final AstNode getFirstChildNode() { result = this.getChildTreeRanked(0) }
Copy link
Preview

Copilot AI Aug 17, 2025

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.

Suggested change
deprecated final AstNode getFirstChildNode() { result = this.getChildTreeRanked(0) }
deprecated final AstNode getFirstChildNode() { result = this.getChildNode(0) }

Copilot uses AI. Check for mistakes.

Copy link
Contributor

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.


/** Gets the first child node of this element. */
final ControlFlowTree getFirstChildTree() { result = this.getChildTreeRanked(0) }

/** Gets the last child node of this node. */
deprecated final AstNode getLastChildElement() {
exists(int last |
result = this.getChildTreeRanked(last) and
not exists(this.getChildTreeRanked(last + 1))
Copy link
Preview

Copilot AI Aug 17, 2025

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.

Suggested change
not exists(this.getChildTreeRanked(last + 1))
result = this.getChildNode(last) and
not exists(this.getChildNode(last + 1))

Copilot uses AI. Check for mistakes.

)
}

/** Gets the last child node of this node. */
final AstNode getLastChildElement() {
final ControlFlowTree getLastChildTree() {
exists(int last |
result = this.getChildNodeRanked(last) and
not exists(this.getChildNodeRanked(last + 1))
result = this.getChildTreeRanked(last) and
not exists(this.getChildTreeRanked(last + 1))
)
}

/** Holds if this element has no children. */
predicate isLeafElement() { not exists(this.getFirstChildNode()) }
predicate isLeafElement() { not exists(this.getFirstChildTree()) }

override predicate propagatesAbnormal(AstNode child) { child = this.getChildNode(_) }

pragma[nomagic]
override predicate succ(AstNode pred, AstNode succ, Completion c) {
exists(int i |
last(this.getChildNodeRanked(i), pred, c) and
last(this.getChildTreeRanked(i), pred, c) and
completionIsNormal(c) and
first(this.getChildNodeRanked(i + 1), succ)
first(this.getChildTreeRanked(i + 1), succ)
)
}
}

/** A standard element that is executed in pre-order. */
abstract class StandardPreOrderTree extends StandardTree, PreOrderTree {
override predicate last(AstNode last, Completion c) {
last(this.getLastChildElement(), last, c)
last(this.getLastChildTree(), last, c)
or
this.isLeafElement() and
completionIsValidFor(c, this) and
Expand All @@ -305,24 +317,24 @@ module MakeWithSplitting<
StandardTree.super.succ(pred, succ, c)
or
pred = this and
first(this.getFirstChildNode(), succ) and
first(this.getFirstChildTree(), succ) and
completionIsSimple(c)
}
}

/** A standard element that is executed in post-order. */
abstract class StandardPostOrderTree extends StandardTree, PostOrderTree {
override predicate first(AstNode first) {
first(this.getFirstChildNode(), first)
first(this.getFirstChildTree(), first)
or
not exists(this.getFirstChildNode()) and
not exists(this.getFirstChildTree()) and
first = this
}

override predicate succ(AstNode pred, AstNode succ, Completion c) {
StandardTree.super.succ(pred, succ, c)
or
last(this.getLastChildElement(), pred, c) and
last(this.getLastChildTree(), pred, c) and
succ = this and
completionIsNormal(c)
}
Expand Down
Loading