Auto merge of #48914 - gaurikholkar:e0389, r=nikomatsakis

Modify compile-fail/E0389 error message WIP

This fixes #47388

cc @nikomatsakis @estebank

r? @nikomatsakis

Certain ui tests were failing locally. I'll check if the same happens here too.

8 files changed, 182 insertions, 24 deletions

diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs
index fe7aedb4127..62acdf76546 100644
--- a/src/librustc_mir/borrow_check/mod.rs
+++ b/src/librustc_mir/borrow_check/mod.rs
@@ -42,6 +42,7 @@ use dataflow::indexes::BorrowIndex;
 use dataflow::move_paths::{IllegalMoveOriginKind, MoveError};
 use dataflow::move_paths::{HasMoveData, LookupResult, MoveData, MovePathIndex};
 use util::borrowck_errors::{BorrowckErrors, Origin};
+use util::collect_writes::FindAssignments;
 
 use std::iter;
 
@@ -1550,6 +1551,36 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
         }
     }
 
+    fn specialized_description(&self, place:&Place<'tcx>) -> Option<String>{
+        if let Some(_name) = self.describe_place(place) {
+            Some(format!("data in a `&` reference"))
+        } else {
+            None
+        }
+    }
+
+    fn get_default_err_msg(&self, place:&Place<'tcx>) -> String{
+        match self.describe_place(place) {
+            Some(name) => format!("immutable item `{}`", name),
+            None => "immutable item".to_owned(),
+        }
+    }
+
+    fn get_secondary_err_msg(&self, place:&Place<'tcx>) -> String{
+        match self.specialized_description(place) {
+            Some(_) => format!("data in a `&` reference"),
+            None => self.get_default_err_msg(place)
+        }
+    }
+
+    fn get_primary_err_msg(&self, place:&Place<'tcx>) -> String{
+        if let Some(name) = self.describe_place(place) {
+            format!("`{}` is a `&` reference, so the data it refers to cannot be written", name)
+        } else {
+            format!("cannot assign through `&`-reference")
+        }
+    }
+
     /// Check the permissions for the given place and read or write kind
     ///
     /// Returns true if an error is reported, false otherwise.
@@ -1576,43 +1607,70 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                 self.is_mutable(place, is_local_mutation_allowed)
             {
                 error_reported = true;
-
-                let item_msg = match self.describe_place(place) {
-                    Some(name) => format!("immutable item `{}`", name),
-                    None => "immutable item".to_owned(),
-                };
-
+                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!("Value not mutable causing this error: `{}`", name));
+                        err.note(&format!("the value which is causing this path not to be mutable \
+                                           is...: `{}`", name));
                     }
                 }
 
                 err.emit();
             },
             Reservation(WriteKind::Mutate) | Write(WriteKind::Mutate) => {
+
                 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)
+                                            };
+                                            err_info = Some((
+                                                self.mir.source_info(locations[0]).span,
+                                                    "consider changing this to be a \
+                                                    mutable reference: `&mut`", item_msg,
+                                                    self.get_primary_err_msg(base)));
+                                        }
+                                },
+                            _ => {},
+                            }
+                        },
+                        _ => {},
+                    }
 
-                    let item_msg = match self.describe_place(place) {
-                        Some(name) => format!("immutable item `{}`", name),
-                        None => "immutable item".to_owned(),
-                    };
-
-                    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!("Value not mutable causing this error: `{}`", name));
+                    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);
+                        err.span_suggestion(err_help_span, err_help_stmt, format!(""));
+                        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();
                     }
-
-                    err.emit();
                 }
             }
             Reservation(WriteKind::Move)
@@ -1631,9 +1689,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                     );
                 }
             }
-
             Activation(..) => {} // permission checks are done at Reservation point.
-
             Read(ReadKind::Borrow(BorrowKind::Unique))
             | Read(ReadKind::Borrow(BorrowKind::Mut { .. }))
             | Read(ReadKind::Borrow(BorrowKind::Shared))
@@ -2255,3 +2311,4 @@ impl ContextKind {
         }
     }
 }
+
diff --git a/src/librustc_mir/lib.rs b/src/librustc_mir/lib.rs
index 8762e7550cd..0fc90544300 100644
--- a/src/librustc_mir/lib.rs
+++ b/src/librustc_mir/lib.rs
@@ -32,6 +32,7 @@ Rust MIR: a lowered representation of Rust. Also: an experiment!
 #![feature(rustc_diagnostic_macros)]
 #![feature(nonzero)]
 #![feature(inclusive_range_fields)]
+#![feature(crate_visibility_modifier)]
 
 extern crate arena;
 #[macro_use]
diff --git a/src/librustc_mir/util/borrowck_errors.rs b/src/librustc_mir/util/borrowck_errors.rs
index 5e15348de5e..d6b3e674f8f 100644
--- a/src/librustc_mir/util/borrowck_errors.rs
+++ b/src/librustc_mir/util/borrowck_errors.rs
@@ -284,7 +284,8 @@ pub trait BorrowckErrors<'cx>: Sized + Copy {
         self.cancel_if_wrong_origin(err, o)
     }
 
