Improve needless_borrow suggestion for pattern refs

Suggest changing the usages of the binding to match the new type.
Lint in macros in some cases.

clippy_lints/src/needless_borrow.rs

@@ -3,11 +3,14 @@
 //! This lint is **warn** by default
 
 use clippy_utils::diagnostics::span_lint_and_then;
-use clippy_utils::source::snippet_opt;
+use clippy_utils::source::{snippet_opt, snippet_with_applicability, snippet_with_context};
+use clippy_utils::visitors::foreach_local_usage;
+use clippy_utils::{get_parent_expr, get_pat_usable_location};
 use if_chain::if_chain;
 use rustc_errors::Applicability;
-use rustc_hir::{BindingAnnotation, BorrowKind, Expr, ExprKind, Mutability, Pat, PatKind};
-use rustc_lint::{LateContext, LateLintPass};
+use rustc_hir::{BindingAnnotation, BorrowKind, Expr, ExprKind, Mutability, Pat, PatKind, UnOp};
+use rustc_lint::{LateContext, LateLintPass, LintContext};
+use rustc_middle::lint::in_external_macro;
 use rustc_middle::ty;
 use rustc_middle::ty::adjustment::{Adjust, Adjustment};
 use rustc_session::{declare_tool_lint, impl_lint_pass};
@@ -34,9 +37,7 @@ declare_clippy_lint! {
     "taking a reference that is going to be automatically dereferenced"
 }
 
-#[derive(Default)]
 pub struct NeedlessBorrow;
-
 impl_lint_pass!(NeedlessBorrow => [NEEDLESS_BORROW]);
 
 impl<'tcx> LateLintPass<'tcx> for NeedlessBorrow {
@@ -82,31 +83,65 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessBorrow {
         }
     }
     fn check_pat(&mut self, cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>) {
-        if pat.span.from_expansion() {
-            return;
-        }
         if_chain! {
-            if let PatKind::Binding(BindingAnnotation::Ref, .., name, _) = pat.kind;
-            if let ty::Ref(_, tam, mutbl) = *cx.typeck_results().pat_ty(pat).kind();
-            if mutbl == Mutability::Not;
-            if let ty::Ref(_, _, mutbl) = *tam.kind();
+            if let PatKind::Binding(BindingAnnotation::Ref, id, name, _) = pat.kind;
+            if !in_external_macro(cx.sess(), pat.span);
+            if let ty::Ref(_, tam, _) = *cx.typeck_results().pat_ty(pat).kind();
             // only lint immutable refs, because borrowed `&mut T` cannot be moved out
-            if mutbl == Mutability::Not;
+            if let ty::Ref(_, _, Mutability::Not) = *tam.kind();
             then {
+                let (span, search_loc) = match get_pat_usable_location(cx.tcx, pat) {
+                    Some(x) => x,
+                    None => return,
+                };
+                let ctxt = span.ctxt();
+                if pat.span.ctxt() != ctxt {
+                    // The context of the pattern is different than the context using the binding.
+                    // Changing the pattern might affect other code which needs the ref binding.
+                    return;
+                }
+
+                let mut app = Applicability::MachineApplicable;
+                let mut can_fix = true;
+                let mut changes = vec![
+                    (pat.span, snippet_with_context(cx, name.span, ctxt, "..", &mut app).0.into_owned()),
+                ];
+
+                foreach_local_usage(cx, id, search_loc, |e| {
+                    if matches!(
+                        cx.typeck_results().expr_adjustments(e),
+                        [Adjustment { kind: Adjust::Deref(_), .. }, Adjustment { kind: Adjust::Deref(_), .. }, ..]
+                    ) {
+                        // Compiler inserted deref, nothing to change here
+                    } else {
+                        match get_parent_expr(cx, e) {
+                            Some(&Expr { span, kind: ExprKind::Unary(UnOp::Deref, _), .. })
+                            if span.ctxt() == ctxt => {
+                                // Remove explicit deref.
+                                let snip = snippet_with_context(cx, e.span, ctxt, "..", &mut app).0;
+                                changes.push((span, snip.into()));
+                            }
+                            _ if e.span.ctxt() == ctxt => {
+                                // Double reference might be needed at this point.
+                                let snip = snippet_with_applicability(cx, e.span, "..", &mut app);
+                                changes.push((e.span, format!("&{}", snip)));
+                            }
+                            _ => can_fix = false,
+                        }
+                    }
+                });
+
+                if !can_fix {
+                    return;
+                }
+
                 span_lint_and_then(
                     cx,
                     NEEDLESS_BORROW,
                     pat.span,
                     "this pattern creates a reference to a reference",
                     |diag| {
-                        if let Some(snippet) = snippet_opt(cx, name.span) {
-                            diag.span_suggestion(
-                                pat.span,
-                                "change this to",
-                                snippet,
-                                Applicability::MachineApplicable,
-                            );
-                        }
+                        diag.multipart_suggestion("try this", changes, app);
                     }
                 )
             }

clippy_utils/src/lib.rs

@@ -63,9 +63,9 @@ use rustc_hir::def_id::{DefId, LOCAL_CRATE};
 use rustc_hir::intravisit::{self, walk_expr, ErasedMap, NestedVisitorMap, Visitor};
 use rustc_hir::LangItem::{ResultErr, ResultOk};
 use rustc_hir::{
-    def, Arm, BindingAnnotation, Block, Body, Constness, Destination, Expr, ExprKind, FnDecl, GenericArgs, HirId, Impl,
-    ImplItem, ImplItemKind, Item, ItemKind, LangItem, Local, MatchSource, Node, Param, Pat, PatKind, Path, PathSegment,
-    QPath, Stmt, StmtKind, TraitItem, TraitItemKind, TraitRef, TyKind,
+    def, Arm, BindingAnnotation, Block, Body, Constness, Destination, Expr, ExprKind, FnDecl, GenericArgs, Guard,
+    HirId, Impl, ImplItem, ImplItemKind, Item, ItemKind, LangItem, Local, MatchSource, Node, Param, Pat, PatKind, Path,
+    PathSegment, QPath, Stmt, StmtKind, TraitFn, TraitItem, TraitItemKind, TraitRef, TyKind,
 };
 use rustc_lint::{LateContext, Level, Lint, LintContext};
 use rustc_middle::hir::exports::Export;
@@ -1469,6 +1469,82 @@ pub fn run_lints(cx: &LateContext<'_>, lints: &[&'static Lint], id: HirId) -> bo
     })
 }
 
