Improve cannot move errors

* Show the place and type being moved
* Give a special error for variables in match guard
* Simplify search for overloaded deref
* Search for overloaded index

2 files changed, 320 insertions, 209 deletions

diff --git a/src/librustc_mir/borrow_check/move_errors.rs b/src/librustc_mir/borrow_check/move_errors.rs
index 2e3c92acb2c..27486df1967 100644
--- a/src/librustc_mir/borrow_check/move_errors.rs
+++ b/src/librustc_mir/borrow_check/move_errors.rs
@@ -1,8 +1,7 @@
 use core::unicode::property::Pattern_White_Space;
-use std::fmt::{self, Display};
 
 use rustc::mir::*;
-use rustc::ty;
+use rustc::ty::{self, Ty, TyCtxt};
 use rustc_errors::{DiagnosticBuilder,Applicability};
 use syntax_pos::Span;
 
@@ -55,20 +54,66 @@ enum GroupedMoveError<'tcx> {
     },
 }
 
-enum BorrowedContentSource {
-    Arc,
-    Rc,
+enum BorrowedContentSource<'tcx> {
     DerefRawPointer,
-    Other,
+    DerefMutableRef,
+    DerefSharedRef,
+    OverloadedDeref(Ty<'tcx>),
+    OverloadedIndex(Ty<'tcx>),
 }
 
-impl Display for BorrowedContentSource {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+impl BorrowedContentSource<'tcx> {
+    fn describe_for_unnamed_place(&self) -> String {
         match *self {
-            BorrowedContentSource::Arc => write!(f, "an `Arc`"),
-            BorrowedContentSource::Rc => write!(f, "an `Rc`"),
-            BorrowedContentSource::DerefRawPointer => write!(f, "dereference of raw pointer"),
-            BorrowedContentSource::Other => write!(f, "borrowed content"),
+            BorrowedContentSource::DerefRawPointer => format!("a raw pointer"),
+            BorrowedContentSource::DerefSharedRef => format!("a shared reference"),
+            BorrowedContentSource::DerefMutableRef => {
+                format!("a mutable reference")
+            }
+            BorrowedContentSource::OverloadedDeref(ty) => {
+                if ty.is_rc() {
+                   format!("an `Rc`")
+                } else if ty.is_arc() {
+                    format!("an `Arc`")
+                } else {
+                    format!("dereference of `{}`", ty)
+                }
+            }
+            BorrowedContentSource::OverloadedIndex(ty) => format!("index of `{}`", ty),
+        }
+    }
+
+    fn describe_for_named_place(&self) -> Option<&'static str> {
+        match *self {
+            BorrowedContentSource::DerefRawPointer => Some("raw pointer"),
+            BorrowedContentSource::DerefSharedRef => Some("shared reference"),
+            BorrowedContentSource::DerefMutableRef => Some("mutable reference"),
+            // Overloaded deref and index operators should be evaluated into a
+            // temporary. So we don't need a description here.
+            BorrowedContentSource::OverloadedDeref(_)
+            | BorrowedContentSource::OverloadedIndex(_) => None
+        }
+    }
+
+    fn from_call(func: Ty<'tcx>, tcx: TyCtxt<'_, '_, 'tcx>) -> Option<Self> {
+        match func.sty {
+            ty::FnDef(def_id, substs) => {
+                let trait_id = tcx.trait_of_item(def_id)?;
+
+                let lang_items = tcx.lang_items();
+                if Some(trait_id) == lang_items.deref_trait()
+                    || Some(trait_id) == lang_items.deref_mut_trait()
+                {
+                    Some(BorrowedContentSource::OverloadedDeref(substs.type_at(0)))
+                } else if Some(trait_id) == lang_items.index_trait()
+                    || Some(trait_id) == lang_items.index_mut_trait()
+                {
+                    Some(BorrowedContentSource::OverloadedIndex(substs.type_at(0)))
+                } else {
+                    None
+                }
+            }
+            _ => None,
         }
     }
 }
@@ -248,109 +293,194 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
                         (span, original_path, kind)
                     },
                 };
