Fix suggestion for let <pat>

clippy_lints/src/needless_pass_by_value.rs

@@ -8,6 +8,7 @@ use rustc::middle::expr_use_visitor as euv;
 use rustc::middle::mem_categorization as mc;
 use syntax::ast::NodeId;
 use syntax_pos::Span;
+use syntax::errors::DiagnosticBuilder;
 use utils::{in_macro, is_self, is_copy, implements_trait, get_trait_def_id, match_type, snippet, span_lint_and_then,
             paths};
 use std::collections::{HashSet, HashMap};
@@ -59,7 +60,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue {
             return;
         }
 
-        // These are usually passed by value and only used by reference
+        // Allows these to be passed by value.
         let fn_trait = cx.tcx.lang_items.fn_trait().expect("failed to find `Fn` trait");
         let asref_trait = get_trait_def_id(cx, &paths::ASREF_TRAIT).expect("failed to find `AsRef` trait");
         let borrow_trait = get_trait_def_id(cx, &paths::BORROW_TRAIT).expect("failed to find `Borrow` trait");
@@ -71,8 +72,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue {
                 .collect()
         };
 
-        // Collect moved variables and non-moving usages at `match`es from the function body
+        // Collect moved variables and spans which will need dereferencings from the function body.
-        let MovedVariablesCtxt { moved_vars, non_moving_matches, .. } = {
+        let MovedVariablesCtxt { moved_vars, spans_need_deref, .. } = {
             let mut ctx = MovedVariablesCtxt::new(cx);
             let infcx = cx.tcx.borrowck_fake_infer_ctxt(body.id());
             {
@@ -119,11 +120,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue {
                     continue;
                 }
 
-                span_lint_and_then(cx,
+                // Suggestion logic
-                          NEEDLESS_PASS_BY_VALUE,
+                let sugg = |db: &mut DiagnosticBuilder| {
-                          input.span,
-                          "this argument is passed by value, but not consumed in the function body",
-                          |db| {
                     if_let_chain! {[
                         match_type(cx, ty, &paths::VEC),
                         let TyPath(QPath::Resolved(_, ref path)) = input.node,
@@ -148,24 +146,27 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue {
                                            format!("&{}", snippet(cx, input.span, "_")));
                     }
 
-                    // For non-moving consumption at `match`es,
+                    // Suggests adding `*` to dereference the added reference.
-                    // suggests adding `*` to dereference the added reference.
+                    if let Some(spans) = spans_need_deref.get(&defid) {
-                    // e.g. `match x { Some(_) => 1, None => 2 }`
+                        let mut spans: Vec<_> = spans.iter().cloned().collect();
-                    //   -> `match *x { .. }`
+                        spans.sort();
-                    if let Some(matches) = non_moving_matches.get(&defid) {
+                        for (i, span) in spans.into_iter().enumerate() {
-                        for (i, m) in matches.iter().enumerate() {
+                            db.span_suggestion(span,
-                            if let ExprMatch(ref e, ..) = cx.tcx.hir.expect_expr(*m).node {
-                                db.span_suggestion(e.span,
                                                 if i == 0 {
                                                     "...and dereference it here"
                                                 } else {
                                                     "...and here"
                                                 },
-                                                   format!("*{}", snippet(cx, e.span, "<expr>")));
+                                                format!("*{}", snippet(cx, span, "<expr>")));
                         }
                     }
-                    }
+                };
-                });
+
+                span_lint_and_then(cx,
+                          NEEDLESS_PASS_BY_VALUE,
+                          input.span,
+                          "this argument is passed by value, but not consumed in the function body",
+                          sugg);
             }}
         }
     }
@@ -174,7 +175,9 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue {
 struct MovedVariablesCtxt<'a, 'tcx: 'a> {
     cx: &'a LateContext<'a, 'tcx>,
     moved_vars: HashSet<DefId>,
-    non_moving_matches: HashMap<DefId, HashSet<NodeId>>,
+    /// Spans which need to be prefixed with `*` for dereferencing the suggested additional
+    /// reference.
+    spans_need_deref: HashMap<DefId, HashSet<Span>>,
 }
 
 impl<'a, 'tcx: 'a> MovedVariablesCtxt<'a, 'tcx> {
@@ -182,7 +185,7 @@ impl<'a, 'tcx: 'a> MovedVariablesCtxt<'a, 'tcx> {
         MovedVariablesCtxt {
             cx: cx,
             moved_vars: HashSet::new(),
-            non_moving_matches: HashMap::new(),
+            spans_need_deref: HashMap::new(),
         }
     }
 
@@ -196,6 +199,57 @@ impl<'a, 'tcx: 'a> MovedVariablesCtxt<'a, 'tcx> {
                 self.moved_vars.insert(def_id);
         }}
     }
