Skip to content
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.

Sign up for GitHub

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

Rust: Unreachable code query #17525

Merged
merged 18 commits into from
Oct 10, 2024
Merged

Rust: Unreachable code query #17525

Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
35378aa
Rust: Add placeholder query + test for unreachable code query.
geoffw0 Sep 18, 2024
e7e0c6b
Rust: Add qhelp + examples for unreachable code query.
geoffw0 Sep 18, 2024
1eaa998
Rust: Implement unreachable code query.
geoffw0 Sep 18, 2024
58b9355
Merge branch 'main' into unreachable
geoffw0 Sep 20, 2024
3e0d30f
Rust: Merge of unusedvar and unreachable work.
geoffw0 Sep 19, 2024
7c6239b
Merge branch 'main' into unreachable
aibaars Sep 27, 2024
f084bb7
Rust: A couple of interesting cases with short-circuiting.
geoffw0 Oct 8, 2024
483370d
Merge branch 'main' into unreachable
geoffw0 Oct 8, 2024
9e3f4cd
Rust: Accept test changes after merging main.
geoffw0 Oct 8, 2024
40096eb
Rust: More cleanup after merge.
geoffw0 Oct 8, 2024
5d7a92c
Rust: Add example from discussion.
geoffw0 Oct 8, 2024
6bde26d
Rust: Switch firstUnreachable to hvitved's suggested implementation.
geoffw0 Oct 8, 2024
3b1d917
Rust: Autoformat.
geoffw0 Oct 8, 2024
7235ba8
Rust: Fix test compilation errors.
geoffw0 Oct 8, 2024
a7dbe29
Rust: Add parenthesis example to test.
geoffw0 Oct 8, 2024
70d530a
Rust: Exclude nodes that aren't part of the CFG.
geoffw0 Oct 8, 2024
f171eeb
Rust: Restrict 'unreachable' to nodes intended to be part of the CFG.
geoffw0 Oct 9, 2024
719cef8
Merge branch 'main' into unreachable
geoffw0 Oct 9, 2024
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
28 changes: 19 additions & 9 deletions rust/ql/src/queries/unusedentities/UnreachableCode.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,19 +12,29 @@ import rust
import codeql.rust.controlflow.ControlFlowGraph
import codeql.rust.controlflow.internal.ControlFlowGraphImpl as ControlFlowGraphImpl
geoffw0 marked this conversation as resolved.
Show resolved Hide resolved

/**
* Holds if `n` is an AST node that's unreachable.
*/
private predicate unreachable(AstNode n) {
not n = any(CfgNode cfn).getAstNode()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to restrict this to nodes that can possibly be part of a CFG, for example ParenExprs cannot. I think the best heuristic is simply n instanceof ControlFlowTree.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

n instanceof ControlFlowGraphImpl::ControlFlowTree doesn't work ... unless this fix requires more of your work-in-progress to apply?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Try with

  exists(ControlFlowGraphImpl::ControlFlowTree cft |
    cft.succ(n, _, _)
    or
    cft.succ(_, n, _)
  )

instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that works perfectly and I can see what the intent is now.

I've updated the PR.

}

/**
* Holds if `n` is an AST node that's unreachable, and is not the successor
* of an unreachable node (which would be a duplicate result).
*/
predicate firstUnreachable(AstNode n) {
// entry nodes are reachable
not exists(CfgScope s | s.scopeFirst(n)) and
// we never want a `ControlFlowTree` successor node:
// - if the predecessor is reachable, so are we.
// - if the predecessor is unreachable, we're not the *first* unreachable node.
not ControlFlowGraphImpl::succ(_, n, _)
// (note that an unreachable cycle of nodes could be missed by this logic, in
// general it wouldn't be possible to pick one node to represent it)
private predicate firstUnreachable(AstNode n) {
unreachable(n) and
(
// no predecessor -> we are the first unreachable node.
not ControlFlowGraphImpl::succ(_, n, _)
or
// reachable predecessor -> we are the first unreachable node.
exists(AstNode pred |
ControlFlowGraphImpl::succ(pred, n, _) and
not unreachable(pred)
)
)
}

/**
Expand Down
Loading