Track unused mutable variables across closures

KiChjang · KiChjang · commit 5a2b590ec0d6 · 2018-04-28T01:55:25.000-07:00
diff --git a/src/librustc/ich/impls_mir.rs b/src/librustc/ich/impls_mir.rs
@@ -563,6 +563,11 @@ impl<'a, 'gcx> HashStable<StableHashingContext<'a>> for mir::Literal<'gcx> {
 
 impl_stable_hash_for!(struct mir::Location { block, statement_index });
 
+impl_stable_hash_for!(struct mir::BorrowCheckResult<'tcx> {
+    closure_requirements,
+    used_mut_upvars
+});
+
 impl_stable_hash_for!(struct mir::ClosureRegionRequirements<'tcx> {
     num_external_vids,
     outlives_requirements
diff --git a/src/librustc/mir/mod.rs b/src/librustc/mir/mod.rs
@@ -21,6 +21,7 @@ use rustc_data_structures::indexed_vec::{IndexVec, Idx};
 use rustc_data_structures::control_flow_graph::dominators::{Dominators, dominators};
 use rustc_data_structures::control_flow_graph::{GraphPredecessors, GraphSuccessors};
 use rustc_data_structures::control_flow_graph::ControlFlowGraph;
+use rustc_data_structures::small_vec::SmallVec;
 use rustc_serialize as serialize;
 use hir::def::CtorKind;
 use hir::def_id::DefId;
@@ -2043,6 +2044,12 @@ pub struct GeneratorLayout<'tcx> {
     pub fields: Vec<LocalDecl<'tcx>>,
 }
 
+#[derive(Clone, Debug, RustcEncodable, RustcDecodable)]
+pub struct BorrowCheckResult<'gcx> {
+    pub closure_requirements: Option<ClosureRegionRequirements<'gcx>>,
+    pub used_mut_upvars: SmallVec<[Field; 8]>,
+}
+
 /// After we borrow check a closure, we are left with various
 /// requirements that we have inferred between the free regions that
 /// appear in the closure's signature or on its field types.  These
