Rust: More tests for rust/deadcode #17923
Conversation
| } | ||
|
|
||
| const _: () = { | ||
| _ = 1; // $ SPURIOUS: Alert[rust/dead-code] |
There was a problem hiding this comment.
This is a child of a Const (Const > BlockExpr > StmtList > ExprStmt > AssignmentExpr); I'm not sure if we want to exclude these from the query, or add them to the CFG so that they're considered reachable.
There was a problem hiding this comment.
One argument for including them in the CFG is that const code could contain unreachable code. If we exclude constant things, then we will not be able to detect this and other things (unused variables, etc.). An argument in the other direction is that code evaluated at compile time is unlikely to contain security issues. Or maybe it can?
There was a problem hiding this comment.
It feels like const code is less likely to contain security issues, but still could. Certainly for data flow - for example the value of a security relevant constant (e.g. enabling or disabling a feature) could be computed in const code.
There was a problem hiding this comment.
Do you know what steps we need to take to include them in the CFG? Is it just a case of adding the entry points somewhere???
There was a problem hiding this comment.
I've created an issue to track this, but I could do with some pointers (or someone else to take on the issue, if that is easier).
There was a problem hiding this comment.
Thanks! I'll be looking at that together as well as the async problem 👍
| if cond() { | ||
| do_something(); | ||
| bail_1!(); // $ SPURIOUS: Alert[rust/dead-code] | ||
| do_something(); // $ MISSING: Alert[rust/dead-code] |
There was a problem hiding this comment.
This is an issue with the workarounds for PostOrderTree and macro nodes. I'll figure out a way to fix it.
There was a problem hiding this comment.
I've created an issue to track this, I know what to do but I have other priorities.
I've no idea, if you want to look into it that would be great. Maybe create an issue tracking this as well as it seems like it could be fairly important. |
It turns out that it's because the control flow doesn't handle I've created and internal issue for this |
|
I'm seeing consistency check failures outside of the tests that are affected by this PR. I think that means something's broken on the particular version of |
I think you're right. I've tried to fix that in #17944. |
|
I hope it's ok that I merged. The CI is passing now and I'm looking at the |
|
No problem at all. |
Add more tests for
rust/deadcode(inspired by real world code), identifying more spurious and missing results.