Suggest match ergonomics, not ref/ref mut

ashtneoi · ashtneoi · commit a05f82fd2d6f · 2018-08-15T15:14:21.000-07:00
diff --git a/src/librustc/mir/mod.rs b/src/librustc/mir/mod.rs
@@ -523,6 +523,8 @@ pub struct VarBindingForm<'tcx> {
     /// (b) it gives a way to separate this case from the remaining cases
     ///     for diagnostics.
     pub opt_match_place: Option<(Option<Place<'tcx>>, Span)>,
+    /// Span of the pattern in which this variable was bound.
+    pub pat_span: Span,
 }
 
 #[derive(Clone, PartialEq, Eq, Hash, Debug, RustcEncodable, RustcDecodable)]
@@ -540,7 +542,8 @@ CloneTypeFoldableAndLiftImpls! { BindingForm<'tcx>, }
 impl_stable_hash_for!(struct self::VarBindingForm<'tcx> {
     binding_mode,
     opt_ty_info,
-    opt_match_place
+    opt_match_place,
+    pat_span
 });
 
 mod binding_form_impl {
@@ -710,6 +713,7 @@ impl<'tcx> LocalDecl<'tcx> {
                 binding_mode: ty::BindingMode::BindByValue(_),
                 opt_ty_info: _,
                 opt_match_place: _,
+                pat_span: _,
             }))) => true,
 
             // FIXME: might be able to thread the distinction between
@@ -729,6 +733,7 @@ impl<'tcx> LocalDecl<'tcx> {
                 binding_mode: ty::BindingMode::BindByValue(_),
                 opt_ty_info: _,
                 opt_match_place: _,
+                pat_span: _,
             }))) => true,
 
             Some(ClearCrossCrate::Set(BindingForm::ImplicitSelf)) => true,