-    fn cannot_assign(self, span: Span, desc: &str, o: Origin) -> DiagnosticBuilder<'cx>
+    fn cannot_assign(self, span: Span, desc: &str, o: Origin)
+                     -> DiagnosticBuilder<'cx>
     {
         let err = struct_span_err!(self, span, E0594,
                                   "cannot assign to {}{OGN}",
diff --git a/src/librustc_mir/util/collect_writes.rs b/src/librustc_mir/util/collect_writes.rs
new file mode 100644
index 00000000000..f04f9233447
--- /dev/null
+++ b/src/librustc_mir/util/collect_writes.rs
@@ -0,0 +1,67 @@
+// Copyright 2018 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 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+use rustc::mir::{Local, Location};
+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<Location>;
+}
+
+impl<'tcx> FindAssignments for Mir<'tcx>{
+    fn find_assignments(&self, local: Local) -> Vec<Location>{
+            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.
+struct FindLocalAssignmentVisitor {
+    needle: Local,
+    locations: Vec<Location>,
+}
+
+impl<'tcx> Visitor<'tcx> for FindLocalAssignmentVisitor {
+    fn visit_local(&mut self,
+                   local: &Local,
+                   place_context: PlaceContext<'tcx>,
+                   location: Location) {
+        if self.needle != *local {
+            return;
+        }
+
+        match place_context {
+            PlaceContext::Store | PlaceContext::Call => {
+                self.locations.push(location);
+            }
+            PlaceContext::AsmOutput |
+            PlaceContext::Drop |
+            PlaceContext::Inspect |
+            PlaceContext::Borrow { .. } |
+            PlaceContext::Projection(..) |
+            PlaceContext::Copy |
+            PlaceContext::Move |
+            PlaceContext::StorageLive |
+            PlaceContext::StorageDead |
+            PlaceContext::Validate => {
+                // TO-DO
+                // self.super_local(local)
+            }
+        }
+    }
+    // TO-DO
+    // fn super_local()
+}
diff --git a/src/librustc_mir/util/mod.rs b/src/librustc_mir/util/mod.rs
index eebe5a86018..19cd3766886 100644
--- a/src/librustc_mir/util/mod.rs
+++ b/src/librustc_mir/util/mod.rs
@@ -17,6 +17,7 @@ mod alignment;
 mod graphviz;
 pub(crate) mod pretty;
 pub mod liveness;
+pub mod collect_writes;
 
 pub use self::alignment::is_disaligned;
 pub use self::pretty::{dump_enabled, dump_mir, write_mir_pretty, PassWhere};
diff --git a/src/test/compile-fail/borrowck/borrowck-issue-14498.rs b/src/test/compile-fail/borrowck/borrowck-issue-14498.rs
index 8a09ab3fd06..fbdd013024d 100644
--- a/src/test/compile-fail/borrowck/borrowck-issue-14498.rs
+++ b/src/test/compile-fail/borrowck/borrowck-issue-14498.rs
@@ -27,7 +27,7 @@ fn indirect_write_to_imm_box() {
     let y: Box<_> = box &mut x;
     let p = &y;
     ***p = 2; //[ast]~ ERROR cannot assign to data in a `&` reference
-              //[mir]~^ ERROR cannot assign to immutable item `***p`
+              //[mir]~^ ERROR cannot assign to data in a `&` reference
     drop(p);
 }
 
diff --git a/src/test/ui/nll/issue-47388.rs b/src/test/ui/nll/issue-47388.rs
new file mode 100644
index 00000000000..39feea08aa4
--- /dev/null
+++ b/src/test/ui/nll/issue-47388.rs
@@ -0,0 +1,20 @@
+// Copyright 2018 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 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+#![feature(nll)]
+struct FancyNum {
+    num: u8,
+}
+
+fn main() {
+    let mut fancy = FancyNum{ num: 5 };
+    let fancy_ref = &(&mut fancy);
+    fancy_ref.num = 6; //~ ERROR E0594
+    println!("{}", fancy_ref.num);
+}
diff --git a/src/test/ui/nll/issue-47388.stderr b/src/test/ui/nll/issue-47388.stderr
new file mode 100644
index 00000000000..272cb6510aa
--- /dev/null
+++ b/src/test/ui/nll/issue-47388.stderr
@@ -0,0 +1,11 @@
+error[E0594]: cannot assign to data in a `&` reference
+  --> $DIR/issue-47388.rs:18:5
+   |
+LL |     let fancy_ref = &(&mut fancy);
+   |                     ------------- help: consider changing this to be a mutable reference: `&mut`
+LL |     fancy_ref.num = 6; //~ ERROR E0594
+   |     ^^^^^^^^^^^^^^^^^ `fancy_ref` is a `&` reference, so the data it refers to cannot be written
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0594`.