-            let origin = Origin::Mir;
             debug!("report: original_path={:?} span={:?}, kind={:?} \
                    original_path.is_upvar_field_projection={:?}", original_path, span, kind,
                    self.is_upvar_field_projection(original_path));
-            let err = match kind {
-                IllegalMoveOriginKind::Static => {
-                    self.infcx.tcx.cannot_move_out_of(span, "static item", origin)
-                }
-                IllegalMoveOriginKind::BorrowedContent { target_place: place } => {
-                    // Inspect the type of the content behind the
-                    // borrow to provide feedback about why this
-                    // was a move rather than a copy.
-                    let ty = place.ty(self.mir, self.infcx.tcx).ty;
-                    let is_upvar_field_projection =
-                        self.prefixes(&original_path, PrefixSet::All)
-                        .any(|p| self.is_upvar_field_projection(p).is_some());
-                    debug!("report: ty={:?}", ty);
-                    let mut err = match ty.sty {
-                        ty::Array(..) | ty::Slice(..) =>
-                            self.infcx.tcx.cannot_move_out_of_interior_noncopy(
-                                span, ty, None, origin
-                            ),
-                        ty::Closure(def_id, closure_substs)
-                            if def_id == self.mir_def_id && is_upvar_field_projection
-                        => {
-                            let closure_kind_ty =
-                                closure_substs.closure_kind_ty(def_id, self.infcx.tcx);
-                            let closure_kind = closure_kind_ty.to_opt_closure_kind();
-                            let place_description = match closure_kind {
-                                Some(ty::ClosureKind::Fn) => {
-                                    "captured variable in an `Fn` closure"
-                                }
-                                Some(ty::ClosureKind::FnMut) => {
-                                    "captured variable in an `FnMut` closure"
-                                }
-                                Some(ty::ClosureKind::FnOnce) => {
-                                    bug!("closure kind does not match first argument type")
-                                }
-                                None => bug!("closure kind not inferred by borrowck"),
-                            };
-                            debug!("report: closure_kind_ty={:?} closure_kind={:?} \
-                                    place_description={:?}", closure_kind_ty, closure_kind,
-                                    place_description);
-
-                            let mut diag = self.infcx.tcx.cannot_move_out_of(
-                                span, place_description, origin);
-
-                            for prefix in self.prefixes(&original_path, PrefixSet::All) {
-                                if let Some(field) = self.is_upvar_field_projection(prefix) {
-                                    let upvar_hir_id = self.upvars[field.index()].var_hir_id;
-                                    let upvar_span = self.infcx.tcx.hir().span_by_hir_id(
-                                        upvar_hir_id);
-                                    diag.span_label(upvar_span, "captured outer variable");
-                                    break;
-                                }
-                            }
-
-                            diag
-                        }
-                        _ => {
-                            let source = self.borrowed_content_source(place);
-                            self.infcx.tcx.cannot_move_out_of(
-                                span, &source.to_string(), origin
-                            )
-                        },
-                    };
-                    let orig_path_ty = format!(
-                        "{:?}",
-                        original_path.ty(self.mir, self.infcx.tcx).ty,
-                    );
-                    let snippet = self.infcx.tcx.sess.source_map().span_to_snippet(span).unwrap();
-                    let is_option = orig_path_ty.starts_with("std::option::Option");
-                    let is_result = orig_path_ty.starts_with("std::result::Result");
-                    if  is_option || is_result {
-                        err.span_suggestion(
+            (
+                match kind {
+                    IllegalMoveOriginKind::Static => {
+                        self.report_cannot_move_from_static(original_path, span)
+                    }
+                    IllegalMoveOriginKind::BorrowedContent { target_place } => {
+                        self.report_cannot_move_from_borrowed_content(
+                            original_path,
+                            target_place,
                             span,
-                            &format!("consider borrowing the `{}`'s content", if is_option {
-                                "Option"
-                            } else {
-                                "Result"
-                            }),
-                            format!("{}.as_ref()", snippet),
-                            Applicability::MaybeIncorrect,
-                        );
+                        )
                     }
-                    err
-                }
-                IllegalMoveOriginKind::InteriorOfTypeWithDestructor { container_ty: ty } => {
-                    self.infcx.tcx
-                        .cannot_move_out_of_interior_of_drop(span, ty, origin)
-                }
-                IllegalMoveOriginKind::InteriorOfSliceOrArray { ty, is_index } =>
-                    self.infcx.tcx.cannot_move_out_of_interior_noncopy(
-                        span, ty, Some(*is_index), origin
-                    ),
-            };
-            (err, span)
+                    IllegalMoveOriginKind::InteriorOfTypeWithDestructor { container_ty: ty } => {
+                        self.infcx.tcx
+                            .cannot_move_out_of_interior_of_drop(span, ty, Origin::Mir)
+                    }
+                    IllegalMoveOriginKind::InteriorOfSliceOrArray { ty, is_index } =>
+                        self.infcx.tcx.cannot_move_out_of_interior_noncopy(
+                            span, ty, Some(*is_index), Origin::Mir
+                        ),
+                },
+                span,
+            )
         };
 
         self.add_move_hints(error, &mut err, err_span);
         err.buffer(&mut self.errors_buffer);
     }
 
