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

Jump to bottom

Rust: Implement UnusedValue.ql #17726

Closed
wants to merge 3 commits into from
Closed

Rust: Implement UnusedValue.ql #17726

wants to merge 3 commits into from

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Oct 10, 2024 •
edited
Loading

This PR implements an initial version of UnusedValue.ql, based on SSA.

@github-actions github-actions bot added C# Ruby Rust Pull requests that update Rust code Swift labels Oct 10, 2024
github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 10, 2024
View reviewed changes
rust/ql/lib/codeql/rust/dataflow/internal/SsaImpl.qll

/** Same as `lastRefRedef`, but skips uncertain reads. */
pragma[nomagic]
private predicate lastRefSkipUncertainReadsExt(DefinitionExt def, BasicBlock bb, int i) {

Check warning

Code scanning / CodeQL

Missing QLDoc for parameter Warning

The QLDoc has no documentation for bb, or def, or i, but the QLDoc mentions lastRefRedef
@hvitved hvitved force-pushed the rust/unused-value branch 2 times, most recently from 1165f83 to 548521b Compare October 11, 2024 12:47
geoffw0
geoffw0 reviewed Oct 11, 2024
View reviewed changes
rust/ql/src/queries/unusedentities/UnusedValue.ql Show resolved Hide resolved
rust/ql/test/query-tests/unusedentities/main.rs
@@ -189,7 +189,7 @@ enum YesOrNo {
No,
}

use YesOrNo::{Yes, No}; // allows `Yes`, `No` to be accessed without qualifiers.
use YesOrNo::{No, Yes}; // allows `Yes`, `No` to be accessed without qualifiers.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I think it's the auto-formatter.

Copy link
Contributor

Choose a reason for hiding this comment

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

In general I believe we should not autoformat test code in the target language. Autoformatting adds a regularity that could (theoretically) mask issues in the extractor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not that I strongly care about this particular case (reverting not necessary).

rust/ql/test/query-tests/unusedentities/main.rs Outdated Show resolved Hide resolved
geoffw0
geoffw0 reviewed Oct 14, 2024
View reviewed changes
Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

LGTM once #17713 is merged.

@geoffw0
Copy link
Contributor

geoffw0 commented Oct 14, 2024

Oh, we should have a (successful) DCA run to check this query isn't too noisy (like rust/unused-variable was).

@geoffw0 geoffw0 mentioned this pull request Oct 15, 2024
Merged
@geoffw0
Copy link
Contributor

geoffw0 commented Oct 15, 2024

The underlying PR (first two commits here) is merged and I'm happy with the third commit. However there are merge conflicts and we need a DCA run, but @hvitved is away. So I've made a new PR here containing the third commit + merge fixup here: #17773

@hvitved
Copy link
Contributor Author

hvitved commented Oct 21, 2024

Superseded by #17773.

@hvitved hvitved closed this Oct 21, 2024
@geoffw0
Copy link
Contributor

geoffw0 commented Oct 28, 2024

Thanks for the work you did on this (all merged now).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geoffw0 geoffw0 geoffw0 left review comments

Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

Assignees
No one assigned
Labels
Rust Pull requests that update Rust code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hvitved @geoffw0