Improve needless_borrow

Use the traversal done by the linting framework instead of a visitor to find binding usages.
Only lint once when a binding appears multiple times in an `or` pattern.
Don't suggest adding a reference before accessing a field.

Author: Jarcho

clippy_lints/src/lib.rs

@@ -1088,7 +1088,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| box zero_div_zero::ZeroDiv);
     store.register_late_pass(|| box mutex_atomic::Mutex);
     store.register_late_pass(|| box needless_update::NeedlessUpdate);
-    store.register_late_pass(|| box needless_borrow::NeedlessBorrow);
+    store.register_late_pass(|| box needless_borrow::NeedlessBorrow::default());
     store.register_late_pass(|| box needless_borrowed_ref::NeedlessBorrowedRef);
     store.register_late_pass(|| box no_effect::NoEffect);
     store.register_late_pass(|| box temporary_assignment::TemporaryAssignment);

clippy_lints/src/needless_borrow.rs

@@ -4,16 +4,21 @@
 
 use clippy_utils::diagnostics::span_lint_and_then;
 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 clippy_utils::{get_parent_expr, path_to_local};
 use if_chain::if_chain;
+use rustc_ast::util::parser::PREC_POSTFIX;
+use rustc_data_structures::fx::FxIndexMap;
 use rustc_errors::Applicability;
-use rustc_hir::{BindingAnnotation, BorrowKind, Expr, ExprKind, Mutability, Pat, PatKind, UnOp};
-use rustc_lint::{LateContext, LateLintPass, LintContext};
+use rustc_hir::{
+    BindingAnnotation, Block, Body, BodyId, BorrowKind, Expr, ExprKind, HirId, ImplItem, Item, Mutability, Node, Pat,
+    PatKind, TraitItem, UnOp,
+};
+use rustc_lint::{LateContext, LateLintPass, Lint, LintContext};
 use rustc_middle::lint::in_external_macro;
 use rustc_middle::ty;
 use rustc_middle::ty::adjustment::{Adjust, Adjustment};
-use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_session::{declare_tool_lint, impl_lint_pass};
+use rustc_span::{Span, SyntaxContext};
 
 declare_clippy_lint! {
     /// **What it does:** Checks for address of operations (`&`) that are going to
@@ -63,10 +68,36 @@ declare_clippy_lint! {
     "`ref` binding to a reference"
 }
 
-declare_lint_pass!(NeedlessBorrow => [NEEDLESS_BORROW, REF_BINDING_TO_REFERENCE]);
+impl_lint_pass!(NeedlessBorrow => [NEEDLESS_BORROW, REF_BINDING_TO_REFERENCE]);
+#[derive(Default)]
+pub struct NeedlessBorrow {
+    /// The body the first local was found in. Used to emit lints when the traversal of the body has
+    /// been finished. Note we can't lint at the end of every body as they can be nested within each
+    /// other.
+    current_body: Option<BodyId>,
+    /// The list of locals currently being checked by the lint.
+    ref_locals: FxIndexMap<HirId, Option<RefPat>>,
+}
+
+struct RefPat {
+    /// The lint to use for this suggestion. Either `needless_borrow` or `ref_binding_to_reference`.
+    lint: &'static Lint,
+    /// The spans of all the ref bindings for this local.
+    spans: Vec<Span>,
+    /// The context replacements are to be made in.
+    ctxt: SyntaxContext,
+    /// The applicability of this suggestion.
+    app: Applicability,
+    /// All the replacements which need to be made.
+    replacements: Vec<(Span, String)>,
+}
 
 impl<'tcx> LateLintPass<'tcx> for NeedlessBorrow {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) {
+        if let Some(local) = path_to_local(e) {
+            self.check_local_usage(cx, e, local);
+        }
+
         if e.span.from_expansion() {
             return;
         }
@@ -107,70 +138,154 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessBorrow {
             }
         }
     }