+    fn report_cannot_move_from_static(
+        &mut self,
+        place: &Place<'tcx>,
+        span: Span
+    ) -> DiagnosticBuilder<'a> {
+        let mut base_static = place;
+        loop {
+            match base_static {
+                Place::Base(_) => break,
+                Place::Projection(box Projection { base, .. }) => base_static = base,
+            }
+        }
+
+        let description = if let Place::Base(_) = place {
+            format!("static item `{}`", self.describe_place(place).unwrap())
+        } else {
+            format!(
+                "`{:?}` as `{:?}` is a static item",
+                self.describe_place(place).unwrap(),
+                self.describe_place(base_static).unwrap(),
+            )
+        };
+
+        self.infcx.tcx.cannot_move_out_of(span, &description, Origin::Mir)
+    }
+
+    fn report_cannot_move_from_borrowed_content(
+        &mut self,
+        move_place: &Place<'tcx>,
+        deref_target_place: &Place<'tcx>,
+        span: Span,
+    ) -> DiagnosticBuilder<'a> {
+        let origin = Origin::Mir;
+
+        // Inspect the type of the content behind the
+        // borrow to provide feedback about why this
+        // was a move rather than a copy.
+        let ty = deref_target_place.ty(self.mir, self.infcx.tcx).ty;
+        let upvar_field = self.prefixes(&move_place, PrefixSet::All)
+            .find_map(|p| self.is_upvar_field_projection(p));
+
+        let deref_base = match deref_target_place {
+            Place::Projection(box Projection { base, elem: ProjectionElem::Deref }) => base,
+            _ => bug!("deref_target_place is not a deref projection"),
+        };
+
+        if let Place::Base(PlaceBase::Local(local)) = *deref_base {
+            let decl = &self.mir.local_decls[local];
+            if decl.is_ref_for_guard() {
+                let mut err = self.infcx.tcx.cannot_move_out_of(
+                    span,
+                    &format!("`{}` in pattern guard", decl.name.unwrap()),
+                    origin,
+                );
+                err.note(
+                    "variables bound in patterns cannot be moved from \
+                     until after the end of the pattern guard");
+                return err;
+            }
+        }
+
+        debug!("report: ty={:?}", ty);
+        let mut err = match ty.sty {
+            ty::Array(..) | ty::Slice(..) =>
+                self.infcx.tcx.cannot_move_out_of_interior_noncopy(
+                    span, ty, None, origin
+                ),
+            ty::Closure(def_id, closure_substs)
+                if def_id == self.mir_def_id && upvar_field.is_some()
+            => {
+                let closure_kind_ty = closure_substs.closure_kind_ty(def_id, self.infcx.tcx);
+                let closure_kind = closure_kind_ty.to_opt_closure_kind();
+                let capture_description = match closure_kind {
+                    Some(ty::ClosureKind::Fn) => {
+                        "captured variable in an `Fn` closure"
+                    }
+                    Some(ty::ClosureKind::FnMut) => {
+                        "captured variable in an `FnMut` closure"
+                    }
+                    Some(ty::ClosureKind::FnOnce) => {
+                        bug!("closure kind does not match first argument type")
+                    }
+                    None => bug!("closure kind not inferred by borrowck"),
+                };
+
+                let upvar = &self.upvars[upvar_field.unwrap().index()];
+                let upvar_hir_id = upvar.var_hir_id;
+                let upvar_name = upvar.name;
+                let upvar_span = self.infcx.tcx.hir().span_by_hir_id(upvar_hir_id);
+
+                let place_name = self.describe_place(move_place).unwrap();
+
+                let place_description = if self.is_upvar_field_projection(move_place).is_some() {
+                    format!("`{}`, a {}", place_name, capture_description)
+                } else {
+                    format!(
+                        "`{}`, as `{}` is a {}",
+                        place_name,
+                        upvar_name,
+                        capture_description,
+                    )
+                };
+
+                debug!(
+                    "report: closure_kind_ty={:?} closure_kind={:?} place_description={:?}",
+                    closure_kind_ty, closure_kind, place_description,
+                );
+
+                let mut diag = self.infcx.tcx.cannot_move_out_of(span, &place_description, origin);
+
+                diag.span_label(upvar_span, "captured outer variable");
+
+                diag
+            }
+            _ => {
+                let source = self.borrowed_content_source(deref_base);
+                match (self.describe_place(move_place), source.describe_for_named_place()) {
+                    (Some(place_desc), Some(source_desc)) => {
+                        self.infcx.tcx.cannot_move_out_of(
+                            span,
+                            &format!("`{}` which is behind a {}", place_desc, source_desc),
+                            origin,
+                        )
+                    }
+                    (_, _) => {
+                        self.infcx.tcx.cannot_move_out_of(
+                            span,
+                            &source.describe_for_unnamed_place(),
+                            origin,
+                        )
+                    }
+                }
+            },
+        };
+        let move_ty = format!(
+            "{:?}",
+            move_place.ty(self.mir, self.infcx.tcx).ty,
+        );
+        let snippet = self.infcx.tcx.sess.source_map().span_to_snippet(span).unwrap();
+        let is_option = move_ty.starts_with("std::option::Option");
+        let is_result = move_ty.starts_with("std::result::Result");
+        if  is_option || is_result {
+            err.span_suggestion(
+                span,
+                &format!("consider borrowing the `{}`'s content", if is_option {
+                    "Option"
+                } else {
+                    "Result"
+                }),
+                format!("{}.as_ref()", snippet),
+                Applicability::MaybeIncorrect,
+            );
+        }
+        err
+    }
+
     fn add_move_hints(
         &self,
         error: GroupedMoveError<'tcx>,
@@ -391,9 +521,25 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
                     );
                 }
 