+
+    fn non_moving_pat(&mut self, matched_pat: &Pat, cmt: mc::cmt<'tcx>) {
+        let cmt = unwrap_downcast_or_interior(cmt);
+
+        if_let_chain! {[
+            let mc::Categorization::Local(vid) = cmt.cat,
+            let Some(def_id) = self.cx.tcx.hir.opt_local_def_id(vid),
+        ], {
+            let mut id = matched_pat.id;
+            loop {
+                let parent = self.cx.tcx.hir.get_parent_node(id);
+                if id == parent {
+                    // no parent
+                    return;
+                }
+                id = parent;
+
+                if let Some(node) = self.cx.tcx.hir.find(id) {
+                    match node {
+                        map::Node::NodeExpr(e) => {
+                            // `match` and `if let`
+                            if let ExprMatch(ref c, ..) = e.node {
+                                self.spans_need_deref
+                                    .entry(def_id)
+                                    .or_insert_with(HashSet::new)
+                                    .insert(c.span);
+                            }
+                        }
+
+                        map::Node::NodeStmt(s) => {
+                            // `let <pat> = x;`
+                            if_let_chain! {[
+                                let StmtDecl(ref decl, _) = s.node,
+                                let DeclLocal(ref local) = decl.node,
+                            ], {
+                                self.spans_need_deref
+                                    .entry(def_id)
+                                    .or_insert_with(HashSet::new)
+                                    .insert(local.init
+                                        .as_ref()
+                                        .map(|e| e.span)
+                                        .expect("`let` stmt without init aren't caught by match_pat"));
+                            }}
+                        }
+
+                        _ => {}
+                    }
+                }
+            }
+        }}
+    }
 }
 
 impl<'a, 'tcx: 'a> euv::Delegate<'tcx> for MovedVariablesCtxt<'a, 'tcx> {
@@ -209,27 +263,7 @@ impl<'a, 'tcx: 'a> euv::Delegate<'tcx> for MovedVariablesCtxt<'a, 'tcx> {
         if let euv::MatchMode::MovingMatch = mode {
             self.move_common(matched_pat.id, matched_pat.span, cmt);
         } else {
-            let cmt = unwrap_downcast_or_interior(cmt);
+            self.non_moving_pat(matched_pat, cmt);
-
-            if_let_chain! {[
-                let mc::Categorization::Local(vid) = cmt.cat,
-                let Some(def_id) = self.cx.tcx.hir.opt_local_def_id(vid),
-            ], {
-                // Find the `ExprMatch` which contains this pattern
-                let mut match_id = matched_pat.id;
-                loop {
-                    match_id = self.cx.tcx.hir.get_parent_node(match_id);
-                    if_let_chain! {[
-                        let Some(map::Node::NodeExpr(e)) = self.cx.tcx.hir.find(match_id),
-                        let ExprMatch(..) = e.node,
-                    ], {
-                        break;
-                    }}
-                }
-
-                self.non_moving_matches.entry(def_id).or_insert_with(HashSet::new)
-                    .insert(match_id);
-            }}
         }
     }
 
@@ -241,25 +275,18 @@ impl<'a, 'tcx: 'a> euv::Delegate<'tcx> for MovedVariablesCtxt<'a, 'tcx> {
 
     fn borrow(
         &mut self,
-        _borrow_id: NodeId,
+        _: NodeId,
-        _borrow_span: Span,
+        _: Span,
-        _cmt: mc::cmt<'tcx>,
+        _: mc::cmt<'tcx>,
-        _loan_region: &'tcx ty::Region,
+        _: &'tcx ty::Region,
-        _bk: ty::BorrowKind,
+        _: ty::BorrowKind,
-        _loan_cause: euv::LoanCause
+        _: euv::LoanCause
     ) {
     }
 
-    fn mutate(
+    fn mutate(&mut self, _: NodeId, _: Span, _: mc::cmt<'tcx>, _: euv::MutateMode) {}
-        &mut self,
-        _assignment_id: NodeId,
-        _assignment_span: Span,
-        _assignee_cmt: mc::cmt<'tcx>,
-        _mode: euv::MutateMode
-    ) {
-    }
 
-    fn decl_without_init(&mut self, _id: NodeId, _span: Span) {}
+    fn decl_without_init(&mut self, _: NodeId, _: Span) {}
 }
 
 

tests/ui/needless_pass_by_value.rs

@@ -2,7 +2,7 @@
 #![plugin(clippy)]
 
 #![deny(needless_pass_by_value)]
-#![allow(dead_code, single_match)]
+#![allow(dead_code, single_match, if_let_redundant_pattern_matching, many_single_char_names)]
 
 // `v` should be warned
 // `w`, `x` and `y` are allowed (moved or mutated)
@@ -49,10 +49,12 @@ fn test_match(x: Option<Option<String>>, y: Option<Option<String>>) {
     };
 }
 
-// x should be warned, but y is ok
+// x and y should be warned, but z is ok
-fn test_destructure(x: Wrapper, y: Wrapper) {
+fn test_destructure(x: Wrapper, y: Wrapper, z: Wrapper) {
-    let Wrapper(s) = y; // moved
+    let Wrapper(s) = z; // moved
+    let Wrapper(ref t) = y; // not moved
     assert_eq!(x.0.len(), s.len());
+    println!("{}", t);
 }
 
 fn main() {}

tests/ui/needless_pass_by_value.stderr

@@ -53,11 +53,22 @@ help: ...and dereference it here
 error: this argument is passed by value, but not consumed in the function body
   --> $DIR/needless_pass_by_value.rs:53:24
    |
-53 | fn test_destructure(x: Wrapper, y: Wrapper) {
+53 | fn test_destructure(x: Wrapper, y: Wrapper, z: Wrapper) {
    |                        ^^^^^^^
    |
 help: consider taking a reference instead
-   | fn test_destructure(x: &Wrapper, y: Wrapper) {
+   | fn test_destructure(x: &Wrapper, y: Wrapper, z: Wrapper) {
 
-error: aborting due to 6 previous errors
+error: this argument is passed by value, but not consumed in the function body
+  --> $DIR/needless_pass_by_value.rs:53:36
+   |
+53 | fn test_destructure(x: Wrapper, y: Wrapper, z: Wrapper) {
+   |                                    ^^^^^^^
+   |
+help: consider taking a reference instead
+   | fn test_destructure(x: Wrapper, y: &Wrapper, z: Wrapper) {
+help: ...and dereference it here
+   |     let Wrapper(ref t) = *y; // not moved
+
+error: aborting due to 7 previous errors