+
     fn check_pat(&mut self, cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>) {
-        if_chain! {
-            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 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;
+        if let PatKind::Binding(BindingAnnotation::Ref, id, name, _) = pat.kind {
+            if let Some(opt_prev_pat) = self.ref_locals.get_mut(&id) {
+                // This binding id has been seen before. Add this pattern to the list of changes.
+                if let Some(prev_pat) = opt_prev_pat {
+                    if prev_pat.ctxt == pat.span.ctxt() {
+                        prev_pat.spans.push(pat.span);
+                        prev_pat.replacements.push((
+                            pat.span,
+                            snippet_with_context(cx, name.span, prev_pat.ctxt, "..", &mut prev_pat.app)
+                                .0
+                                .into(),
+                        ));
+                    } else {
+                        // Doesn't match the context of the previous pattern. Can't lint here.
+                        *opt_prev_pat = None;
+                    }
                 }
+                return;
+            }
 
-                let mut app = Applicability::MachineApplicable;
-                let mut can_fix = true;
-                let mut lint = NEEDLESS_BORROW;
-                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| {
-                    // Check for auto-deref
-                    if !matches!(
-                        cx.typeck_results().expr_adjustments(e),
-                        [Adjustment { kind: Adjust::Deref(_), .. }, Adjustment { kind: Adjust::Deref(_), .. }, ..]
-                    ) {
-                        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.
-                                lint = REF_BINDING_TO_REFERENCE;
-                                let snip = snippet_with_applicability(cx, e.span, "..", &mut app);
-                                changes.push((e.span, format!("&{}", snip)));
+            if_chain! {
+                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 let ty::Ref(_, _, Mutability::Not) = *tam.kind();
+                then {
+                    let map = cx.tcx.hir();
+                    let mut iter = map.parent_iter(pat.hir_id);
+                    let expr_ctxt = loop {
+                        // Find the context shared by the pattern, and it's scope use site.
+                        match iter.next() {
+                            None => {
+                                // This shouldn't happen, but mark this as unfixable if it does.
+                                self.ref_locals.insert(id, None);
+                                return;
                             }
-                            _ => can_fix = false,
+                            Some((_, Node::ImplItem(ImplItem { span, .. })
+                                | Node::TraitItem(TraitItem { span, .. })
+                                | Node::Item(Item { span, .. })
+                                | Node::Block( Block { span, ..})
+                                | Node::Expr( Expr { span, .. }))) => break span.ctxt(),
+                            _ => (),
                         }
-                    }
-                });
+                    };
 
-                if !can_fix {
-                    return;
+                    if pat.span.ctxt() == expr_ctxt {
+                        let mut app = Applicability::MachineApplicable;
+                        let snip = snippet_with_context(cx, name.span, expr_ctxt, "..", &mut app).0;
+                        self.current_body = self.current_body.or(cx.enclosing_body);
+                        self.ref_locals.insert(
+                            id,
+                            Some(RefPat {
+                                lint: NEEDLESS_BORROW,
+                                spans: vec![pat.span],
+                                ctxt: expr_ctxt,
+                                app,
+                                replacements: vec![(pat.span, snip.into())],
+                            }),
+                        );
+                    } else {
+                        // 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.
+                        // Mark the local as having been seen, but not fixable.
+                        self.ref_locals.insert(id, None);
+                    }
                 }
+            }
+        }
+    }
 
+    fn check_body_post(&mut self, cx: &LateContext<'tcx>, body: &'tcx Body<'_>) {
+        if Some(body.id()) == self.current_body {
+            for pat in self.ref_locals.drain(..).filter_map(|(_, x)| x) {
+                let replacements = pat.replacements;
+                let app = pat.app;
                 span_lint_and_then(
                     cx,
-                    lint,
-                    pat.span,
+                    pat.lint,
+                    pat.spans,
                     "this pattern creates a reference to a reference",
                     |diag| {
-                        diag.multipart_suggestion("try this", changes, app);
-                    }
+                        diag.multipart_suggestion("try this", replacements, app);
+                    },
                 )
             }
+            self.current_body = None;
+        }
+    }
+}
+impl NeedlessBorrow {
+    fn check_local_usage(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>, local: HirId) {
+        if let Some(outer_pat) = self.ref_locals.get_mut(&local) {
+            if let Some(pat) = outer_pat {
+                // Check for auto-deref
+                if !matches!(
+                    cx.typeck_results().expr_adjustments(e),
+                    [
+                        Adjustment {
+                            kind: Adjust::Deref(_),
+                            ..
+                        },
+                        Adjustment {
+                            kind: Adjust::Deref(_),
+                            ..
+                        },
+                        ..
+                    ]
+                ) {
+                    match get_parent_expr(cx, e) {
+                        // Field accesses are the same no matter the number of references.
+                        Some(Expr {
+                            kind: ExprKind::Field(..),
+                            ..
+                        }) => (),
+                        Some(&Expr {
+                            span,
+                            kind: ExprKind::Unary(UnOp::Deref, _),
+                            ..
+                        }) if span.ctxt() == pat.ctxt => {
+                            // Remove explicit deref.
+                            let snip = snippet_with_context(cx, e.span, pat.ctxt, "..", &mut pat.app).0;
+                            pat.replacements.push((span, snip.into()));
+                        },
+                        Some(parent) if parent.span.ctxt() == pat.ctxt => {
+                            // Double reference might be needed at this point.
+                            if parent.precedence().order() == PREC_POSTFIX {
+                                // Parenthesis would be needed here, don't lint.
+                                *outer_pat = None;
+                            } else {
+                                let snip = snippet_with_context(cx, e.span, pat.ctxt, "..", &mut pat.app).0;
+                                pat.replacements.push((e.span, format!("&{}", snip)));
+                            }
+                        },
+                        _ if e.span.ctxt() == pat.ctxt => {
+                            // Double reference might be needed at this point.
+                            pat.lint = REF_BINDING_TO_REFERENCE;
+                            let snip = snippet_with_applicability(cx, e.span, "..", &mut pat.app);
+                            pat.replacements.push((e.span, format!("&{}", snip)));
+                        },
+                        // Edge case for macros. The span of the identifier will usually match the context of the
+                        // binding, but not if the identifier was created in a macro. e.g. `concat_idents` and proc
+                        // macros
+                        _ => *outer_pat = None,
+                    }
+                }
+            }
         }
     }
 }

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, Guard,
-    HirId, Impl, ImplItem, ImplItemKind, Item, ItemKind, LangItem, Local, MatchSource, Node, Param, Pat, PatKind, Path,
-    PathSegment, QPath, Stmt, StmtKind, TraitFn, TraitItem, TraitItemKind, TraitRef, TyKind,
+    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,
 };
 use rustc_lint::{LateContext, Level, Lint, LintContext};
 use rustc_middle::hir::exports::Export;
@@ -1469,77 +1469,6 @@ 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::Item(&Item {
-                    span,
-                    kind: ItemKind::Fn(.., body),
-                    ..
-                })
-                | Node::ImplItem(&ImplItem {
-                    span,
-                    kind: ImplItemKind::Fn(_, body),
-                    ..
-                })
-                | Node::Expr(&Expr {
-                    span,
-                    kind: ExprKind::Closure(_, _, 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.