-
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
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.
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
let
expressions.Before this PR, we only handled
let
expressions that occurred immediately under anif
or awhile
expression, but with this PR we make the following changes: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 handlelet
expressions in any position.Generalize the current variable resolution implementation to handle
let
expressions in any position. Care must be taken when variables are introduced inside a condition:Just like
let
statements can shadow previous definitions, so canlet
expressions, and we utilize the existing machinery for handling that. In order for the write atD
to not reach the read atH
, we model all conditions as a separate variable scopes, and then add only thethen
branches to those scopes.let
expressions (like we already do forlet
statements).DCA looks great: CFG inconsistencies are down,
rust/unused-variable
FPs removed, performance is unchanged.