Auto merge of #41564 - gaurikholkar:master, r=nikomatsakis

Disable ref hint for pattern in let and adding ui tests #40402

A fix to #40402

The `to prevent move, use ref e or ref mut e ` has been disabled.
```
fn main() {
    let v = vec![String::from("oh no")];

    let e = v[0];
}
```
now gives
```
error[E0507]: cannot move out of indexed content
 --> example.rs:4:13
  |
4 |     let e = v[0];
  |             ^^^^ cannot move out of indexed content

error: aborting due to previous error
```
I have added ui tests for the same and also modified a compile-fail test.

Author: bors

src/librustc_borrowck/borrowck/gather_loans/gather_moves.rs

@@ -11,7 +11,7 @@
 //! Computes moves.
 
 use borrowck::*;
-use borrowck::gather_loans::move_error::MoveSpanAndPath;
+use borrowck::gather_loans::move_error::MovePlace;
 use borrowck::gather_loans::move_error::{MoveError, MoveErrorCollector};
 use borrowck::move_data::*;
 use rustc::middle::expr_use_visitor as euv;
@@ -23,13 +23,67 @@ use rustc::ty::{self, Ty};
 use std::rc::Rc;
 use syntax::ast;
 use syntax_pos::Span;
-use rustc::hir::{self, PatKind};
+use rustc::hir::*;
+use rustc::hir::map::Node::*;
 
 struct GatherMoveInfo<'tcx> {
     id: ast::NodeId,
     kind: MoveKind,
     cmt: mc::cmt<'tcx>,
-    span_path_opt: Option<MoveSpanAndPath>
+    span_path_opt: Option<MovePlace<'tcx>>
+}
+
+/// Represents the kind of pattern
+#[derive(Debug, Clone, Copy)]
+pub enum PatternSource<'tcx> {
+    MatchExpr(&'tcx Expr),
+    LetDecl(&'tcx Local),
+    Other,
+}
+
+/// Analyzes the context where the pattern appears to determine the
+/// kind of hint we want to give. In particular, if the pattern is in a `match`
+/// or nested within other patterns, we want to suggest a `ref` binding:
+///
+///     let (a, b) = v[0]; // like the `a` and `b` patterns here
+///     match v[0] { a => ... } // or the `a` pattern here
+///
+/// But if the pattern is the outermost pattern in a `let`, we would rather
+/// suggest that the author add a `&` to the initializer:
+///
+///     let x = v[0]; // suggest `&v[0]` here
+///
+/// In this latter case, this function will return `PatternSource::LetDecl`
+/// with a reference to the let
+fn get_pattern_source<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, pat: &Pat) -> PatternSource<'tcx> {
+
+    let parent = tcx.hir.get_parent_node(pat.id);
+
+    match tcx.hir.get(parent) {
+        NodeExpr(ref e) => {
+            // the enclosing expression must be a `match` or something else
+            assert!(match e.node {
+                        ExprMatch(..) => true,
+                        _ => return PatternSource::Other,
+                    });
+            PatternSource::MatchExpr(e)
+        }
+        NodeStmt(ref s) => {
+            // the enclosing statement must be a `let` or something else
+            match s.node {
+                StmtDecl(ref decl, _) => {
+                    match decl.node {
+                        DeclLocal(ref local) => PatternSource::LetDecl(local),
+                        _ => return PatternSource::Other,
+                    }
+                }
+                _ => return PatternSource::Other,
+            }
+        }
+
+        _ => return PatternSource::Other,
+
+    }
 }
 
 pub fn gather_decl<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>,
@@ -95,11 +149,15 @@ pub fn gather_move_from_pat<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>,
                                       move_error_collector: &mut MoveErrorCollector<'tcx>,
                                       move_pat: &hir::Pat,
                                       cmt: mc::cmt<'tcx>) {
+    let source = get_pattern_source(bccx.tcx,move_pat);
     let pat_span_path_opt = match move_pat.node {
         PatKind::Binding(_, _, ref path1, _) => {
-            Some(MoveSpanAndPath{span: move_pat.span,
-                                 name: path1.node})
-        },
+            Some(MovePlace {
+                     span: move_pat.span,
+                     name: path1.node,
+                     pat_source: source,
+                 })
+        }
         _ => None,
     };
     let move_info = GatherMoveInfo {
@@ -108,6 +166,11 @@ pub fn gather_move_from_pat<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>,
         cmt: cmt,
         span_path_opt: pat_span_path_opt,
     };
+
+    debug!("gather_move_from_pat: move_pat={:?} source={:?}",
+           move_pat,
+           source);
+
     gather_move(bccx, move_data, move_error_collector, move_info);
 }
 

src/librustc_borrowck/borrowck/gather_loans/move_error.rs

@@ -17,6 +17,7 @@ use rustc::ty;
 use syntax::ast;
 use syntax_pos;
 use errors::DiagnosticBuilder;
+use borrowck::gather_loans::gather_moves::PatternSource;
 
 pub struct MoveErrorCollector<'tcx> {
     errors: Vec<MoveError<'tcx>>