+pub enum PatUsableLocation<'tcx> {
+    Decl,
+    Body(&'tcx Expr<'tcx>),
+    Local(&'tcx [Stmt<'tcx>], Option<&'tcx Expr<'tcx>>),
+    Arm(Option<&'tcx Expr<'tcx>>, &'tcx Expr<'tcx>),
+}
+
+/// Gets all the expressions a pattern binding is visible to, and the span encompassing all the
+/// expression as well as the pattern. Note name shadowing is not considered here.
+pub fn get_pat_usable_location(tcx: TyCtxt<'tcx>, pat: &Pat<'_>) -> Option<(Span, PatUsableLocation<'tcx>)> {
+    let map = tcx.hir();
+    let mut parents = map.parent_iter(pat.hir_id);
+    loop {
+        match parents.next() {
+            Some((_, Node::Pat(_) | Node::Local(_) | Node::Param(_))) => (),
+            Some((_, Node::Arm(arm))) => {
+                break Some((
+                    arm.span,
+                    PatUsableLocation::Arm(
+                        arm.guard.as_ref().map(|&(Guard::If(e) | Guard::IfLet(_, e))| e),
+                        arm.body,
+                    ),
+                ));
+            },
+            Some((id, Node::Stmt(_))) => {
+                if let Some((_, Node::Block(block))) = parents.next() {
+                    let mut stmts = block.stmts.iter();
+                    stmts.find(|s| s.hir_id == id);
+                    break Some((block.span, PatUsableLocation::Local(stmts.as_slice(), block.expr)));
+                }
+                // Should be impossible. Statements can only have blocks as their parent.
+                break None;
+            },
+            Some((
+                _,
+                Node::Expr(&Expr {
+                    span,
+                    kind: ExprKind::Closure(_, _, body, _, _),
+                    ..
+                }),
+            )) => {
+                break Some((span, PatUsableLocation::Body(&map.body(body).value)));
+            },
+            Some((
+                _,
+                Node::Item(&Item {
+                    span,
+                    kind: ItemKind::Fn(.., body),
+                    ..
+                })
+                | Node::ImplItem(&ImplItem {
+                    span,
+                    kind: ImplItemKind::Fn(_, body),
+                    ..
+                }),
+            )) => {
+                break Some((span, PatUsableLocation::Body(&map.body(body).value)));
+            },
+            Some((
+                _,
+                Node::TraitItem(&TraitItem {
+                    span,
+                    kind: TraitItemKind::Fn(_, ref kind),
+                    ..
+                }),
+            )) => {
+                break match *kind {
+                    TraitFn::Required(_) => Some((span, PatUsableLocation::Decl)),
+                    TraitFn::Provided(body) => Some((span, PatUsableLocation::Body(&map.body(body).value))),
+                };
+            },
+            _ => break None,
+        }
+    }
+}
+
 /// Returns Option<String> where String is a textual representation of the type encapsulated in the
 /// slice iff the given expression is a slice of primitives (as defined in the
 /// `is_recursively_primitive_type` function) and None otherwise.