-                binds_to.sort();
-                binds_to.dedup();
-                self.add_move_error_details(err, &binds_to);
+                if binds_to.is_empty() {
+                    let place_ty = move_from.ty(self.mir, self.infcx.tcx).ty;
+                    let place_desc = match self.describe_place(&move_from) {
+                        Some(desc) => format!("`{}`", desc),
+                        None => format!("value"),
+                    };
+
+                    self.note_type_does_not_implement_copy(
+                        err,
+                        &place_desc,
+                        place_ty,
+                        Some(span)
+                    );
+                } else {
+                    binds_to.sort();
+                    binds_to.dedup();
+
+                    self.add_move_error_details(err, &binds_to);
+                }
             }
             GroupedMoveError::MovesFromValue { mut binds_to, .. } => {
                 binds_to.sort();
@@ -402,7 +548,19 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
                 self.add_move_error_details(err, &binds_to);
             }
             // No binding. Nothing to suggest.
-            GroupedMoveError::OtherIllegalMove { .. } => (),
+            GroupedMoveError::OtherIllegalMove { ref original_path, span, .. } => {
+                let place_ty = original_path.ty(self.mir, self.infcx.tcx).ty;
+                let place_desc = match self.describe_place(original_path) {
+                    Some(desc) => format!("`{}`", desc),
+                    None => format!("value"),
+                };
+                self.note_type_does_not_implement_copy(
+                    err,
+                    &place_desc,
+                    place_ty,
+                    Some(span),
+                );
+            },
         }
     }
 
