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.

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: Fix rust/unused-variable FPs #17913

Merged
merged 10 commits into from
Nov 8, 2024
Merged

Rust: Fix rust/unused-variable FPs #17913

merged 10 commits into from
Nov 8, 2024

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Nov 5, 2024

Add more test cases for rust/unused-variable, inspired by syntax in real world false positive results for the query. I could only get one pattern to reproduce in the test and I've fixed it here.

In one of our largest DCA databases this reduces the total number of results from 212,420 -> 100. A DCA run will confirm the overall picture.

@geoffw0 geoffw0 added the Rust Pull requests that update Rust code label Nov 5, 2024
rust/ql/test/query-tests/unusedentities/main.rs Dismissed Show dismissed Hide dismissed
rust/ql/test/query-tests/unusedentities/main.rs Dismissed Show dismissed Hide dismissed
rust/ql/test/query-tests/unusedentities/main.rs Dismissed Show dismissed Hide dismissed
rust/ql/test/query-tests/unusedentities/main.rs Dismissed Show dismissed Hide dismissed
rust/ql/test/query-tests/unusedentities/main.rs Dismissed Show dismissed Hide dismissed
rust/ql/test/query-tests/unusedentities/main.rs Dismissed Show dismissed Hide dismissed
@geoffw0
Copy link
Contributor Author

geoffw0 commented Nov 6, 2024

DCA shows the total number of results for rust/unused-variable dropping from 212613 to 286 by 212,613. Also analysis is 1% faster, probably because we're dealing with less results data.

hvitved
hvitved previously approved these changes Nov 6, 2024
// variable is in a context where is may not have a use
not v.getPat().isInMacroExpansion() and
not exists(FnPtrType fp | fp.getParamList().getParam(_).getPat() = v.getPat())
not exists(v.getInitializer())
Copy link
Contributor

Choose a reason for hiding this comment

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

@hvitved Should there be a "write access" for a variable with an initializer? The following two examples should probably be considered equivalent.

let x : i32 = 10;

and

let x : i32;
x = 10;

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, I have plans to flip this behaviour so that both would be considered an unused variable (and not an unused value). There's an issue tracking this, it will probably be simple, but I haven't got around to it yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

The x in let x : i 32 = 10; is a pattern and not an expression, so making it a write access is not that natural.

aibaars
aibaars previously approved these changes Nov 6, 2024
Copy link
Contributor

@aibaars aibaars left a comment

Choose a reason for hiding this comment

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

This is great! I had never imagined that most of the false positives originated from function pointer types.

@geoffw0
Copy link
Contributor Author

geoffw0 commented Nov 6, 2024

The second DCA run LGTM.

I had never imagined that most of the false positives originated from function pointer types.

One project in particular uses them a huge amount.

@geoffw0 geoffw0 dismissed stale reviews from aibaars and hvitved via 18ce8be November 6, 2024 19:00
@geoffw0 geoffw0 merged commit 0610c26 into github:main Nov 8, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants