Rust: Flow through captured variables #18270
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| 1000 + i | ||
| } | ||
|
|
||
| fn sink(s: i64) { |
Check notice
Code scanning / CodeQL
Unused variable Note test
paldepind
force-pushed
the
rust-captured-variables
branch
from
2f56b3e to
215922c
Compare
paldepind
force-pushed
the
rust-captured-variables
branch
from
215922c to
94b037f
Compare
|
The QLDoc checker is complaining, but it doesn't seem to be directly about the added code and otherwise the CI is happy. |
Co-authored-by: intrigus-lgtm <[email protected]>
|
Thanks @intrigus-lgtm. I've applied the suggestions :) |
| * ```rust | ||
| * fn capture_mut() { | ||
| * let mut y = 0; | ||
| * (0..5).for_each(|| { |
| * | ||
| * a definition for `y` is inserted at the call to `for_each`. | ||
| */ | ||
| class CapturedCallDefinition extends Definition, SsaImpl::UncertainWriteDefinition { |
There was a problem hiding this comment.
Presumably this class was only introduced to improve the toString representation? I suggest moving it into SsaImpl or making it private.
| @@ -101,11 +101,19 @@ final class ParameterPosition extends TParameterPosition { | |||
| /** Holds if this position represents the `self` position. */ | |||
| predicate isSelf() { this = TSelfParameterPosition() } | |||
|
|
|||
| /** | |||
| * Holds if this position represents a reference to a lambda itself. Only | |||
There was a problem hiding this comment.
I think we should use the closure terminology in Rust instead of lambda.
| @@ -402,6 +445,37 @@ module Node { | |||
| final override string toString() { result = PostUpdateNode.super.toString() } | |||
| } | |||
|
|
|||
| private class CapturePostUpdateNode extends PostUpdateNode, CaptureNode { | |||
There was a problem hiding this comment.
Nit: Move this down after the class CaptureNode.
There was a problem hiding this comment.
What about moving CaptureNode up above PostUpdateNode instead? Having all the extensions of PostUpdateNode follow right after the abstract class makes it easier to see what it's domain is.
| or | ||
| node instanceof Node::ClosureParameterNode | ||
| or | ||
| node instanceof Node::ClosureArgumentNode |
There was a problem hiding this comment.
I don't think we should hide ClosureArgumentNodes
|
I've now added the consistency checks for captured variables. I've fixed an inconsistency, which I think occurred because async blocks capture variables (due to how we've defined capture) but was not considered a lambda expression. So I've added a case for async blocks in I've not added metric queries for the new consistency checks. |
|
DCA shows new data flow inconsistencies. I'll investigate. |
|
@hvitved Do you think it would be ok to merge the PR as-is and accept the additional data flow inconsistencies? The inconsistencies doesn't seem trivial to fix and are of the form "Node steps to itself" and occur for some phi nodes at the entries to loops when there are both captured variables and Here's one example: pub fn foo(cond: bool, names: Vec<Option<String>>) {
let default_pattern = "hello";
for name in names { // INCONSISTENCY: phi(default_pattern) | Node steps to itself
if cond {
let _ = name.unwrap_or_else(|| default_pattern.to_string());
continue;
}
}
} |
|
I've added a new test that reproduces the inconsistency and added an internal issue to track it. |
No description provided.