@@ -40,12 +41,12 @@ impl<'tcx> MoveErrorCollector<'tcx> {
 
 pub struct MoveError<'tcx> {
     move_from: mc::cmt<'tcx>,
-    move_to: Option<MoveSpanAndPath>
+    move_to: Option<MovePlace<'tcx>>
 }
 
 impl<'tcx> MoveError<'tcx> {
     pub fn with_move_info(move_from: mc::cmt<'tcx>,
-                          move_to: Option<MoveSpanAndPath>)
+                          move_to: Option<MovePlace<'tcx>>)
                           -> MoveError<'tcx> {
         MoveError {
             move_from: move_from,
@@ -55,30 +56,41 @@ impl<'tcx> MoveError<'tcx> {
 }
 
 #[derive(Clone)]
-pub struct MoveSpanAndPath {
+pub struct MovePlace<'tcx> {
     pub span: syntax_pos::Span,
     pub name: ast::Name,
+    pub pat_source: PatternSource<'tcx>,
 }
 
 pub struct GroupedMoveErrors<'tcx> {
     move_from: mc::cmt<'tcx>,
-    move_to_places: Vec<MoveSpanAndPath>
+    move_to_places: Vec<MovePlace<'tcx>>
 }
 
-fn report_move_errors<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>,
-                                errors: &Vec<MoveError<'tcx>>) {
+fn report_move_errors<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>, errors: &Vec<MoveError<'tcx>>) {
     let grouped_errors = group_errors_with_same_origin(errors);
     for error in &grouped_errors {
         let mut err = report_cannot_move_out_of(bccx, error.move_from.clone());
         let mut is_first_note = true;
-        for move_to in &error.move_to_places {
-            err = note_move_destination(err, move_to.span, move_to.name, is_first_note);
-            is_first_note = false;
+        match error.move_to_places.get(0) {
+            Some(&MovePlace { pat_source: PatternSource::LetDecl(_), .. }) => {
+                // ignore patterns that are found at the top-level of a `let`;
+                // see `get_pattern_source()` for details
+            }
+            _ => {
+                for move_to in &error.move_to_places {
+
+                    err = note_move_destination(err, move_to.span, move_to.name, is_first_note);
+                    is_first_note = false;
+                }
+            }
         }
         if let NoteClosureEnv(upvar_id) = error.move_from.note {
-            err.span_label(bccx.tcx.hir.span(upvar_id.var_id), &"captured outer variable");
+            err.span_label(bccx.tcx.hir.span(upvar_id.var_id),
+                           &"captured outer variable");
         }
         err.emit();
+
     }
 }
 

src/test/compile-fail/borrowck/borrowck-vec-pattern-nesting.rs

@@ -54,7 +54,6 @@ fn c() {
         _ => {}
     }
     let a = vec[0]; //~ ERROR cannot move out
-    //~^ NOTE to prevent move
     //~| cannot move out of here
 }
 
@@ -68,7 +67,6 @@ fn d() {
         _ => {}
     }
     let a = vec[0]; //~ ERROR cannot move out
-    //~^ NOTE to prevent move
     //~| cannot move out of here
 }
 
@@ -84,7 +82,6 @@ fn e() {
         _ => {}
     }
     let a = vec[0]; //~ ERROR cannot move out
-    //~^ NOTE to prevent move
     //~| cannot move out of here
 }
 

src/test/ui/issue-40402-ref-hints/issue-40402-1.rs

@@ -0,0 +1,20 @@
+// Copyright 2016 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.
+
+// Check that we do not suggest `ref f` here in the `main()` function.
+struct Foo {
+    pub v: Vec<String>,
+}
+
+fn main() {
+    let mut f = Foo { v: Vec::new() };
+    f.v.push("hello".to_string());
+    let e = f.v[0];
+}

src/test/ui/issue-40402-ref-hints/issue-40402-1.stderr

@@ -0,0 +1,8 @@
+error[E0507]: cannot move out of indexed content
+  --> $DIR/issue-40402-1.rs:19:13
+   |
+19 |     let e = f.v[0];
+   |             ^^^^^^ cannot move out of indexed content
+
+error: aborting due to previous error
+

src/test/ui/issue-40402-ref-hints/issue-40402-2.rs

@@ -0,0 +1,16 @@
+// Copyright 2016 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.
+
+// Check that we do suggest `(ref a, ref b)` here, since `a` and `b`
+// are nested within a pattern
+fn main() {
+    let x = vec![(String::new(), String::new())];
+    let (a, b) = x[0];
+}

src/test/ui/issue-40402-ref-hints/issue-40402-2.stderr

@@ -0,0 +1,11 @@
+error[E0507]: cannot move out of indexed content
+  --> $DIR/issue-40402-2.rs:15:18
+   |
+15 |     let (a, b) = x[0];
+   |          -  -    ^^^^ cannot move out of indexed content
+   |          |  |
+   |          |  ...and here (use `ref b` or `ref mut b`)
+   |          hint: to prevent move, use `ref a` or `ref mut a`
+
+error: aborting due to previous error
+