clippy_utils/src/visitors.rs

@@ -1,10 +1,49 @@
-use crate::path_to_local_id;
+use crate::{path_to_local_id, PatUsableLocation};
 use rustc_hir as hir;
 use rustc_hir::intravisit::{self, walk_expr, NestedVisitorMap, Visitor};
 use rustc_hir::{Arm, Body, Expr, HirId, Stmt};
 use rustc_lint::LateContext;
 use rustc_middle::hir::map::Map;
 
+/// A type which can be visited.
+pub trait Visitable<'tcx> {
+    /// Calls the corresponding `visit_*` function on the visitor.
+    fn visit<V: Visitor<'tcx>>(self, visitor: &mut V);
+}
+macro_rules! visitable_ref {
+    ($t:ident, $f:ident) => {
+        impl Visitable<'tcx> for &'tcx $t<'tcx> {
+            fn visit<V: Visitor<'tcx>>(self, visitor: &mut V) {
+                visitor.$f(self);
+            }
+        }
+    };
+}
+visitable_ref!(Stmt, visit_stmt);
+visitable_ref!(Expr, visit_expr);
+impl Visitable<'tcx> for PatUsableLocation<'tcx> {
+    fn visit<V: Visitor<'tcx>>(self, visitor: &mut V) {
+        match self {
+            PatUsableLocation::Decl => (),
+            PatUsableLocation::Arm(guard, body) => {
+                if let Some(guard) = guard {
+                    visitor.visit_expr(guard);
+                }
+                visitor.visit_expr(body)
+            },
+            PatUsableLocation::Body(body) => visitor.visit_expr(body),
+            PatUsableLocation::Local(stmts, expr) => {
+                for stmt in stmts {
+                    visitor.visit_stmt(stmt);
+                }
+                if let Some(expr) = expr {
+                    visitor.visit_expr(expr);
+                }
+            },
+        }
+    }
+}
+
 /// returns `true` if expr contains match expr desugared from try
 fn contains_try(expr: &hir::Expr<'_>) -> bool {
     struct TryFinder {
@@ -188,3 +227,34 @@ impl<'v> Visitor<'v> for LocalUsedVisitor<'v> {
         NestedVisitorMap::OnlyBodies(self.hir)
     }
 }
