Skip to content
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

Warn about unused captured variables #72465

Merged
merged 5 commits into from
May 29, 2020
Merged

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented May 22, 2020

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.

@rust-highfive
Copy link
Collaborator

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 rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 22, 2020
@petrochenkov
Copy link
Contributor

r? @nikomatsakis for review or reassignment

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this... false? Oh, I guess it's true because of the move

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we can provide some 'help" text or something about that

Copy link
Contributor

Choose a reason for hiding this comment

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

can we add a test like this, except that the closure is not move?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the reason that we get no warning here because we assume the closure may be called again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@tmiasko
Copy link
Contributor Author

tmiasko commented May 22, 2020

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 tmiasko force-pushed the liveness-upvars branch 2 times, most recently from aa7c766 to 0d3e204 Compare May 24, 2020 16:48
@bors
Copy link
Contributor

bors commented May 25, 2020

☔ The latest upstream changes (presumably #72562) made this pull request unmergeable. Please resolve the merge conflicts.

@tmiasko tmiasko force-pushed the liveness-upvars branch from 0d3e204 to 74697f6 Compare May 25, 2020 15:05
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

r=me with comment, nits fixed

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

pre-existing, I guess, but these comments seem out of date

@@ -647,9 +638,8 @@ impl RWUTable {

#[derive(Copy, Clone)]
struct Specials {
Copy link
Contributor

Choose a reason for hiding this comment

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

Pre-existing, but if we're going to be extending this code, can we add a doc-comment explaining what a "special" is.

) -> LiveNode {
debug!("compute: using id for body, {:?}", body.value);

let mut only_by_ref_upvars = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment here would be nice --

  • why do we care if we only have by-ref upvars?
  • what is the role of the self.acc below (presumably so that we assume that by-ref upvars are used after the closure completes)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed only_by_ref_upvars. In retrospect it wasn't useful since in that case the computation converges immediately anyway.

param.pat.each_binding(|_bm, hir_id, _x, ident| {
let var = self.variable(hir_id, ident.span);
self.define(self.s.entry_ln, var);
})
Copy link
Contributor

Choose a reason for hiding this comment

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

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;
}

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).
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved comment to the start of the function.

//
// 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

where does this logic get setup, though?

Copy link
Contributor

Choose a reason for hiding this comment

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

(maybe mention it in the comment)

@nikomatsakis
Copy link
Contributor

@bors delegate+

@bors
Copy link
Contributor

bors commented May 28, 2020

✌️ @tmiasko can now approve this pull request

@tmiasko tmiasko force-pushed the liveness-upvars branch from 74697f6 to 4dc5661 Compare May 29, 2020 15:49
@tmiasko
Copy link
Contributor Author

tmiasko commented May 29, 2020

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented May 29, 2020

📌 Commit 4dc5661 has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 29, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request May 29, 2020
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
@bors bors merged commit 9ef6227 into rust-lang:master May 29, 2020
@ltratt
Copy link
Contributor

ltratt commented Jun 1, 2020

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?

@tmiasko
Copy link
Contributor Author

tmiasko commented Jun 1, 2020

@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 tmiasko deleted the liveness-upvars branch June 1, 2020 15:29
@ltratt
Copy link
Contributor

ltratt commented Jun 1, 2020

@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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
6 participants