Skip to content

Rust: Handle chained let expressions #20203

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

Merged
merged 3 commits into from
Aug 14, 2025

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Aug 11, 2025

This PR adds support for chained let expressions.

Before this PR, we only handled let expressions that occurred immediately under an if or a while expression, but with this PR we make the following changes:

  1. Generalize the control flow graph implementation to handle let expressions like other expressions (in post-order instead of pre-order), which means that the CFG implementation should be able to handle let expressions in any position.

  2. Generalize the current variable resolution implementation to handle let expressions in any position. Care must be taken when variables are introduced inside a condition:

let x = Some(42);          // A: defines x1

if let Some(x)             // B: defines x2
    = x                    // C: reads x1
    &&
    let Some(x)            // D: defines x3
    = Some(x)              // E: reads x2
    &&
    x > 0                  // F: reads x3
{
    print_i64(x);          // G: reads x3
} else {
    print_i64(x.unwrap()); // H: reads x1
}

Just like let statements can shadow previous definitions, so can let expressions, and we utilize the existing machinery for handling that. In order for the write at D to not reach the read at H, we model all conditions as a separate variable scopes, and then add only the then branches to those scopes.

  1. Extend data flow to propagte flow from RHS to LHS for let expressions (like we already do for let statements).

DCA looks great: CFG inconsistencies are down, rust/unused-variable FPs removed, performance is unchanged.

@github-actions github-actions bot added the Rust Pull requests that update Rust code label Aug 11, 2025
@hvitved hvitved force-pushed the rust/if-let-chain-test branch from def7f7a to 3994566 Compare August 12, 2025 13:47
@hvitved hvitved force-pushed the rust/if-let-chain-test branch 5 times, most recently from ba623c6 to 450b458 Compare August 12, 2025 17:40
@github github deleted a comment from juancarlos850810-cpu Aug 13, 2025
@hvitved hvitved changed the title Rust: Add test for if-let chain Rust: Handle chained let expressions Aug 13, 2025
@hvitved hvitved force-pushed the rust/if-let-chain-test branch 4 times, most recently from 423d2b4 to 3659ff5 Compare August 14, 2025 08:24
* child of the loop condition instead of the loop itself.
*/
pragma[nomagic]
private Element getImmediateChildAdj(Element e, int index) {

Check warning

Code scanning / CodeQL

Missing QLDoc for parameter Warning

The QLDoc has no documentation for e, or index, but the QLDoc mentions body, and cond, and while
@hvitved hvitved force-pushed the rust/if-let-chain-test branch from 3659ff5 to f63e55c Compare August 14, 2025 08:36
@hvitved hvitved marked this pull request as ready for review August 14, 2025 12:40
@hvitved hvitved requested a review from a team as a code owner August 14, 2025 12:40
@Copilot Copilot AI review requested due to automatic review settings August 14, 2025 12:40
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for chained let expressions in Rust by generalizing control flow graph implementation and variable resolution to handle let expressions in any position, not just immediately under if or while expressions.

Key changes:

  • Generalizes CFG implementation to handle let expressions in post-order instead of pre-order
  • Extends variable resolution to handle let expressions with proper scoping and shadowing rules
  • Adds data flow propagation from RHS to LHS for let expressions

Reviewed Changes

Copilot reviewed 29 out of 32 changed files in this pull request and generated no comments.

Show a summary per file
File Description
rust/ql/test/utils-tests/modelgenerator/option.rs Updates expected model generation output with new data flow summaries
rust/ql/test/query-tests/unusedentities/options.yml Enables nightly Rust features for testing
rust/ql/test/query-tests/unusedentities/main.rs Adds test for chained let expressions and enables let_chains feature
rust/ql/test/query-tests/unusedentities/UnusedVariable.expected Updates expected unused variable detections with corrected line numbers
rust/ql/test/query-tests/unusedentities/UnusedValue.expected Updates expected unused value detections including new alert for unused let variable
rust/ql/test/query-tests/security/CWE-825/lifetime.rs Updates test expectations for lifetime analysis with chained let expressions
rust/ql/test/query-tests/security/CWE-825/AccessAfterLifetime.expected Updates expected lifetime violation detections
rust/ql/test/library-tests/variables/ Multiple files updating variable analysis tests with new chained let expression support
rust/ql/test/library-tests/dataflow/ Updates data flow tests to reflect improved let expression handling

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

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.

The tests you introduced are precise and thorough. The improvements to existing dataflow, query, and model generator tests prove the applicability / value of this change. I'm looking forward to this being merged!

@redsun82 are you happy as well?

@@ -318,7 +319,7 @@ fn if_lets_matches() {
No => {}
}

if let j = Yes { // $ Alert[rust/unused-variable]
if let j = Yes { // $ Alert[rust/unused-value]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this was more correct as "unused variable" (because j isn't mentioned anywhere else) - but it's probably not worth worrying about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now consistent with let statements with initializers, e.g. on line 11 we have

let a = 1; // $ Alert[rust/unused-value]

@hvitved hvitved merged commit f1bff93 into github:main Aug 14, 2025
21 checks passed
@hvitved hvitved deleted the rust/if-let-chain-test branch August 14, 2025 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants