-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Conversation
def7f7a to
3994566
Compare
ba623c6 to
450b458
Compare
let expressions
423d2b4 to
3659ff5
Compare
| * 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
3659ff5 to
f63e55c
Compare
There was a problem hiding this 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
letexpressions in post-order instead of pre-order - Extends variable resolution to handle
letexpressions with proper scoping and shadowing rules - Adds data flow propagation from RHS to LHS for
letexpressions
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.
There was a problem hiding this 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] | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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]
This PR adds support for chained
letexpressions.Before this PR, we only handled
letexpressions that occurred immediately under anifor awhileexpression, but with this PR we make the following changes:Generalize the control flow graph implementation to handle
letexpressions like other expressions (in post-order instead of pre-order), which means that the CFG implementation should be able to handleletexpressions in any position.Generalize the current variable resolution implementation to handle
letexpressions in any position. Care must be taken when variables are introduced inside a condition:Just like
letstatements can shadow previous definitions, so canletexpressions, and we utilize the existing machinery for handling that. In order for the write atDto not reach the read atH, we model all conditions as a separate variable scopes, and then add only thethenbranches to those scopes.letexpressions (like we already do forletstatements).DCA looks great: CFG inconsistencies are down,
rust/unused-variableFPs removed, performance is unchanged.