diff --git a/src/librustc/ty/maps/mod.rs b/src/librustc/ty/maps/mod.rs
@@ -211,7 +211,7 @@ define_maps! { <'tcx>
 
     /// Borrow checks the function body. If this is a closure, returns
     /// additional requirements that the closure's creator must verify.
-    [] fn mir_borrowck: MirBorrowCheck(DefId) -> Option<mir::ClosureRegionRequirements<'tcx>>,
+    [] fn mir_borrowck: MirBorrowCheck(DefId) -> mir::BorrowCheckResult<'tcx>,
 
     /// Gets a complete map from all types to their inherent impls.
     /// Not meant to be used directly outside of coherence.
diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs
@@ -18,15 +18,16 @@ use rustc::infer::InferCtxt;
 use rustc::ty::{self, ParamEnv, TyCtxt};
 use rustc::ty::maps::Providers;
 use rustc::lint::builtin::UNUSED_MUT;
-use rustc::mir::{AssertMessage, BasicBlock, BorrowKind, ClearCrossCrate, Local};
-use rustc::mir::{Location, Place, Mir, Mutability, Operand, Projection, ProjectionElem};
-use rustc::mir::{Rvalue, Field, Statement, StatementKind, Terminator, TerminatorKind};
-use rustc::mir::ClosureRegionRequirements;
+use rustc::mir::{AssertMessage, AggregateKind, BasicBlock, BorrowCheckResult, BorrowKind};
+use rustc::mir::{ClearCrossCrate, Local, Location, Place, Mir, Mutability, Operand};
+use rustc::mir::{Projection, ProjectionElem, Rvalue, Field, Statement, StatementKind};
+use rustc::mir::{Terminator, TerminatorKind};
 
 use rustc_data_structures::control_flow_graph::dominators::Dominators;
 use rustc_data_structures::fx::FxHashSet;
 use rustc_data_structures::indexed_set::IdxSetBuf;
 use rustc_data_structures::indexed_vec::Idx;
+use rustc_data_structures::small_vec::SmallVec;
 
 use std::rc::Rc;
 
@@ -70,12 +71,15 @@ pub fn provide(providers: &mut Providers) {
 fn mir_borrowck<'a, 'tcx>(
     tcx: TyCtxt<'a, 'tcx, 'tcx>,
     def_id: DefId,
-) -> Option<ClosureRegionRequirements<'tcx>> {
+) -> BorrowCheckResult<'tcx> {
     let input_mir = tcx.mir_validated(def_id);
     debug!("run query mir_borrowck: {}", tcx.item_path_str(def_id));
 
     if !tcx.has_attr(def_id, "rustc_mir_borrowck") && !tcx.use_mir_borrowck() {
-        return None;
+        return BorrowCheckResult {
+            closure_requirements: None,
+            used_mut_upvars: SmallVec::new(),
+        };
     }
 
     let opt_closure_req = tcx.infer_ctxt().enter(|infcx| {
@@ -91,7 +95,7 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>(
     infcx: &InferCtxt<'a, 'gcx, 'tcx>,
     input_mir: &Mir<'gcx>,
     def_id: DefId,
-) -> Option<ClosureRegionRequirements<'gcx>> {
+) -> BorrowCheckResult<'gcx> {
     let tcx = infcx.tcx;
     let attributes = tcx.get_attrs(def_id);
     let param_env = tcx.param_env(def_id);
@@ -239,6 +243,7 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>(
         moved_error_reported: FxHashSet(),
         nonlexical_regioncx: opt_regioncx,
         used_mut: FxHashSet(),
+        used_mut_upvars: SmallVec::new(),
         nonlexical_cause_info: None,
         borrow_set,
         dominators,
@@ -254,7 +259,28 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>(
 
     mbcx.analyze_results(&mut state); // entry point for DataflowResultsConsumer
 
-    opt_closure_req
+    debug!("mbcx.used_mut: {:?}", mbcx.used_mut);
+
+    for local in mbcx.mir.mut_vars_iter().filter(|local| !mbcx.used_mut.contains(local)) {
+        if let ClearCrossCrate::Set(ref vsi) = mbcx.mir.visibility_scope_info {
+            let source_info = mbcx.mir.local_decls[local].source_info;
+            let mut_span = tcx.sess.codemap().span_until_non_whitespace(source_info.span);
+
+            tcx.struct_span_lint_node(
+                UNUSED_MUT,
+                vsi[source_info.scope].lint_root,
+                source_info.span,
+                "variable does not need to be mutable"
+            )
+            .span_suggestion_short(mut_span, "remove this `mut`", "".to_owned())
+            .emit();
+        }
+    }
+
+    BorrowCheckResult {
+        closure_requirements: opt_closure_req,
+        used_mut_upvars: mbcx.used_mut_upvars,
+    }
 }
 
 #[allow(dead_code)]
@@ -292,6 +318,9 @@ pub struct MirBorrowckCtxt<'cx, 'gcx: 'tcx, 'tcx: 'cx> {
     /// This field keeps track of all the local variables that are declared mut and are mutated.
     /// Used for the warning issued by an unused mutable local variable.
     used_mut: FxHashSet<Local>,
+    /// If the function we're checking is a closure, then we'll need to report back the list of
+    /// mutable upvars that have been used. This field keeps track of them.
+    used_mut_upvars: SmallVec<[Field; 8]>,
     /// Non-lexical region inference context, if NLL is enabled.  This
     /// contains the results from region inference and lets us e.g.
     /// find out which CFG points are contained in each borrow region.
@@ -439,22 +468,6 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx
 
         self.check_activations(location, span, flow_state);
 
-        for local in self.mir.mut_vars_iter().filter(|local| !self.used_mut.contains(local)) {
-            if let ClearCrossCrate::Set(ref vsi) = self.mir.visibility_scope_info {
-                let source_info = self.mir.local_decls[local].source_info;
-                let mut_span = self.tcx.sess.codemap().span_until_non_whitespace(source_info.span);
-
-                self.tcx.struct_span_lint_node(
-                    UNUSED_MUT,
-                    vsi[source_info.scope].lint_root,
-                    source_info.span,
-                    "variable does not need to be mutable"
-                )
-                .span_suggestion_short(mut_span, "remove this `mut`", "".to_owned())
-                .emit();
-            }
-        }
-
         match term.kind {
             TerminatorKind::SwitchInt {
                 ref discr,
@@ -1118,9 +1131,33 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                 // `NullOp::Box`?
             }
 
-            Rvalue::Aggregate(ref _aggregate_kind, ref operands) => for operand in operands {
-                self.consume_operand(context, (operand, span), flow_state);
-            },
+            Rvalue::Aggregate(ref aggregate_kind, ref operands) => {
+                // We need to report back the list of mutable upvars that were
+                // moved into the closure and subsequently used by the closure,
+                // in order to populate our used_mut set.
+                if let AggregateKind::Closure(def_id, _) = &**aggregate_kind {
+                    let BorrowCheckResult { used_mut_upvars, .. } = self.tcx.mir_borrowck(*def_id);
+                    for field in used_mut_upvars {
+                        match operands[field.index()] {
+                            Operand::Move(Place::Local(local)) => {
+                                self.used_mut.insert(local);
+                            }
+                            Operand::Move(Place::Projection(ref proj)) => {
+                                if let Some(field) = self.is_upvar_field_projection(&proj.base) {
+                                    self.used_mut_upvars.push(field);
+                                }
+                            }
+                            Operand::Move(Place::Static(..)) |
+                            Operand::Copy(..) |
+                            Operand::Constant(..) => {}
+                        }
+                    }
+                }
+
+                for operand in operands {
+                    self.consume_operand(context, (operand, span), flow_state);
+                }
+            }
         }
     }
 
@@ -1652,8 +1689,16 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                 err.emit();
             },
             Reservation(WriteKind::Mutate) | Write(WriteKind::Mutate) => {
-                if let Place::Local(local) = *place {
-                    self.used_mut.insert(local);
+                match place {
+                    Place::Local(local) => {
+                        self.used_mut.insert(*local);
+                    }
+                    Place::Projection(ref proj) => {
+                        if let Some(field) = self.is_upvar_field_projection(&proj.base) {
+                            self.used_mut_upvars.push(field);
+                        }
+                    }
+                    Place::Static(..) => {}
                 }
                 if let Err(place_err) = self.is_mutable(place, is_local_mutation_allowed) {
                     error_reported = true;
diff --git a/src/librustc_mir/borrow_check/nll/type_check/mod.rs b/src/librustc_mir/borrow_check/nll/type_check/mod.rs
@@ -1426,7 +1426,9 @@ impl<'a, 'gcx, 'tcx> TypeChecker<'a, 'gcx, 'tcx> {
             // these extra requirements are basically like where
             // clauses on the struct.
             AggregateKind::Closure(def_id, substs) => {
-                if let Some(closure_region_requirements) = tcx.mir_borrowck(*def_id) {
+                if let Some(closure_region_requirements) =
+                    tcx.mir_borrowck(*def_id).closure_requirements
+                {
                     closure_region_requirements.apply_requirements(
                         self.infcx,
                         self.body_id,
diff --git a/src/test/compile-fail/lint-unused-mut-variables.rs b/src/test/compile-fail/lint-unused-mut-variables.rs
@@ -8,6 +8,9 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
+// revisions: lexical nll
+#![cfg_attr(nll, feature(nll))]
+
 // Exercise the unused_mut attribute in some positive and negative cases
 
 #![allow(unused_assignments)]
@@ -18,53 +21,68 @@
 
 fn main() {
     // negative cases
-    let mut a = 3; //~ ERROR: variable does not need to be mutable
-    let mut a = 2; //~ ERROR: variable does not need to be mutable
-    let mut b = 3; //~ ERROR: variable does not need to be mutable
-    let mut a = vec![3]; //~ ERROR: variable does not need to be mutable
-    let (mut a, b) = (1, 2); //~ ERROR: variable does not need to be mutable
-    let mut a; //~ ERROR: variable does not need to be mutable
+    let mut a = 3; //[lexical]~ ERROR: variable does not need to be mutable
+                   //[nll]~^ ERROR: variable does not need to be mutable
+    let mut a = 2; //[lexical]~ ERROR: variable does not need to be mutable
+                   //[nll]~^ ERROR: variable does not need to be mutable
+    let mut b = 3; //[lexical]~ ERROR: variable does not need to be mutable
+                   //[nll]~^ ERROR: variable does not need to be mutable
+    let mut a = vec![3]; //[lexical]~ ERROR: variable does not need to be mutable
+                         //[nll]~^ ERROR: variable does not need to be mutable
+    let (mut a, b) = (1, 2); //[lexical]~ ERROR: variable does not need to be mutable
+                             //[nll]~^ ERROR: variable does not need to be mutable
+    let mut a; //[lexical]~ ERROR: variable does not need to be mutable
+               //[nll]~^ ERROR: variable does not need to be mutable
     a = 3;
 
-    let mut b; //~ ERROR: variable does not need to be mutable
+    let mut b; //[lexical]~ ERROR: variable does not need to be mutable
+               //[nll]~^ ERROR: variable does not need to be mutable
     if true {
         b = 3;
     } else {
         b = 4;
     }
 
     match 30 {
-        mut x => {} //~ ERROR: variable does not need to be mutable
+        mut x => {} //[lexical]~ ERROR: variable does not need to be mutable
+                    //[nll]~^ ERROR: variable does not need to be mutable
     }
     match (30, 2) {
-      (mut x, 1) | //~ ERROR: variable does not need to be mutable
+      (mut x, 1) | //[lexical]~ ERROR: variable does not need to be mutable
+                   //[nll]~^ ERROR: variable does not need to be mutable
       (mut x, 2) |
       (mut x, 3) => {
       }
       _ => {}
     }
 
-    let x = |mut y: isize| 10; //~ ERROR: variable does not need to be mutable
-    fn what(mut foo: isize) {} //~ ERROR: variable does not need to be mutable
+    let x = |mut y: isize| 10; //[lexical]~ ERROR: variable does not need to be mutable
+                               //[nll]~^ ERROR: variable does not need to be mutable
+    fn what(mut foo: isize) {} //[lexical]~ ERROR: variable does not need to be mutable
+                               //[nll]~^ ERROR: variable does not need to be mutable
 
-    let mut a = &mut 5; //~ ERROR: variable does not need to be mutable
+    let mut a = &mut 5; //[lexical]~ ERROR: variable does not need to be mutable
+                        //[nll]~^ ERROR: variable does not need to be mutable
     *a = 4;
 
     let mut a = 5;
-    let mut b = (&mut a,);
-    *b.0 = 4; //~^ ERROR: variable does not need to be mutable
+    let mut b = (&mut a,); //[lexical]~ ERROR: variable does not need to be mutable
+    *b.0 = 4;              //[nll]~^ ERROR: variable does not need to be mutable
 
-    let mut x = &mut 1; //~ ERROR: variable does not need to be mutable
+    let mut x = &mut 1; //[lexical]~ ERROR: variable does not need to be mutable
+                        //[nll]~^ ERROR: variable does not need to be mutable
     let mut f = || {
       *x += 1;
     };
     f();
 
     fn mut_ref_arg(mut arg : &mut [u8]) -> &mut [u8] {
-        &mut arg[..] //~^ ERROR: variable does not need to be mutable
+        &mut arg[..] //[lexical]~^ ERROR: variable does not need to be mutable
+                     //[nll]~^^ ERROR: variable does not need to be mutable
     }
 
-    let mut v : &mut Vec<()> = &mut vec![]; //~ ERROR: variable does not need to be mutable
+    let mut v : &mut Vec<()> = &mut vec![]; //[lexical]~ ERROR: variable does not need to be mutable
+                                            //[nll]~^ ERROR: variable does not need to be mutable
     v.push(());
 
     // positive cases
@@ -76,6 +94,12 @@ fn main() {
     callback(|| {
         a.push(3);
     });
+    let mut a = Vec::new();
+    callback(|| {
+        callback(|| {
+            a.push(3);
+        });
+    });
     let (mut a, b) = (1, 2);
     a = 34;
 
@@ -116,5 +140,6 @@ fn foo(mut a: isize) {
 fn bar() {
     #[allow(unused_mut)]
     let mut a = 3;
-    let mut b = vec![2]; //~ ERROR: variable does not need to be mutable
+    let mut b = vec![2]; //[lexical]~ ERROR: variable does not need to be mutable
+                         //[nll]~^ ERROR: variable does not need to be mutable
 }
diff --git a/src/test/run-pass/borrowck/borrowck-unused-mut-locals.rs b/src/test/run-pass/borrowck/borrowck-unused-mut-locals.rs
