From 6c649fbed4d4d86aed16dff8c0245b4871353cd1 Mon Sep 17 00:00:00 2001 From: gaurikholkar Date: Wed, 14 Mar 2018 23:51:54 +0530 Subject: [PATCH] address code review comments --- src/librustc_mir/borrow_check/mod.rs | 52 +++++++++++++------------ src/librustc_mir/util/collect_writes.rs | 28 ++++++------- 2 files changed, 43 insertions(+), 37 deletions(-) diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index c774a8b9d928..dec32f5b2e53 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -1422,13 +1422,6 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { } } - fn get_main_error_message(&self, place:&Place<'tcx>) -> String{ - match self.describe_place(place) { - Some(name) => format!("immutable item `{}`", name), - None => "immutable item".to_owned(), - } - } - /// Currently MoveData does not store entries for all places in /// the input MIR. For example it will currently filter out /// places that are Copy; thus we do not track places of shared @@ -1533,6 +1526,21 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { } } + fn specialized_description(&self, place:&Place<'tcx>) -> Option{ + if let Some(name) = self.describe_place(place) { + Some(format!("`&`-reference `{}`", name)) + } else { + None + } + } + + fn get_main_error_message(&self, place:&Place<'tcx>) -> String{ + match self.describe_place(place) { + Some(name) => format!("immutable item `{}`", name), + None => "immutable item".to_owned(), + } + } + /// Check the permissions for the given place and read or write kind /// /// Returns true if an error is reported, false otherwise. @@ -1566,7 +1574,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { if place != place_err { if let Some(name) = self.describe_place(place_err) { - err.note(&format!("value not mutable causing this error: `{}`", name)); + err.note(&format!("the value which is causing this path not to be mutable is...: `{}`", name)); } } @@ -1576,7 +1584,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { if let Err(place_err) = self.is_mutable(place, is_local_mutation_allowed) { error_reported = true; - let err_info = match *place_err { + let mut err_info = None; + match *place_err { Place::Projection(ref proj) => { match proj.elem { ProjectionElem::Deref => { @@ -1585,32 +1594,27 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { let locations = self.mir.find_assignments(local); if locations.len() > 0 { let item_msg = if error_reported { - if let Some(name) = - self.describe_place(place_err) { - let var = str::replace(&name, "*", ""); - format!("`&`-reference `{}`", var) - } else { - self.get_main_error_message(place) + match self.specialized_description(&proj.base){ + Some(msg) => msg, + None => self.get_main_error_message(place) } } else { self.get_main_error_message(place) }; - Some((self.mir.source_info(locations[0]).span, + err_info = Some((self.mir.source_info(locations[0]).span, "consider changing this to be a \ mutable reference: `&mut`", item_msg, - "cannot assign through `&`-reference")) - } else { - None + "cannot assign through `&`-reference")); } } - _ => None, + _ => {}, } } - _ => None, + _ => {} } } - _ => None, - }; + _ => {} + } if let Some((err_help_span, err_help_stmt, item_msg, sec_span)) = err_info { let mut err = self.tcx.cannot_assign(span, &item_msg, Origin::Mir, true); @@ -1625,7 +1629,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { err.span_label(span, "cannot mutate"); if place != place_err { if let Some(name) = self.describe_place(place_err) { - err.note(&format!("value not mutable causing this error: `{}`", + err.note(&format!("the value which is causing this path not to be mutable is...: `{}`", name)); } } diff --git a/src/librustc_mir/util/collect_writes.rs b/src/librustc_mir/util/collect_writes.rs index cd52f39c0808..f04f9233447c 100644 --- a/src/librustc_mir/util/collect_writes.rs +++ b/src/librustc_mir/util/collect_writes.rs @@ -13,9 +13,23 @@ use rustc::mir::Mir; use rustc::mir::visit::PlaceContext; use rustc::mir::visit::Visitor; +crate trait FindAssignments { + // Finds all statements that assign directly to local (i.e., X = ...) + // and returns their locations. + fn find_assignments(&self, local: Local) -> Vec; +} + +impl<'tcx> FindAssignments for Mir<'tcx>{ + fn find_assignments(&self, local: Local) -> Vec{ + let mut visitor = FindLocalAssignmentVisitor{ needle: local, locations: vec![]}; + visitor.visit_mir(self); + visitor.locations + } +} + // The Visitor walks the MIR to return the assignment statements corresponding // to a Local. -pub struct FindLocalAssignmentVisitor { +struct FindLocalAssignmentVisitor { needle: Local, locations: Vec, } @@ -51,15 +65,3 @@ impl<'tcx> Visitor<'tcx> for FindLocalAssignmentVisitor { // TO-DO // fn super_local() } - -crate trait FindAssignments { - fn find_assignments(&self, local: Local) -> Vec; - } - -impl<'tcx> FindAssignments for Mir<'tcx>{ - fn find_assignments(&self, local: Local) -> Vec{ - let mut visitor = FindLocalAssignmentVisitor{ needle: local, locations: vec![]}; - visitor.visit_mir(self); - visitor.locations - } -}