Warn about unused captured variables #72465
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
|
r? @nikomatsakis for review or reassignment |
There was a problem hiding this comment.
Did a quick skim, just of the test cases, and left a few comments. What would be really great is if there was some kind of write-up explaining how this extension to the new analysis works -- not so much the details of the impl, but the high-level idea of when I should expect warnings and so forth. e.g., does it assume closures might be called than once? Does it try to track which closures are actually called? If they are FnOnce, does it still assume that? etc
| let mut last = None; | ||
| let mut f = move |s| { | ||
| last = Some(s); //~ WARN value assigned to `last` is never read | ||
| //~| WARN variable `last` is assigned to, but never used |
There was a problem hiding this comment.
isn't this... false? Oh, I guess it's true because of the move
There was a problem hiding this comment.
I wonder if we can provide some 'help" text or something about that
There was a problem hiding this comment.
can we add a test like this, except that the closure is not move?
There was a problem hiding this comment.
As for an additional explanation, maybe something along the lines of: "variable captured by value, did you mean to capture by reference instead?", possibly highlighting the "move" part of closure.
| || { | ||
| move || { | ||
| println!("{}", c); | ||
| c += 1; |
There was a problem hiding this comment.
Is the reason that we get no warning here because we assume the closure may be called again?
There was a problem hiding this comment.
Yes. The store is not dead because it might be read by the closure when called again. I added similar test cases for FnOnce and generators to show the difference.
|
There is one longer comment trying to give high-level idea behind the analysis. The computation is still about what happens within a single function / closure. Whether closure is actually invoked is outside the consideration. Analysis assumes that variables captured by reference will be used after leaving the closure. For variables captured by value it takes into account the closure kind. In FnOnce closures, variables captured by value are presumed dead after exit. In Fn / FnMut closures, it assumes that variables captured by value might be accessed again, but only by the closure itself on subsequent invocations. For variables captured by value, is also presumes that the initial captured value should be used and warns otherwise. |
tmiasko
force-pushed
the
liveness-upvars
branch
2 times, most recently
from
aa7c766
to
0d3e204
Compare
|
☔ The latest upstream changes (presumably #72562) made this pull request unmergeable. Please resolve the merge conflicts. |
src/librustc_passes/liveness.rs
Outdated
| @@ -677,11 +667,8 @@ impl<'a, 'tcx> Liveness<'a, 'tcx> { | |||
| // - exit_ln represents the end of the fn, either by return or panic | |||
| // - implicit_ret_var is a pseudo-variable that represents | |||
There was a problem hiding this comment.
pre-existing, I guess, but these comments seem out of date
| @@ -647,9 +638,8 @@ impl RWUTable { | |||
|
|
|||
| #[derive(Copy, Clone)] | |||
| struct Specials { | |||
There was a problem hiding this comment.
Pre-existing, but if we're going to be extending this code, can we add a doc-comment explaining what a "special" is.
src/librustc_passes/liveness.rs
Outdated
| ) -> LiveNode { | ||
| debug!("compute: using id for body, {:?}", body.value); | ||
|
|
||
| let mut only_by_ref_upvars = true; |
There was a problem hiding this comment.
Comment here would be nice --
- why do we care if we only have by-ref upvars?
- what is the role of the
self.accbelow (presumably so that we assume that by-ref upvars are used after the closure completes)
There was a problem hiding this comment.
Removed only_by_ref_upvars. In retrospect it wasn't useful since in that case the computation converges immediately anyway.
src/librustc_passes/liveness.rs
Outdated
| param.pat.each_binding(|_bm, hir_id, _x, ident| { | ||
| let var = self.variable(hir_id, ident.span); | ||
| self.define(self.s.entry_ln, var); | ||
| }) |
There was a problem hiding this comment.
can't we make this more dry by doing something like
let mut first_merge = true;
loop {
self.init_from_succ(self.s.entry_ln, succ);
for param in body.params {
param.pat.each_binding(|_bm, hir_id, _x, ident| {
let var = self.variable(hir_id, ident.span);
self.define(self.s.entry_ln, var);
})
if !self.merge_from_succ(...) { break; }
first_merge = false;
}
src/librustc_passes/liveness.rs
Outdated
| let entry_ln = self.propagate_through_expr(body, s.fallthrough_ln); | ||
| // When computing the liveness for captured variables we take into | ||
| // account how variable is captured (ByRef vs ByValue) and what is the | ||
| // closure kind (Generator / FnOnce vs Fn / FnMut). |
There was a problem hiding this comment.
this comment captures some of the points I raised above, it just came later so I wouldn't have seen it yet while reading the code
There was a problem hiding this comment.
Moved comment to the start of the function.
src/librustc_passes/liveness.rs
Outdated
| // | ||
| // In Fn / FnMut closures, variables captured by value are live on exit | ||
| // if they are live on the entry to the closure, since only the closure | ||
| // itself can access them on subsequent invocations. |
There was a problem hiding this comment.
where does this logic get setup, though?
There was a problem hiding this comment.
(maybe mention it in the comment)
|
@bors delegate+ |
|
✌️ @tmiasko can now approve this pull request |
|
@bors r=nikomatsakis |
|
📌 Commit 4dc5661 has been approved by |
bors
added
S-waiting-on-bors
Rollup of 9 pull requests Successful merges: - rust-lang#72310 (Add Peekable::next_if) - rust-lang#72383 (Suggest using std::mem::drop function instead of explicit destructor call) - rust-lang#72398 (SocketAddr and friends now correctly pad its content) - rust-lang#72465 (Warn about unused captured variables) - rust-lang#72568 (Implement total_cmp for f32, f64) - rust-lang#72572 (Add some regression tests) - rust-lang#72591 (librustc_middle: Rename upvar_list to closure_captures) - rust-lang#72701 (Fix grammar in liballoc raw_vec) - rust-lang#72731 (Add missing empty line in E0619 explanation) Failed merges: r? @ghost
|
I might be doing something wrong, but this PR doesn't seem to produce a warning for the example I had in #63220 (comment). Perhaps it's because the captured variable is used? |
|
@ltratt println inside the closure uses the captured variable, so there will be no warning. It will warn when println is removed, or about the real bug linked from that comment. |
|
@tmiasko You're quite right! I tried the "real bug" link and it gives the "unused variable" warning. Thank much for fixing this problem and for pointing out that I'd checked the wrong thing :) |
Include captured variables in liveness analysis. Warn when captured variables
are unused (but possibly read or written to). Warn about dead assignments to
captured variables.
Fixes #37707.
Fixes #47128.
Fixes #63220.