Rust: Unreachable code query #17525
Rust: Unreachable code query #17525
Conversation
|
QHelp previews: rust/ql/src/queries/unusedentities/UnreachableCode.qhelpUnreachable codeThis rule finds code that is never reached. Unused code should be removed to increase readability and avoid confusion. RecommendationRemove any unreachable code. ExampleIn the following example, the final The problem can be fixed simply by removing the unreachable code: References
|
There was a problem hiding this comment.
This looks fine to me. However, it may be worth fixing some of the missing CFG steps instead of marking a lot of false results in the expected output. By the looks of it they are mainly caused by the lack of special handling of boolean literals. I updated the branch to pull in the latest CFG improvements, with a bit of luck some of the issues have already been fixed.
|
@hvitved Could you have a quick look at this too? |
| * 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) { |
There was a problem hiding this comment.
I think it would be better to base this on the resulting CFG, as that can take things like splitting into account (if we implement that). For example, it would be able to identify
if (x) {
...
if (!x) {
// dead
}
}
The CFG construction only assigns CFG nodes to AST nodes that are actually reachable, so it would be something like
private predicate unreachable(AstNode n) {
not n = any(CfgNode cfn).getAstNode()
)
private predicate firstUnreachable(AstNode n) {
unreachable(n) and
(
not ControlFlowGraphImpl::succ(_, n, _)
or
exists(AstNode pred |
ControlFlowGraphImpl::succ(pred, n, _) and
not unreachable(pred)
)
)
)
|
|
||
| fn unreachable_if() { | ||
| if false { | ||
| do_something(); // BAD: unreachable code [NOT DETECTED] |
There was a problem hiding this comment.
Should be fixed by #17602.
Unreachable code query for Rust.
@paldepind please would you review, particularly whether the logic in
firstUnreachableandskipNodeis reasonable and written as cleanly as possible right now; and whether there are obvious explanations for the false positive results in the test?