From 2338adf48cbb6bfcf03c5f00f48ee63014d793cd Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Wed, 28 Feb 2018 01:09:08 -0800 Subject: [PATCH 1/9] Allow MIR borrowck to catch unused mutable locals --- src/librustc/mir/mod.rs | 14 +++++++++++ src/librustc_mir/borrow_check/mod.rs | 37 ++++++++++++++++++++++------ 2 files changed, 44 insertions(+), 7 deletions(-) diff --git a/src/librustc/mir/mod.rs b/src/librustc/mir/mod.rs index c525c4ed651f2..501f77547e282 100644 --- a/src/librustc/mir/mod.rs +++ b/src/librustc/mir/mod.rs @@ -247,6 +247,20 @@ impl<'tcx> Mir<'tcx> { }) } + /// Returns an iterator over all user-declared mutable locals. + #[inline] + pub fn mut_vars_iter<'a>(&'a self) -> impl Iterator + 'a { + (self.arg_count+1..self.local_decls.len()).filter_map(move |index| { + let local = Local::new(index); + let decl = &self.local_decls[local]; + if decl.is_user_variable && decl.mutability == Mutability::Mut { + Some(local) + } else { + None + } + }) + } + /// Returns an iterator over all function arguments. #[inline] pub fn args_iter(&self) -> impl Iterator { diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index 5c7061abbb660..6cebd290b9a25 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -17,10 +17,11 @@ use rustc::hir::map::definitions::DefPathData; use rustc::infer::InferCtxt; use rustc::ty::{self, ParamEnv, TyCtxt}; use rustc::ty::maps::Providers; -use rustc::mir::{AssertMessage, BasicBlock, BorrowKind, Location, Place}; -use rustc::mir::{Mir, Mutability, Operand, Projection, ProjectionElem, Rvalue}; -use rustc::mir::{Field, Statement, StatementKind, Terminator, TerminatorKind}; -use rustc::mir::{ClosureRegionRequirements, Local}; +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_data_structures::control_flow_graph::dominators::Dominators; use rustc_data_structures::fx::FxHashSet; @@ -236,7 +237,8 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>( access_place_error_reported: FxHashSet(), reservation_error_reported: FxHashSet(), moved_error_reported: FxHashSet(), - nonlexical_regioncx: regioncx, + nonlexical_regioncx: opt_regioncx, + used_mut: FxHashSet(), nonlexical_cause_info: None, borrow_set, dominators, @@ -287,6 +289,9 @@ pub struct MirBorrowckCtxt<'cx, 'gcx: 'tcx, 'tcx: 'cx> { /// This field keeps track of errors reported in the checking of moved variables, /// so that we don't report report seemingly duplicate errors. moved_error_reported: FxHashSet>, + /// 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, /// 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. @@ -434,6 +439,22 @@ 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, @@ -1594,7 +1615,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { /// /// Returns true if an error is reported, false otherwise. fn check_access_permissions( - &self, + &mut self, (place, span): (&Place<'tcx>, Span), kind: ReadOrWrite, is_local_mutation_allowed: LocalMutationIsAllowed, @@ -1631,7 +1652,9 @@ 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); + } if let Err(place_err) = self.is_mutable(place, is_local_mutation_allowed) { error_reported = true; let mut err_info = None; From 3e423d05863ec7c02f6e1efebed5480a3211755e Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Thu, 1 Mar 2018 21:14:36 -0800 Subject: [PATCH 2/9] Disable AST unused mut check when using MIR borrowck --- src/librustc_borrowck/borrowck/mod.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/librustc_borrowck/borrowck/mod.rs b/src/librustc_borrowck/borrowck/mod.rs index 6d832d4060a1f..519dd574e5afe 100644 --- a/src/librustc_borrowck/borrowck/mod.rs +++ b/src/librustc_borrowck/borrowck/mod.rs @@ -144,7 +144,10 @@ fn borrowck<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, owner_def_id: DefId) { check_loans::check_loans(&mut bccx, &loan_dfcx, &flowed_moves, &all_loans, body); } - unused::check(&mut bccx, body); + + if !tcx.use_mir_borrowck() { + unused::check(&mut bccx, body); + } Lrc::new(BorrowCheckResult { used_mut_nodes: bccx.used_mut_nodes.into_inner(), From 5a2b590ec0d6f1de5e44c00adb6638c05da6a265 Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Fri, 2 Mar 2018 20:42:37 -0800 Subject: [PATCH 3/9] Track unused mutable variables across closures --- src/librustc/ich/impls_mir.rs | 5 + src/librustc/mir/mod.rs | 7 ++ src/librustc/ty/maps/mod.rs | 2 +- src/librustc_mir/borrow_check/mod.rs | 103 +++++++++++++----- .../borrow_check/nll/type_check/mod.rs | 4 +- .../compile-fail/lint-unused-mut-variables.rs | 61 ++++++++--- .../borrowck/borrowck-unused-mut-locals.rs | 56 ++++++++++ 7 files changed, 189 insertions(+), 49 deletions(-) create mode 100644 src/test/run-pass/borrowck/borrowck-unused-mut-locals.rs diff --git a/src/librustc/ich/impls_mir.rs b/src/librustc/ich/impls_mir.rs index c73f171806e42..437626ff5369a 100644 --- a/src/librustc/ich/impls_mir.rs +++ b/src/librustc/ich/impls_mir.rs @@ -563,6 +563,11 @@ impl<'a, 'gcx> HashStable> 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 index 501f77547e282..7b6389072b7b2 100644 --- 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>, } +#[derive(Clone, Debug, RustcEncodable, RustcDecodable)] +pub struct BorrowCheckResult<'gcx> { + pub closure_requirements: Option>, + 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 index 57223a3c7b274..d89846a75ef51 100644 --- 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>, + [] 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 index 6cebd290b9a25..ea628cefd3e1c 100644 --- 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> { +) -> 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> { +) -> 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, + /// 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 index 0ed95a319f7de..a811b2c147e98 100644 --- 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 index 3c76740d2b5dd..14d836074dca3 100644 --- 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,15 +21,22 @@ 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 { @@ -34,37 +44,45 @@ fn main() { } 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 new file mode 100644 index 0000000000000..7f1b6ed170168 --- /dev/null +++ b/src/test/run-pass/borrowck/borrowck-unused-mut-locals.rs @@ -0,0 +1,56 @@ +// Copyright 2015 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#![feature(nll)] +#![deny(unused_mut)] + +#[derive(Debug)] +struct A {} + +fn init_a() -> A { + A {} +} + +#[derive(Debug)] +struct B<'a> { + ed: &'a mut A, +} + +fn init_b<'a>(ed: &'a mut A) -> B<'a> { + B { ed } +} + +#[derive(Debug)] +struct C<'a> { + pd: &'a mut B<'a>, +} + +fn init_c<'a>(pd: &'a mut B<'a>) -> C<'a> { + C { pd } +} + +#[derive(Debug)] +struct D<'a> { + sd: &'a mut C<'a>, +} + +fn init_d<'a>(sd: &'a mut C<'a>) -> D<'a> { + D { sd } +} + +fn main() { + let mut a = init_a(); + let mut b = init_b(&mut a); + let mut c = init_c(&mut b); + + let d = init_d(&mut c); + + println!("{:?}", d) +} From e3b372f67d9fbfda2818febe3af914d4f4069c6d Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Mon, 12 Mar 2018 08:47:44 -0700 Subject: [PATCH 4/9] Only check possibly initialized values and also loop over fn args --- src/librustc/mir/mod.rs | 10 ++++++---- src/librustc_mir/borrow_check/mod.rs | 23 +++++++++++++++++++---- 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/src/librustc/mir/mod.rs b/src/librustc/mir/mod.rs index 7b6389072b7b2..c26b3014e53dd 100644 --- a/src/librustc/mir/mod.rs +++ b/src/librustc/mir/mod.rs @@ -248,13 +248,15 @@ impl<'tcx> Mir<'tcx> { }) } - /// Returns an iterator over all user-declared mutable locals. + /// Returns an iterator over all user-declared mutable arguments and locals. #[inline] - pub fn mut_vars_iter<'a>(&'a self) -> impl Iterator + 'a { - (self.arg_count+1..self.local_decls.len()).filter_map(move |index| { + pub fn mut_vars_and_args_iter<'a>(&'a self) -> impl Iterator + 'a { + (1..self.local_decls.len()).filter_map(move |index| { let local = Local::new(index); let decl = &self.local_decls[local]; - if decl.is_user_variable && decl.mutability == Mutability::Mut { + if (decl.is_user_variable || index < self.arg_count + 1) + && decl.mutability == Mutability::Mut + { Some(local) } else { None diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index ea628cefd3e1c..60691d1179a68 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -261,9 +261,17 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>( debug!("mbcx.used_mut: {:?}", mbcx.used_mut); - for local in mbcx.mir.mut_vars_iter().filter(|local| !mbcx.used_mut.contains(local)) { + for local in mbcx.mir.mut_vars_and_args_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 local_decl = &mbcx.mir.local_decls[local]; + + // Skip over locals that begin with an underscore + match local_decl.name { + Some(name) if name.as_str().starts_with("_") => continue, + _ => {}, + } + + let source_info = local_decl.source_info; let mut_span = tcx.sess.codemap().span_until_non_whitespace(source_info.span); tcx.struct_span_lint_node( @@ -864,7 +872,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { } let mutability_error = - self.check_access_permissions(place_span, rw, is_local_mutation_allowed); + self.check_access_permissions(place_span, rw, is_local_mutation_allowed, flow_state); let conflict_error = self.check_access_for_conflict(context, place_span, sd, rw, flow_state); @@ -1656,6 +1664,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { (place, span): (&Place<'tcx>, Span), kind: ReadOrWrite, is_local_mutation_allowed: LocalMutationIsAllowed, + flow_state: &Flows<'cx, 'gcx, 'tcx>, ) -> bool { debug!( "check_access_permissions({:?}, {:?}, {:?})", @@ -1691,7 +1700,13 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { Reservation(WriteKind::Mutate) | Write(WriteKind::Mutate) => { match place { Place::Local(local) => { - self.used_mut.insert(*local); + // If the local may be initialized, and it is now currently being + // mutated, then it is justified to be annotated with the `mut` keyword, + // since the mutation may be a possible reassignment. + let mpi = self.move_data.rev_lookup.find_local(*local); + if flow_state.inits.contains(&mpi) { + self.used_mut.insert(*local); + } } Place::Projection(ref proj) => { if let Some(field) = self.is_upvar_field_projection(&proj.base) { From 71d221f29425bf7c87fbc781fcbc5740b00d1c8f Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Fri, 16 Mar 2018 02:52:07 -0700 Subject: [PATCH 5/9] Determine unused mutable variables based on the 'root' place --- src/librustc_mir/borrow_check/mod.rs | 197 +++++++++++++++------------ 1 file changed, 111 insertions(+), 86 deletions(-) diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index 60691d1179a68..2498162d10fa1 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -241,7 +241,7 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>( access_place_error_reported: FxHashSet(), reservation_error_reported: FxHashSet(), moved_error_reported: FxHashSet(), - nonlexical_regioncx: opt_regioncx, + nonlexical_regioncx: regioncx, used_mut: FxHashSet(), used_mut_upvars: SmallVec::new(), nonlexical_cause_info: None, @@ -276,7 +276,7 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>( tcx.struct_span_lint_node( UNUSED_MUT, - vsi[source_info.scope].lint_root, + vsi[local_decl.syntactic_scope].lint_root, source_info.span, "variable does not need to be mutable" ) @@ -1366,7 +1366,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { ) { debug!("check_if_reassignment_to_immutable_state({:?})", place); // determine if this path has a non-mut owner (and thus needs checking). - if let Ok(()) = self.is_mutable(place, LocalMutationIsAllowed::No) { + if let Ok(_) = self.is_mutable(place, LocalMutationIsAllowed::No) { return; } debug!( @@ -1679,27 +1679,11 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { } } Reservation(WriteKind::MutableBorrow(BorrowKind::Mut { .. })) - | Write(WriteKind::MutableBorrow(BorrowKind::Mut { .. })) => if let Err(place_err) = - self.is_mutable(place, is_local_mutation_allowed) - { - error_reported = true; - let item_msg = self.get_default_err_msg(place); - let mut err = self.tcx - .cannot_borrow_path_as_mutable(span, &item_msg, Origin::Mir); - err.span_label(span, "cannot borrow as mutable"); - - if place != place_err { - if let Some(name) = self.describe_place(place_err) { - err.note(&format!("the value which is causing this path not to be mutable \ - is...: `{}`", name)); - } - } - - err.emit(); - }, - Reservation(WriteKind::Mutate) | Write(WriteKind::Mutate) => { - match place { - Place::Local(local) => { + | Write(WriteKind::MutableBorrow(BorrowKind::Mut { .. })) => { + match self.is_mutable(place, is_local_mutation_allowed) { + Ok(Place::Local(local)) + if is_local_mutation_allowed != LocalMutationIsAllowed::Yes => + { // If the local may be initialized, and it is now currently being // mutated, then it is justified to be annotated with the `mut` keyword, // since the mutation may be a possible reassignment. @@ -1708,77 +1692,118 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { self.used_mut.insert(*local); } } - Place::Projection(ref proj) => { + Ok(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; - let mut err_info = None; - match *place_err { - - Place::Projection(box Projection { - ref base, elem:ProjectionElem::Deref}) => { - match *base { - Place::Local(local) => { - let locations = self.mir.find_assignments(local); - if locations.len() > 0 { - let item_msg = if error_reported { - self.get_secondary_err_msg(base) - } else { - self.get_default_err_msg(place) - }; - let sp = self.mir.source_info(locations[0]).span; - let mut to_suggest_span = String::new(); - if let Ok(src) = - self.tcx.sess.codemap().span_to_snippet(sp) { - to_suggest_span = src[1..].to_string(); - }; - err_info = Some(( - sp, - "consider changing this to be a \ - mutable reference", - to_suggest_span, - item_msg, - self.get_primary_err_msg(base))); - } - }, - _ => {}, - } - }, - _ => {}, - } + Ok(Place::Local(_)) | + Ok(Place::Static(..)) => {} + Err(place_err) => { + error_reported = true; + let item_msg = self.get_default_err_msg(place); + let mut err = self.tcx + .cannot_borrow_path_as_mutable(span, &item_msg, Origin::Mir); + err.span_label(span, "cannot borrow as mutable"); - if let Some((err_help_span, - err_help_stmt, - to_suggest_span, - item_msg, - sec_span)) = err_info { - let mut err = self.tcx.cannot_assign(span, &item_msg, Origin::Mir); - err.span_suggestion(err_help_span, - err_help_stmt, - format!("&mut {}", to_suggest_span)); - if place != place_err { - err.span_label(span, sec_span); - } - err.emit() - } else { - let item_msg_ = self.get_default_err_msg(place); - let mut err = self.tcx.cannot_assign(span, &item_msg_, Origin::Mir); - err.span_label(span, "cannot mutate"); if place != place_err { if let Some(name) = self.describe_place(place_err) { err.note(&format!("the value which is causing this path not to be \ - mutable is...: `{}`", name)); + mutable is...: `{}`", name)); } } + err.emit(); } } } + Reservation(WriteKind::Mutate) | Write(WriteKind::Mutate) => { + match self.is_mutable(place, is_local_mutation_allowed) { + Ok(Place::Local(local)) + if is_local_mutation_allowed != LocalMutationIsAllowed::Yes => + { + // If the local may be initialized, and it is now currently being + // mutated, then it is justified to be annotated with the `mut` keyword, + // since the mutation may be a possible reassignment. + let mpi = self.move_data.rev_lookup.find_local(*local); + if flow_state.inits.contains(&mpi) { + self.used_mut.insert(*local); + } + } + Ok(Place::Projection(ref proj)) => { + if let Some(field) = self.is_upvar_field_projection(&proj.base) { + self.used_mut_upvars.push(field); + } + } + Ok(Place::Local(_)) | + Ok(Place::Static(..)) => {} + Err(place_err) => { + error_reported = true; + + let err_info = if let Place::Projection( + box Projection { + ref base, + elem: ProjectionElem::Deref + } + ) = *place_err { + if let Place::Local(local) = *base { + let locations = self.mir.find_assignments(local); + if locations.len() > 0 { + let item_msg = if error_reported { + self.get_secondary_err_msg(base) + } else { + self.get_default_err_msg(place) + }; + let sp = self.mir.source_info(locations[0]).span; + let mut to_suggest_span = String::new(); + if let Ok(src) = + self.tcx.sess.codemap().span_to_snippet(sp) { + to_suggest_span = src[1..].to_string(); + }; + Some((sp, + "consider changing this to be a \ + mutable reference", + to_suggest_span, + item_msg, + self.get_primary_err_msg(base))) + } else { + None + } + } else { + None + } + } else { + None + }; + + if let Some((err_help_span, + err_help_stmt, + to_suggest_span, + item_msg, + sec_span)) = err_info { + let mut err = self.tcx.cannot_assign(span, &item_msg, Origin::Mir); + err.span_suggestion(err_help_span, + err_help_stmt, + format!("&mut {}", to_suggest_span)); + if place != place_err { + err.span_label(span, sec_span); + } + err.emit() + } else { + let item_msg = self.get_default_err_msg(place); + let mut err = self.tcx.cannot_assign(span, &item_msg, Origin::Mir); + err.span_label(span, "cannot mutate"); + if place != place_err { + if let Some(name) = self.describe_place(place_err) { + err.note(&format!("the value which is causing this path not \ + to be mutable is...: `{}`", name)); + } + } + err.emit(); + } + } + } + } Reservation(WriteKind::Move) | Reservation(WriteKind::StorageDeadOrDrop) | Reservation(WriteKind::MutableBorrow(BorrowKind::Shared)) @@ -1810,25 +1835,25 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { &self, place: &'d Place<'tcx>, is_local_mutation_allowed: LocalMutationIsAllowed, - ) -> Result<(), &'d Place<'tcx>> { + ) -> Result<&'d Place<'tcx>, &'d Place<'tcx>> { match *place { Place::Local(local) => { let local = &self.mir.local_decls[local]; match local.mutability { Mutability::Not => match is_local_mutation_allowed { LocalMutationIsAllowed::Yes | LocalMutationIsAllowed::ExceptUpvars => { - Ok(()) + Ok(place) } LocalMutationIsAllowed::No => Err(place), }, - Mutability::Mut => Ok(()), + Mutability::Mut => Ok(place), } } Place::Static(ref static_) => if self.tcx.is_static(static_.def_id) != Some(hir::Mutability::MutMutable) { Err(place) } else { - Ok(()) + Ok(place) }, Place::Projection(ref proj) => { match proj.elem { @@ -1866,7 +1891,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { hir::MutImmutable => return Err(place), // `*mut` raw pointers are always mutable, regardless of context // The users have to check by themselve. - hir::MutMutable => return Ok(()), + hir::MutMutable => return Ok(place), } } // `Box` owns its content, so mutable if its location is mutable From 180e2426c90cae9362bcc68576293da7a5bdd22a Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Tue, 20 Mar 2018 12:58:38 -0700 Subject: [PATCH 6/9] Return LocalMutationIsAllowed in is_mutable --- src/librustc_mir/borrow_check/mod.rs | 63 ++++++++++++++-------------- 1 file changed, 32 insertions(+), 31 deletions(-) diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index 2498162d10fa1..9da5d1dcdcd4f 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -1366,7 +1366,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { ) { debug!("check_if_reassignment_to_immutable_state({:?})", place); // determine if this path has a non-mut owner (and thus needs checking). - if let Ok(_) = self.is_mutable(place, LocalMutationIsAllowed::No) { + if let Ok(..) = self.is_mutable(place, LocalMutationIsAllowed::No) { return; } debug!( @@ -1681,24 +1681,23 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { Reservation(WriteKind::MutableBorrow(BorrowKind::Mut { .. })) | Write(WriteKind::MutableBorrow(BorrowKind::Mut { .. })) => { match self.is_mutable(place, is_local_mutation_allowed) { - Ok(Place::Local(local)) - if is_local_mutation_allowed != LocalMutationIsAllowed::Yes => - { - // If the local may be initialized, and it is now currently being - // mutated, then it is justified to be annotated with the `mut` keyword, - // since the mutation may be a possible reassignment. - let mpi = self.move_data.rev_lookup.find_local(*local); - if flow_state.inits.contains(&mpi) { - self.used_mut.insert(*local); + Ok((Place::Local(local), mut_allowed)) => { + if mut_allowed != LocalMutationIsAllowed::Yes { + // If the local may be initialized, and it is now currently being + // mutated, then it is justified to be annotated with the `mut` + // keyword, since the mutation may be a possible reassignment. + let mpi = self.move_data.rev_lookup.find_local(*local); + if flow_state.inits.contains(&mpi) { + self.used_mut.insert(*local); + } } } - Ok(Place::Projection(ref proj)) => { + Ok((Place::Projection(ref proj), _mut_allowed)) => { if let Some(field) = self.is_upvar_field_projection(&proj.base) { self.used_mut_upvars.push(field); } } - Ok(Place::Local(_)) | - Ok(Place::Static(..)) => {} + Ok((Place::Static(..), _mut_allowed)) => {} Err(place_err) => { error_reported = true; let item_msg = self.get_default_err_msg(place); @@ -1719,24 +1718,23 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { } Reservation(WriteKind::Mutate) | Write(WriteKind::Mutate) => { match self.is_mutable(place, is_local_mutation_allowed) { - Ok(Place::Local(local)) - if is_local_mutation_allowed != LocalMutationIsAllowed::Yes => - { - // If the local may be initialized, and it is now currently being - // mutated, then it is justified to be annotated with the `mut` keyword, - // since the mutation may be a possible reassignment. - let mpi = self.move_data.rev_lookup.find_local(*local); - if flow_state.inits.contains(&mpi) { - self.used_mut.insert(*local); + Ok((Place::Local(local), mut_allowed)) => { + if mut_allowed != LocalMutationIsAllowed::Yes { + // If the local may be initialized, and it is now currently being + // mutated, then it is justified to be annotated with the `mut` + // keyword, since the mutation may be a possible reassignment. + let mpi = self.move_data.rev_lookup.find_local(*local); + if flow_state.inits.contains(&mpi) { + self.used_mut.insert(*local); + } } } - Ok(Place::Projection(ref proj)) => { + Ok((Place::Projection(ref proj), _mut_allowed)) => { if let Some(field) = self.is_upvar_field_projection(&proj.base) { self.used_mut_upvars.push(field); } } - Ok(Place::Local(_)) | - Ok(Place::Static(..)) => {} + Ok((Place::Static(..), _mut_allowed)) => {} Err(place_err) => { error_reported = true; @@ -1835,25 +1833,28 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { &self, place: &'d Place<'tcx>, is_local_mutation_allowed: LocalMutationIsAllowed, - ) -> Result<&'d Place<'tcx>, &'d Place<'tcx>> { + ) -> Result<(&'d Place<'tcx>, LocalMutationIsAllowed), &'d Place<'tcx>> { match *place { Place::Local(local) => { let local = &self.mir.local_decls[local]; match local.mutability { Mutability::Not => match is_local_mutation_allowed { - LocalMutationIsAllowed::Yes | LocalMutationIsAllowed::ExceptUpvars => { - Ok(place) + LocalMutationIsAllowed::Yes => { + Ok((place, LocalMutationIsAllowed::Yes)) + } + LocalMutationIsAllowed::ExceptUpvars => { + Ok((place, LocalMutationIsAllowed::ExceptUpvars)) } LocalMutationIsAllowed::No => Err(place), }, - Mutability::Mut => Ok(place), + Mutability::Mut => Ok((place, is_local_mutation_allowed)), } } Place::Static(ref static_) => if self.tcx.is_static(static_.def_id) != Some(hir::Mutability::MutMutable) { Err(place) } else { - Ok(place) + Ok((place, is_local_mutation_allowed)) }, Place::Projection(ref proj) => { match proj.elem { @@ -1891,7 +1892,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { hir::MutImmutable => return Err(place), // `*mut` raw pointers are always mutable, regardless of context // The users have to check by themselve. - hir::MutMutable => return Ok(place), + hir::MutMutable => return Ok((place, is_local_mutation_allowed)), } } // `Box` owns its content, so mutable if its location is mutable From 8c607eaf949a0ba9c365dd00e18d948418cfe957 Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Fri, 23 Mar 2018 01:59:56 -0700 Subject: [PATCH 7/9] Skip implicit self argument for closures --- src/librustc_mir/borrow_check/mod.rs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index 9da5d1dcdcd4f..a70c601bd1a06 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -265,6 +265,11 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>( if let ClearCrossCrate::Set(ref vsi) = mbcx.mir.visibility_scope_info { let local_decl = &mbcx.mir.local_decls[local]; + // Skip implicit `self` argument for closures + if local.index() == 1 && tcx.is_closure(mbcx.mir_def_id) { + continue; + } + // Skip over locals that begin with an underscore match local_decl.name { Some(name) if name.as_str().starts_with("_") => continue, @@ -1890,9 +1895,11 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { match tnm.mutbl { // `*const` raw pointers are not mutable hir::MutImmutable => return Err(place), - // `*mut` raw pointers are always mutable, regardless of context - // The users have to check by themselve. - hir::MutMutable => return Ok((place, is_local_mutation_allowed)), + // `*mut` raw pointers are always mutable, regardless of + // context. The users have to check by themselves. + hir::MutMutable => { + return Ok((place, is_local_mutation_allowed)); + } } } // `Box` owns its content, so mutable if its location is mutable From ded06976790299c87ca3501aca23d3c74b5b0039 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 6 Apr 2018 12:35:50 -0400 Subject: [PATCH 8/9] Return RootPlace in is_mutable --- src/librustc_mir/borrow_check/mod.rs | 42 +++++++++++++++++++++++----- 1 file changed, 35 insertions(+), 7 deletions(-) diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index a70c601bd1a06..f2b0ec58e7823 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -1150,13 +1150,14 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { // 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); + debug!("{:?} used_mut_upvars={:?}", def_id, used_mut_upvars); 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) { + Operand::Move(ref place @ Place::Projection(_)) => { + if let Some(field) = self.is_upvar_field_projection(place) { self.used_mut_upvars.push(field); } } @@ -1697,8 +1698,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { } } } - Ok((Place::Projection(ref proj), _mut_allowed)) => { - if let Some(field) = self.is_upvar_field_projection(&proj.base) { + Ok((Place::Projection(_), _mut_allowed)) => { + if let Some(field) = self.is_upvar_field_projection(&place) { self.used_mut_upvars.push(field); } } @@ -1734,8 +1735,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { } } } - Ok((Place::Projection(ref proj), _mut_allowed)) => { - if let Some(field) = self.is_upvar_field_projection(&proj.base) { + Ok((Place::Projection(_), _mut_allowed)) => { + if let Some(field) = self.is_upvar_field_projection(&place) { self.used_mut_upvars.push(field); } } @@ -1930,7 +1931,34 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { } (Mutability::Not, LocalMutationIsAllowed::Yes) | (Mutability::Mut, _) => { - self.is_mutable(&proj.base, is_local_mutation_allowed) + // Subtle: this is an upvar + // reference, so it looks like + // `self.foo` -- we want to double + // check that the context `*self` + // is mutable (i.e., this is not a + // `Fn` closure). But if that + // check succeeds, we want to + // *blame* the mutability on + // `place` (that is, + // `self.foo`). This is used to + // propagate the info about + // whether mutability declarations + // are used outwards, so that we register + // the outer variable as mutable. Otherwise a + // test like this fails to record the `mut` + // as needed: + // + // ``` + // fn foo(_f: F) { } + // fn main() { + // let var = Vec::new(); + // foo(move || { + // var.push(1); + // }); + // } + // ``` + let _ = self.is_mutable(&proj.base, is_local_mutation_allowed)?; + Ok((place, is_local_mutation_allowed)) } } } else { From 0a1cb9b91422e38baf352993ad78446a29160ed8 Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Sat, 21 Apr 2018 23:41:44 -0700 Subject: [PATCH 9/9] Add the actual used mutable var to the set --- src/librustc_mir/borrow_check/mod.rs | 167 ++++++++++++++++----------- 1 file changed, 102 insertions(+), 65 deletions(-) diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index f2b0ec58e7823..3145be7df851e 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -259,6 +259,31 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>( mbcx.analyze_results(&mut state); // entry point for DataflowResultsConsumer + // For each non-user used mutable variable, check if it's been assigned from + // a user-declared local. If so, then put that local into the used_mut set. + // Note that this set is expected to be small - only upvars from closures + // would have a chance of erroneously adding non-user-defined mutable vars + // to the set. + let temporary_used_locals: FxHashSet = + mbcx.used_mut.iter() + .filter(|&local| !mbcx.mir.local_decls[*local].is_user_variable) + .cloned() + .collect(); + + for local in temporary_used_locals { + for location in mbcx.mir.find_assignments(local) { + for moi in &mbcx.move_data.loc_map[location] { + let mpi = &mbcx.move_data.moves[*moi].path; + let path = &mbcx.move_data.move_paths[*mpi]; + debug!("assignment of {:?} to {:?}, adding {:?} to used mutable set", + path.place, local, path.place); + if let Place::Local(user_local) = path.place { + mbcx.used_mut.insert(user_local); + } + } + } + } + debug!("mbcx.used_mut: {:?}", mbcx.used_mut); for local in mbcx.mir.mut_vars_and_args_iter().filter(|local| !mbcx.used_mut.contains(local)) { @@ -731,6 +756,11 @@ enum InitializationRequiringAction { Assignment, } +struct RootPlace<'d, 'tcx: 'd> { + place: &'d Place<'tcx>, + is_local_mutation_allowed: LocalMutationIsAllowed, +} + impl InitializationRequiringAction { fn as_noun(self) -> &'static str { match self { @@ -1687,23 +1717,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { Reservation(WriteKind::MutableBorrow(BorrowKind::Mut { .. })) | Write(WriteKind::MutableBorrow(BorrowKind::Mut { .. })) => { match self.is_mutable(place, is_local_mutation_allowed) { - Ok((Place::Local(local), mut_allowed)) => { - if mut_allowed != LocalMutationIsAllowed::Yes { - // If the local may be initialized, and it is now currently being - // mutated, then it is justified to be annotated with the `mut` - // keyword, since the mutation may be a possible reassignment. - let mpi = self.move_data.rev_lookup.find_local(*local); - if flow_state.inits.contains(&mpi) { - self.used_mut.insert(*local); - } - } - } - Ok((Place::Projection(_), _mut_allowed)) => { - if let Some(field) = self.is_upvar_field_projection(&place) { - self.used_mut_upvars.push(field); - } - } - Ok((Place::Static(..), _mut_allowed)) => {} + Ok(root_place) => self.add_used_mut(root_place, flow_state), Err(place_err) => { error_reported = true; let item_msg = self.get_default_err_msg(place); @@ -1724,55 +1738,35 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { } Reservation(WriteKind::Mutate) | Write(WriteKind::Mutate) => { match self.is_mutable(place, is_local_mutation_allowed) { - Ok((Place::Local(local), mut_allowed)) => { - if mut_allowed != LocalMutationIsAllowed::Yes { - // If the local may be initialized, and it is now currently being - // mutated, then it is justified to be annotated with the `mut` - // keyword, since the mutation may be a possible reassignment. - let mpi = self.move_data.rev_lookup.find_local(*local); - if flow_state.inits.contains(&mpi) { - self.used_mut.insert(*local); - } - } - } - Ok((Place::Projection(_), _mut_allowed)) => { - if let Some(field) = self.is_upvar_field_projection(&place) { - self.used_mut_upvars.push(field); - } - } - Ok((Place::Static(..), _mut_allowed)) => {} + Ok(root_place) => self.add_used_mut(root_place, flow_state), Err(place_err) => { error_reported = true; let err_info = if let Place::Projection( box Projection { - ref base, + base: Place::Local(local), elem: ProjectionElem::Deref } ) = *place_err { - if let Place::Local(local) = *base { - let locations = self.mir.find_assignments(local); - if locations.len() > 0 { - let item_msg = if error_reported { - self.get_secondary_err_msg(base) - } else { - self.get_default_err_msg(place) - }; - let sp = self.mir.source_info(locations[0]).span; - let mut to_suggest_span = String::new(); - if let Ok(src) = - self.tcx.sess.codemap().span_to_snippet(sp) { - to_suggest_span = src[1..].to_string(); - }; - Some((sp, - "consider changing this to be a \ - mutable reference", - to_suggest_span, - item_msg, - self.get_primary_err_msg(base))) + let locations = self.mir.find_assignments(local); + if locations.len() > 0 { + let item_msg = if error_reported { + self.get_secondary_err_msg(&Place::Local(local)) } else { - None - } + self.get_default_err_msg(place) + }; + let sp = self.mir.source_info(locations[0]).span; + let mut to_suggest_span = String::new(); + if let Ok(src) = + self.tcx.sess.codemap().span_to_snippet(sp) { + to_suggest_span = src[1..].to_string(); + }; + Some((sp, + "consider changing this to be a \ + mutable reference", + to_suggest_span, + item_msg, + self.get_primary_err_msg(&Place::Local(local)))) } else { None } @@ -1834,33 +1828,76 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { error_reported } - /// Can this value be written or borrowed mutably + /// Adds the place into the used mutable variables set + fn add_used_mut<'d>( + &mut self, + root_place: RootPlace<'d, 'tcx>, + flow_state: &Flows<'cx, 'gcx, 'tcx> + ) { + match root_place { + RootPlace { + place: Place::Local(local), + is_local_mutation_allowed, + } => { + if is_local_mutation_allowed != LocalMutationIsAllowed::Yes { + // If the local may be initialized, and it is now currently being + // mutated, then it is justified to be annotated with the `mut` + // keyword, since the mutation may be a possible reassignment. + let mpi = self.move_data.rev_lookup.find_local(*local); + if flow_state.inits.contains(&mpi) { + self.used_mut.insert(*local); + } + } + } + RootPlace { + place: place @ Place::Projection(_), + is_local_mutation_allowed: _, + } => { + if let Some(field) = self.is_upvar_field_projection(&place) { + self.used_mut_upvars.push(field); + } + } + RootPlace { + place: Place::Static(..), + is_local_mutation_allowed: _, + } => {} + } + } + + /// Whether this value be written or borrowed mutably. + /// Returns the root place if the place passed in is a projection. fn is_mutable<'d>( &self, place: &'d Place<'tcx>, is_local_mutation_allowed: LocalMutationIsAllowed, - ) -> Result<(&'d Place<'tcx>, LocalMutationIsAllowed), &'d Place<'tcx>> { + ) -> Result, &'d Place<'tcx>> { match *place { Place::Local(local) => { let local = &self.mir.local_decls[local]; match local.mutability { Mutability::Not => match is_local_mutation_allowed { LocalMutationIsAllowed::Yes => { - Ok((place, LocalMutationIsAllowed::Yes)) + Ok(RootPlace { + place, + is_local_mutation_allowed: LocalMutationIsAllowed::Yes + }) } LocalMutationIsAllowed::ExceptUpvars => { - Ok((place, LocalMutationIsAllowed::ExceptUpvars)) + Ok(RootPlace { + place, + is_local_mutation_allowed: LocalMutationIsAllowed::ExceptUpvars + }) } LocalMutationIsAllowed::No => Err(place), }, - Mutability::Mut => Ok((place, is_local_mutation_allowed)), + Mutability::Mut => Ok(RootPlace { place, is_local_mutation_allowed }), } } Place::Static(ref static_) => if self.tcx.is_static(static_.def_id) != Some(hir::Mutability::MutMutable) { Err(place) } else { - Ok((place, is_local_mutation_allowed)) + Ok(RootPlace { place, is_local_mutation_allowed }) }, Place::Projection(ref proj) => { match proj.elem { @@ -1899,7 +1936,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { // `*mut` raw pointers are always mutable, regardless of // context. The users have to check by themselves. hir::MutMutable => { - return Ok((place, is_local_mutation_allowed)); + return Ok(RootPlace { place, is_local_mutation_allowed }); } } } @@ -1958,7 +1995,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { // } // ``` let _ = self.is_mutable(&proj.base, is_local_mutation_allowed)?; - Ok((place, is_local_mutation_allowed)) + Ok(RootPlace { place, is_local_mutation_allowed }) } } } else {