@@ -493,105 +651,64 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
         }
     }
 
-    fn borrowed_content_source(&self, place: &Place<'tcx>) -> BorrowedContentSource {
-        // Look up the provided place and work out the move path index for it,
-        // we'll use this to work back through where this value came from and check whether it
-        // was originally part of an `Rc` or `Arc`.
-        let initial_mpi = match self.move_data.rev_lookup.find(place) {
-            LookupResult::Exact(mpi) | LookupResult::Parent(Some(mpi)) => mpi,
-            _ => return BorrowedContentSource::Other,
-        };
-
-        let mut queue = vec![initial_mpi];
-        let mut visited = Vec::new();
-        debug!("borrowed_content_source: queue={:?}", queue);
-        while let Some(mpi) = queue.pop() {
-            debug!(
-                "borrowed_content_source: mpi={:?} queue={:?} visited={:?}",
-                mpi, queue, visited
-            );
+    fn borrowed_content_source(&self, deref_base: &Place<'tcx>) -> BorrowedContentSource<'tcx> {
+        let tcx = self.infcx.tcx;
 
-            // Don't visit the same path twice.
-            if visited.contains(&mpi) {
-                continue;
-            }
-            visited.push(mpi);
-
-            for i in &self.move_data.init_path_map[mpi] {
-                let init = &self.move_data.inits[*i];
-                debug!("borrowed_content_source: init={:?}", init);
-                // We're only interested in statements that initialized a value, not the
-                // initializations from arguments.
-                let loc = match init.location {
-                    InitLocation::Statement(stmt) => stmt,
-                    _ => continue,
-                };
+        // Look up the provided place and work out the move path index for it,
+        // we'll use this to check whether it was originally from an overloaded
+        // operator.
+        match self.move_data.rev_lookup.find(deref_base) {
+            LookupResult::Exact(mpi) | LookupResult::Parent(Some(mpi)) => {
+                debug!("borrowed_content_source: mpi={:?}", mpi);
+
+                for i in &self.move_data.init_path_map[mpi] {
+                    let init = &self.move_data.inits[*i];
+                    debug!("borrowed_content_source: init={:?}", init);
+                    // We're only interested in statements that initialized a value, not the
+                    // initializations from arguments.
+                    let loc = match init.location {
+                        InitLocation::Statement(stmt) => stmt,
+                        _ => continue,
+                    };
 
-                let bbd = &self.mir[loc.block];
-                let is_terminator = bbd.statements.len() == loc.statement_index;
-                debug!("borrowed_content_source: loc={:?} is_terminator={:?}", loc, is_terminator);
-                if !is_terminator {
-                    let stmt = &bbd.statements[loc.statement_index];
-                    debug!("borrowed_content_source: stmt={:?}", stmt);
-                    // We're only interested in assignments (in particular, where the
-                    // assignment came from - was it an `Rc` or `Arc`?).
-                    if let StatementKind::Assign(_, box Rvalue::Ref(_, _, source)) = &stmt.kind {
-                        let ty = source.ty(self.mir, self.infcx.tcx).ty;
-                        let ty = match ty.sty {
-                            ty::Ref(_, ty, _) => ty,
-                            _ => ty,
-                        };
-                        debug!("borrowed_content_source: ty={:?}", ty);
-
-                        if ty.is_arc() {
-                            return BorrowedContentSource::Arc;
-                        } else if ty.is_rc() {
-                            return BorrowedContentSource::Rc;
-                        } else {
-                            queue.push(init.path);
-                        }
-                    }
-                } else if let Some(Terminator {
-                    kind: TerminatorKind::Call { args, .. },
-                    ..
-                }) = &bbd.terminator {
-                    for arg in args {
-                        let source = match arg {
-                            Operand::Copy(place) | Operand::Move(place) => place,
-                            _ => continue,
-                        };
-
-                        let ty = source.ty(self.mir, self.infcx.tcx).ty;
-                        let ty = match ty.sty {
-                            ty::Ref(_, ty, _) => ty,
-                            _ => ty,
-                        };
-                        debug!("borrowed_content_source: ty={:?}", ty);
-
-                        if ty.is_arc() {
-                            return BorrowedContentSource::Arc;
-                        } else if ty.is_rc() {
-                            return BorrowedContentSource::Rc;
-                        } else {
-                            queue.push(init.path);
+                    let bbd = &self.mir[loc.block];
+                    let is_terminator = bbd.statements.len() == loc.statement_index;
+                    debug!(
+                        "borrowed_content_source: loc={:?} is_terminator={:?}",
+                        loc,
+                        is_terminator,
+                    );
+                    if !is_terminator {
+                        continue;
+                    } else if let Some(Terminator {
+                        kind: TerminatorKind::Call {
+                            ref func,
+                            from_hir_call: false,
+                            ..
+                        },
+                        ..
+                    }) = bbd.terminator {
+                        if let Some(source)
+                            = BorrowedContentSource::from_call(func.ty(self.mir, tcx), tcx)
+                        {
+                            return source;
                         }
                     }
                 }
             }
-        }
+            // Base is a `static` so won't be from an overloaded operator
+            _ => (),
+        };
 
-        // If we didn't find an `Arc` or an `Rc`, then check specifically for
-        // a dereference of a place that has the type of a raw pointer.
-        // We can't use `place.ty(..).to_ty(..)` here as that strips away the raw pointer.
-        if let Place::Projection(box Projection {
-            base,
-            elem: ProjectionElem::Deref,
-        }) = place {
-            if base.ty(self.mir, self.infcx.tcx).ty.is_unsafe_ptr() {
-                return BorrowedContentSource::DerefRawPointer;
-            }
+        // If we didn't find an overloaded deref or index, then assume it's a
+        // built in deref and check the type of the base.
+        let base_ty = deref_base.ty(self.mir, tcx).ty;
+        if base_ty.is_unsafe_ptr() {
+            BorrowedContentSource::DerefRawPointer
+        } else if base_ty.is_mutable_pointer() {
+            BorrowedContentSource::DerefMutableRef
+        } else {
+            BorrowedContentSource::DerefSharedRef
         }
-
-        BorrowedContentSource::Other
     }
 }
diff --git a/src/librustc_mir/util/borrowck_errors.rs b/src/librustc_mir/util/borrowck_errors.rs
index a5dfb736b81..37df368f3b7 100644
--- a/src/librustc_mir/util/borrowck_errors.rs
+++ b/src/librustc_mir/util/borrowck_errors.rs
@@ -399,7 +399,7 @@ pub trait BorrowckErrors<'cx>: Sized + Copy {
         move_from_desc: &str,
         o: Origin,
     ) -> DiagnosticBuilder<'cx> {
-        let mut err = struct_span_err!(
+        let err = struct_span_err!(
             self,
             move_from_span,
             E0507,
@@ -407,10 +407,6 @@ pub trait BorrowckErrors<'cx>: Sized + Copy {
             move_from_desc,
             OGN = o
         );
-        err.span_label(
-            move_from_span,
-            format!("cannot move out of {}", move_from_desc),
-        );
 
         self.cancel_if_wrong_origin(err, o)
     }
@@ -434,8 +430,7 @@ pub trait BorrowckErrors<'cx>: Sized + Copy {
             self,
             move_from_span,
             E0508,
-            "cannot move out of type `{}`, \
-             a non-copy {}{OGN}",
+            "cannot move out of type `{}`, a non-copy {}{OGN}",
             ty,
             type_name,
             OGN = o
@@ -455,8 +450,7 @@ pub trait BorrowckErrors<'cx>: Sized + Copy {
             self,
             move_from_span,
             E0509,
-            "cannot move out of type `{}`, \
-             which implements the `Drop` trait{OGN}",
+            "cannot move out of type `{}`, which implements the `Drop` trait{OGN}",
             container_ty,
             OGN = o
         );