+
+/// Calls the given function for each usage of the given local.
+pub fn foreach_local_usage<'tcx>(
+    cx: &LateContext<'tcx>,
+    local: HirId,
+    visit: impl Visitable<'tcx>,
+    f: impl FnMut(&'tcx Expr<'tcx>),
+) {
+    struct V<'a, 'tcx, F> {
+        cx: &'a LateContext<'tcx>,
+        local: HirId,
+        f: F,
+    }
+    impl<'tcx, F: FnMut(&'tcx Expr<'tcx>)> Visitor<'tcx> for V<'_, 'tcx, F> {
+        type Map = Map<'tcx>;
+        fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
+            NestedVisitorMap::OnlyBodies(self.cx.tcx.hir())
+        }
+
+        fn visit_expr(&mut self, e: &'tcx Expr<'_>) {
+            if path_to_local_id(e, self.local) {
+                (self.f)(e);
+            } else {
+                walk_expr(self, e)
+            }
+        }
+    }
+
+    let mut v = V { cx, local, f };
+    visit.visit(&mut v);
+}

tests/ui/needless_borrow.fixed

@@ -18,7 +18,6 @@ fn main() {
     let vec = Vec::new();
     let vec_val = g(&vec); // should not error, because `&Vec<T>` derefs to `&[T]`
     h(&"foo"); // should not error, because the `&&str` is required, due to `&Trait`
-    if let Some(cake) = Some(&5) {}
     let garbl = match 42 {
         44 => &a,
         45 => {
@@ -43,19 +42,3 @@ trait Trait {}
 impl<'a> Trait for &'a str {}
 
 fn h(_: &dyn Trait) {}
-#[warn(clippy::needless_borrow)]
-#[allow(dead_code)]
-fn issue_1432() {
-    let mut v = Vec::<String>::new();
-    let _ = v.iter_mut().filter(|&ref a| a.is_empty());
-    let _ = v.iter().filter(|&a| a.is_empty());
-
-    let _ = v.iter().filter(|&a| a.is_empty());
-}
-
-#[allow(dead_code)]
-#[warn(clippy::needless_borrow)]
-#[derive(Debug)]
-enum Foo<'a> {
-    Str(&'a str),
-}

tests/ui/needless_borrow.rs

@@ -18,7 +18,6 @@ fn main() {
     let vec = Vec::new();
     let vec_val = g(&vec); // should not error, because `&Vec<T>` derefs to `&[T]`
     h(&"foo"); // should not error, because the `&&str` is required, due to `&Trait`
-    if let Some(ref cake) = Some(&5) {}
     let garbl = match 42 {
         44 => &a,
         45 => {
@@ -43,19 +42,3 @@ trait Trait {}
 impl<'a> Trait for &'a str {}
 
 fn h(_: &dyn Trait) {}
-#[warn(clippy::needless_borrow)]
-#[allow(dead_code)]
-fn issue_1432() {
-    let mut v = Vec::<String>::new();
-    let _ = v.iter_mut().filter(|&ref a| a.is_empty());
-    let _ = v.iter().filter(|&ref a| a.is_empty());
-
-    let _ = v.iter().filter(|&a| a.is_empty());
-}
-
-#[allow(dead_code)]
-#[warn(clippy::needless_borrow)]
-#[derive(Debug)]
-enum Foo<'a> {
-    Str(&'a str),
-}

tests/ui/needless_borrow.stderr

@@ -6,23 +6,11 @@ LL |     let c = x(&&a);
    |
    = note: `-D clippy::needless-borrow` implied by `-D warnings`
 
-error: this pattern creates a reference to a reference
-  --> $DIR/needless_borrow.rs:21:17
-   |
-LL |     if let Some(ref cake) = Some(&5) {}
-   |                 ^^^^^^^^ help: change this to: `cake`
-
 error: this expression borrows a reference (`&i32`) that is immediately dereferenced by the compiler
-  --> $DIR/needless_borrow.rs:28:15
+  --> $DIR/needless_borrow.rs:27:15
    |
 LL |         46 => &&a,
    |               ^^^ help: change this to: `&a`
 
-error: this pattern creates a reference to a reference
-  --> $DIR/needless_borrow.rs:51:31
-   |
-LL |     let _ = v.iter().filter(|&ref a| a.is_empty());
-   |                               ^^^^^ help: change this to: `a`
-
-error: aborting due to 4 previous errors
+error: aborting due to 2 previous errors