Rust: Improve rust/unused-variable and rust/unused-value#18952
Rust: Improve rust/unused-variable and rust/unused-value#18952geoffw0 merged 9 commits intogithub:mainfrom
Conversation
…what the comments apply to.
geoffw0
added
no-change-note-required
There was a problem hiding this comment.
PR Overview
This PR improves the filtering of unused variable and unused value alerts in Rust by excluding results in functions that contain unexpanded macros. Key changes include:
- Addition of new test functions (macros1–macros6, hygiene_mismatch) to cover macro expansion scenarios.
- Adjustments in test annotations to identify missing and spurious alerts.
- A configuration update in options.yml to disable Cargo checking.
Reviewed Changes
| File | Description |
|---|---|
| rust/ql/test/query-tests/unusedentities/main.rs | Added and updated test cases for macros and hygiene scenarios. |
| rust/ql/test/query-tests/unusedentities/options.yml | Updated testing option to disable cargo check. |
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
rust/ql/test/query-tests/unusedentities/main.rs:533
- The assignment inside the block in macros6 does not trigger the expected 'unused-value' alert. Verify if the macro expansion behavior should indeed trigger an unused value alert or update the test annotation accordingly.
x = 10; // $ MISSING: Alert[rust/unused-value]
Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more
|
Some expected output still needs an update: |
|
DCA shows 1509 less results for |
paldepind
left a comment
paldepind
left a comment
There was a problem hiding this comment.
This seems like a great way to reduce the number of FPs. I've left one comment and otherwise it looks great.
| v.getPat().isInMacroExpansion() | ||
| or | ||
| // declared in an incomplete callable | ||
| v.getEnclosingCfgScope() instanceof IncompleteCallable |
There was a problem hiding this comment.
While this works, it would perhaps be more correct to only exclude results when there is an unexpandable macro in the scope that the variable is defined. However, I realize that the concept of variable scope is not currently exposed, so perhaps that is better done follow-up.
There was a problem hiding this comment.
What is required to expose the concept of a variable scope?
I've added a test that exposes the situation I think we're talking about here.
There was a problem hiding this comment.
We would have to expose the VariableScope class from VariableImpl.qll.
There was a problem hiding this comment.
OK, I've added it to the existing improvements issue for rust/unused-variable. It sounds like a straightforward follow-up, apart from the need to go through another cycle of DCA / testing.
Is there any reason VariableScope shouldn't be exposed publically?
There was a problem hiding this comment.
Is there any reason
VariableScopeshouldn't be exposed publically?
Only that we haven't needed it before. We may also have to change it slightly, for example right now x and y are considered to be in the same scope in
let x = ...;
let y = ...;
…s not needed) and accept consistency check changes.
Improve
rust/unused-variableandrust/unused-value, by excluding results in functions that contain unexpanded macros. In these functions, there may be and often are accesses of the supposedly unused variables / values that we simply can't see.In my local testing this removed about 80% of results, for
rust/unused-variablethe quality of remaining results seemed much better (on average), though forrust/unused-valueeven the remaining results were still mostly false positives (note that the latter query has a lower@precision).DCA will confirm.