diff --git a/src/librustc_borrowck/borrowck/mod.rs b/src/librustc_borrowck/borrowck/mod.rs
@@ -1235,7 +1235,7 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
                     ty::BindByReference(..) => {
                         let let_span = self.tcx.hir.span(node_id);
                         let suggestion = suggest_ref_mut(self.tcx, let_span);
-                        if let Some((let_span, replace_str)) = suggestion {
+                        if let Some(replace_str) = suggestion {
                             db.span_suggestion(
                                 let_span,
                                 "use a mutable reference instead",
diff --git a/src/librustc_mir/borrow_check/move_errors.rs b/src/librustc_mir/borrow_check/move_errors.rs
@@ -8,7 +8,7 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-use rustc::hir;
+use core::unicode::property::Pattern_White_Space;
 use rustc::mir::*;
 use rustc::ty;
 use rustc_errors::DiagnosticBuilder;
@@ -36,25 +36,27 @@ use util::borrowck_errors::{BorrowckErrors, Origin};
 // let (&x, &y) = (&String::new(), &String::new());
 #[derive(Debug)]
 enum GroupedMoveError<'tcx> {
-    // Match place can't be moved from
+    // Place expression can't be moved from,
     // e.g. match x[0] { s => (), } where x: &[String]
-    MovesFromMatchPlace {
+    MovesFromPlace {
         original_path: Place<'tcx>,
         span: Span,
         move_from: Place<'tcx>,
         kind: IllegalMoveOriginKind<'tcx>,
         binds_to: Vec<Local>,
     },
-    // Part of a pattern can't be moved from,
+    // Part of a value expression can't be moved from,
     // e.g. match &String::new() { &x => (), }
-    MovesFromPattern {
+    MovesFromValue {
         original_path: Place<'tcx>,
         span: Span,
         move_from: MovePathIndex,
         kind: IllegalMoveOriginKind<'tcx>,
         binds_to: Vec<Local>,
     },
     // Everything that isn't from pattern matching.
+    // FIXME(ashtneoi): I think this is only for moves into temporaries, as
+    // when returning a value. Clarification needed.
     OtherIllegalMove {
         original_path: Place<'tcx>,
         span: Span,
@@ -119,6 +121,7 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
                         opt_match_place: Some((ref opt_match_place, match_span)),
                         binding_mode: _,
                         opt_ty_info: _,
+                        pat_span: _,
                     }))) = local_decl.is_user_variable
                     {
                         self.append_binding_error(
@@ -166,7 +169,7 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
             // Error with the match place
             LookupResult::Parent(_) => {
                 for ge in &mut *grouped_errors {
-                    if let GroupedMoveError::MovesFromMatchPlace { span, binds_to, .. } = ge {
+                    if let GroupedMoveError::MovesFromPlace { span, binds_to, .. } = ge {
                         if match_span == *span {
                             debug!("appending local({:?}) to list", bind_to);
                             if !binds_to.is_empty() {
@@ -184,7 +187,7 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
                 } else {
                     (vec![bind_to], match_span)
                 };
-                grouped_errors.push(GroupedMoveError::MovesFromMatchPlace {
+                grouped_errors.push(GroupedMoveError::MovesFromPlace {
                     span,
                     move_from: match_place.clone(),
                     original_path,
@@ -200,7 +203,7 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
                     _ => unreachable!("Probably not unreachable..."),
                 };
                 for ge in &mut *grouped_errors {
-                    if let GroupedMoveError::MovesFromPattern {
+                    if let GroupedMoveError::MovesFromValue {
                         span,
                         move_from: other_mpi,
                         binds_to,
@@ -215,7 +218,7 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
                     }
                 }
                 debug!("found a new move error location");
-                grouped_errors.push(GroupedMoveError::MovesFromPattern {
+                grouped_errors.push(GroupedMoveError::MovesFromValue {
                     span: match_span,
                     move_from: mpi,
                     original_path,
@@ -230,13 +233,13 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
         let (mut err, err_span) = {
             let (span, original_path, kind): (Span, &Place<'tcx>, &IllegalMoveOriginKind) =
                 match error {
-                    GroupedMoveError::MovesFromMatchPlace {
+                    GroupedMoveError::MovesFromPlace {
                         span,
                         ref original_path,
                         ref kind,
                         ..
                     } |
-                    GroupedMoveError::MovesFromPattern { span, ref original_path, ref kind, .. } |
+                    GroupedMoveError::MovesFromValue { span, ref original_path, ref kind, .. } |
                     GroupedMoveError::OtherIllegalMove { span, ref original_path, ref kind } => {
                         (span, original_path, kind)
                     },
@@ -331,111 +334,119 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
         err: &mut DiagnosticBuilder<'a>,
         span: Span,
     ) {
+        let snippet = self.tcx.sess.codemap().span_to_snippet(span).unwrap();
         match error {
-            GroupedMoveError::MovesFromMatchPlace {
+            GroupedMoveError::MovesFromPlace {
                 mut binds_to,
                 move_from,
                 ..
             } => {
-                // Ok to suggest a borrow, since the target can't be moved from
-                // anyway.
-                if let Ok(snippet) = self.tcx.sess.codemap().span_to_snippet(span) {
-                    match move_from {
-                        Place::Projection(ref proj)
-                            if self.suitable_to_remove_deref(proj, &snippet) =>
-                        {
+                let mut suggest_change_head_expr = false;
+                match move_from {
+                    Place::Projection(box PlaceProjection {
+                        elem: ProjectionElem::Deref,
+                        ..
+                    }) => {
+                        // This is false for (e.g.) index expressions `a[b]`,
+                        // which roughly desugar to `*Index::index(&a, b)` or
+                        // `*IndexMut::index_mut(&mut a, b)`.
+                        if snippet.starts_with('*') {
                             err.span_suggestion(
                                 span,
                                 "consider removing this dereference operator",
                                 (&snippet[1..]).to_owned(),
                             );
-                        }
-                        _ => {
-                            err.span_suggestion(
-                                span,
-                                "consider using a reference instead",
-                                format!("&{}", snippet),
-                            );
+                            suggest_change_head_expr = true;
                         }
                     }
-
-                    binds_to.sort();
-                    binds_to.dedup();
-                    for local in binds_to {
-                        let bind_to = &self.mir.local_decls[local];
-                        let binding_span = bind_to.source_info.span;
-                        err.span_label(
-                            binding_span,
-                            format!(
-                                "move occurs because {} has type `{}`, \
-                                 which does not implement the `Copy` trait",
-                                bind_to.name.unwrap(),
-                                bind_to.ty
-                            ),
+                    _ => {
+                        err.span_suggestion(
+                            span,
+                            "consider using a reference instead",
+                            format!("&{}", snippet),
                         );
+                        suggest_change_head_expr = true;
                     }
                 }
+                binds_to.sort();
+                binds_to.dedup();
+                if !suggest_change_head_expr {
+                    self.add_move_error_suggestions(err, &binds_to);
+                }
+                self.add_move_error_labels(err, &binds_to);
             }
-            GroupedMoveError::MovesFromPattern { mut binds_to, .. } => {
-                // Suggest ref, since there might be a move in
-                // another match arm
+            GroupedMoveError::MovesFromValue { mut binds_to, .. } => {
                 binds_to.sort();
                 binds_to.dedup();
-                let mut multipart_suggestion = Vec::with_capacity(binds_to.len());
-                for (j, local) in binds_to.into_iter().enumerate() {
-                    let bind_to = &self.mir.local_decls[local];
-                    let binding_span = bind_to.source_info.span;
+                self.add_move_error_suggestions(err, &binds_to);
+                self.add_move_error_labels(err, &binds_to);
+            }
+            // No binding. Nothing to suggest.
+            GroupedMoveError::OtherIllegalMove { .. } => (),
+        }
+    }
 
-                    // Suggest ref mut when the user has already written mut.
-                    let ref_kind = match bind_to.mutability {
-                        Mutability::Not => "ref",
-                        Mutability::Mut => "ref mut",
-                    };
-                    if j == 0 {
-                        err.span_label(binding_span, format!("data moved here"));
+    fn add_move_error_suggestions(
+        &self,
+        err: &mut DiagnosticBuilder<'a>,
+        binds_to: &[Local],
+    ) {
+        for local in binds_to {
+            let bind_to = &self.mir.local_decls[*local];
+            if let Some(
+                ClearCrossCrate::Set(BindingForm::Var(VarBindingForm {
+                    pat_span,
+                    ..
+                }))
+            ) = bind_to.is_user_variable {
+                let pat_snippet = self
+                    .tcx.sess.codemap()
+                    .span_to_snippet(pat_span)
+                    .unwrap();
+                if pat_snippet.starts_with('&') {
+                    let pat_snippet = &pat_snippet[1..];
+                    let suggestion;
+                    if pat_snippet.starts_with("mut")
+                        && pat_snippet["mut".len()..].starts_with(Pattern_White_Space)
+                    {
+                        suggestion = pat_snippet["mut".len()..].trim_left();
                     } else {
-                        err.span_label(binding_span, format!("... and here"));
-                    }
-                    match bind_to.name {
-                        Some(name) => {
-                            multipart_suggestion.push((binding_span,
-                                                       format!("{} {}", ref_kind, name)));
-                        }
-                        None => {
-                            err.span_label(
-                                span,
-                                format!("Local {:?} is not suitable for ref", bind_to),
-                            );
-                        }
+                        suggestion = pat_snippet;
                     }
+                    err.span_suggestion(
+                        pat_span,
+                        "consider removing this borrow operator",
+                        suggestion.to_owned(),
+                    );
                 }
-                err.multipart_suggestion("to prevent move, use ref or ref mut",
-                                         multipart_suggestion);
             }
-            // Nothing to suggest.
-            GroupedMoveError::OtherIllegalMove { .. } => (),
         }
     }
 
-    fn suitable_to_remove_deref(&self, proj: &PlaceProjection<'tcx>, snippet: &str) -> bool {
-        let is_shared_ref = |ty: ty::Ty| match ty.sty {
-            ty::TypeVariants::TyRef(.., hir::Mutability::MutImmutable) => true,
-            _ => false,
-        };
+    fn add_move_error_labels(
+        &self,
+        err: &mut DiagnosticBuilder<'a>,
+        binds_to: &[Local],
+    ) {
+        for (j, local) in binds_to.into_iter().enumerate() {
+            let bind_to = &self.mir.local_decls[*local];
+            let binding_span = bind_to.source_info.span;
 
-        proj.elem == ProjectionElem::Deref && snippet.starts_with('*') && match proj.base {
-            Place::Local(local) => {
-                let local_decl = &self.mir.local_decls[local];
-                // If this is a temporary, then this could be from an
-                // overloaded * operator.
-                local_decl.is_user_variable.is_some() && is_shared_ref(local_decl.ty)
+            if j == 0 {
+                err.span_label(binding_span, format!("data moved here"));
+            } else {
+                err.span_label(binding_span, format!("... and here"));
             }
-            Place::Promoted(_) => true,
-            Place::Static(ref st) => is_shared_ref(st.ty),
-            Place::Projection(ref proj) => match proj.elem {
-                ProjectionElem::Field(_, ty) => is_shared_ref(ty),
-                _ => false,
-            },
+
+            err.span_note(
+                binding_span,
+                &format!(
+                    "move occurs because {} has type `{}`, \
+                        which does not implement the `Copy` trait",
+                    bind_to.name.unwrap(),
+                    bind_to.ty
+                ),
+            );
         }
     }
 }
diff --git a/src/librustc_mir/borrow_check/mutability_errors.rs b/src/librustc_mir/borrow_check/mutability_errors.rs
@@ -329,7 +329,11 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
                     ClearCrossCrate::Set(mir::BindingForm::Var(mir::VarBindingForm {
                         binding_mode: ty::BindingMode::BindByReference(_),
                         ..
-                    })) => suggest_ref_mut(self.tcx, local_decl.source_info.span),
+                    })) => {
+                        let pattern_span = local_decl.source_info.span;
+                        suggest_ref_mut(self.tcx, pattern_span)
+                            .map(|replacement| (pattern_span, replacement))
+                    }
 
                     //
                     ClearCrossCrate::Set(mir::BindingForm::RefForGuard) => unreachable!(),
diff --git a/src/librustc_mir/build/matches/mod.rs b/src/librustc_mir/build/matches/mod.rs
@@ -357,7 +357,8 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
             let visibility_scope = visibility_scope.unwrap();
             this.declare_binding(source_info, visibility_scope, mutability, name, mode,
                                  num_patterns, var, ty, has_guard,
-                                 opt_match_place.map(|(x, y)| (x.cloned(), y)));
+                                 opt_match_place.map(|(x, y)| (x.cloned(), y)),
+                                 patterns[0].span);
         });
         visibility_scope
     }
@@ -1182,7 +1183,8 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                        var_id: NodeId,
                        var_ty: Ty<'tcx>,
                        has_guard: ArmHasGuard,
-                       opt_match_place: Option<(Option<Place<'tcx>>, Span)>)
+                       opt_match_place: Option<(Option<Place<'tcx>>, Span)>,
+                       pat_span: Span)
     {
         debug!("declare_binding(var_id={:?}, name={:?}, mode={:?}, var_ty={:?}, \
                 visibility_scope={:?}, source_info={:?})",
@@ -1208,6 +1210,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                 // Instead, just abandon providing diagnostic info.
                 opt_ty_info: None,
                 opt_match_place,
+                pat_span,
             }))),
         };
         let for_arm_body = self.local_decls.push(local.clone());
diff --git a/src/librustc_mir/build/mod.rs b/src/librustc_mir/build/mod.rs
@@ -763,6 +763,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                                     binding_mode,
                                     opt_ty_info,
                                     opt_match_place: Some((Some(place.clone()), span)),
+                                    pat_span: span,
                                 })))
                             };
                         self.var_indices.insert(var, LocalsForNode::One(local));
diff --git a/src/librustc_mir/lib.rs b/src/librustc_mir/lib.rs
@@ -35,6 +35,7 @@ Rust MIR: a lowered representation of Rust. Also: an experiment!
 #![feature(try_trait)]
 #![feature(unicode_internals)]
 #![feature(step_trait)]
+#![feature(slice_concat_ext)]
 
 #![recursion_limit="256"]
 
diff --git a/src/librustc_mir/util/mod.rs b/src/librustc_mir/util/mod.rs
diff --git a/src/test/ui/suggestions/dont-suggest-ref.rs b/src/test/ui/suggestions/dont-suggest-ref.rs
diff --git a/src/test/ui/suggestions/dont-suggest-ref.stderr b/src/test/ui/suggestions/dont-suggest-ref.stderr
