Merge needless_borrow checks into Dereferencing lint pass

Jarcho · Jarcho · commit c530608e186f · 2021-03-03T11:13:59.000-05:00
`needless_borrow` will now check for multiple borrows
`needless_borrow` will now consider mutable references
diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs
@@ -1,11 +1,16 @@
+use crate::needless_borrow::NEEDLESS_BORROW;
 use crate::utils::{
     get_node_span, get_parent_node, in_macro, is_allowed, peel_mid_ty_refs, snippet_with_context, span_lint_and_sugg,
 };
 use rustc_ast::util::parser::PREC_PREFIX;
 use rustc_errors::Applicability;
 use rustc_hir::{BorrowKind, Destination, Expr, ExprKind, HirId, MatchSource, Mutability, Node, UnOp};
 use rustc_lint::{LateContext, LateLintPass};
-use rustc_middle::ty::{self, adjustment::Adjustment, Ty, TyCtxt, TyS, TypeckResults};
+use rustc_middle::ty::{
+    self,
+    adjustment::{Adjust, Adjustment},
+    Ty, TyCtxt, TyS, TypeckResults,
+};
 use rustc_session::{declare_tool_lint, impl_lint_pass};
 use rustc_span::{symbol::sym, Span};
 
@@ -38,6 +43,7 @@ declare_clippy_lint! {
 
 impl_lint_pass!(Dereferencing => [
     EXPLICIT_DEREF_METHODS,
+    NEEDLESS_BORROW,
 ]);
 
 #[derive(Default)]
@@ -64,6 +70,10 @@ enum State {
         ty_changed_count: usize,
         is_final_ufcs: bool,
     },
+    NeedlessBorrow {
+        // The number of borrows remaining
+        remaining: usize,
+    },
 }
 
 // A reference operation considered by this lint pass
@@ -111,15 +121,50 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing {
 
                 let expr_adjustments = find_adjustments(cx.tcx, typeck, expr);
                 let expr_ty = typeck.expr_ty(expr);
-                let target_mut =
-                    if let ty::Ref(_, _, mutability) = *expr_adjustments.last().map_or(expr_ty, |a| a.target).kind() {
+                let data = StateData {
+                    span: expr.span,
+                    target_mut: if let ty::Ref(_, _, mutability) =
+                        *expr_adjustments.last().map_or(expr_ty, |a| a.target).kind()
+                    {
                         mutability
                     } else {
                         Mutability::Not
-                    };
+                    },
+                };
 
-                match kind {
-                    RefOp::Method
+                match (kind, expr_adjustments) {
+                    (
+                        RefOp::AddrOf,
+                        [Adjustment {
+                            kind: Adjust::Deref(None),
+                            target,
+                        }, Adjustment {
+                            kind: Adjust::Deref(None),
+                            ..
+                        }, adjustments @ ..],
+                    ) if target.is_ref() => {
+                        let count = adjustments
+                            .iter()
+                            .take_while(|&a| matches!(a.kind, Adjust::Deref(None)) && a.target.is_ref())
+                            .count();
+                        self.state = Some((
+                            State::NeedlessBorrow {
+                                remaining: if matches!(
+                                    adjustments.get(count),
+                                    Some(Adjustment {
+                                        kind: Adjust::Borrow(_),
+                                        ..
+                                    })
+                                ) {
+                                    count
+                                } else {
+                                    count + 1
+                                },
+                            },
+                            data,
+                        ));
+                    },
+                    (RefOp::Method, _)
                         if !is_allowed(cx, EXPLICIT_DEREF_METHODS, expr.hir_id)
                             && is_linted_explicit_deref_position(parent, expr.hir_id) =>
                     {
@@ -132,10 +177,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing {
                                 },
                                 is_final_ufcs: matches!(expr.kind, ExprKind::Call(..)),
                             },
-                            StateData {
-                                span: expr.span,
-                                target_mut,
-                            },
+                            data,
                         ));
                     }
                     _ => (),
@@ -154,7 +196,14 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing {
                     data,
                 ));
             },
-
+            (Some((State::NeedlessBorrow { remaining }, data)), RefOp::AddrOf) if remaining != 0 => {
+                self.state = Some((
+                    State::NeedlessBorrow {
+                        remaining: remaining - 1,
+                    },
+                    data,
+                ))
+            },
             (Some((state, data)), _) => report(cx, expr, state, data),
         }
     }
@@ -350,5 +399,32 @@ fn report(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, state: State, data: Stat
                 app,
             );
         },
+        State::NeedlessBorrow { .. } => {
+            let ty = cx.typeck_results().expr_ty(expr);
+            let mut app = Applicability::MachineApplicable;
+            let (expr_str, _) = snippet_with_context(cx, expr.span, data.span.ctxt(), "..", &mut app);
+
+            // let reported_ty = if ty.is_ref() {
+            //     ty
+            // } else if let Some(Node::Expr(e)) = get_parent_node(cx.tcx, expr.hir_id) {
+            //     cx.typeck_results().expr_ty(e)
+            // } else {
+            //     ty
+            // };
+
+            span_lint_and_sugg(
+                cx,
+                NEEDLESS_BORROW,
+                data.span,
+                &format!(
+                    "this expression borrows a reference (`{}`) that is immediately dereferenced \
+                            by the compiler",
+                    ty,
+                ),
+                "change this to",
+                expr_str.into(),
+                app,
+            );
+        },
     }
 }
diff --git a/clippy_lints/src/needless_borrow.rs b/clippy_lints/src/needless_borrow.rs
@@ -5,10 +5,9 @@
 use crate::utils::{is_automatically_derived, snippet_opt, span_lint_and_then};
 use if_chain::if_chain;
 use rustc_errors::Applicability;
-use rustc_hir::{BindingAnnotation, BorrowKind, Expr, ExprKind, Item, Mutability, Pat, PatKind};
+use rustc_hir::{BindingAnnotation, Item, Mutability, Pat, PatKind};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::ty;
-use rustc_middle::ty::adjustment::{Adjust, Adjustment};
 use rustc_session::{declare_tool_lint, impl_lint_pass};
 use rustc_span::def_id::LocalDefId;
 
@@ -42,47 +41,6 @@ pub struct NeedlessBorrow {
 impl_lint_pass!(NeedlessBorrow => [NEEDLESS_BORROW]);
 
 impl<'tcx> LateLintPass<'tcx> for NeedlessBorrow {
-    fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) {
-        if e.span.from_expansion() || self.derived_item.is_some() {
-            return;
-        }
-        if let ExprKind::AddrOf(BorrowKind::Ref, Mutability::Not, ref inner) = e.kind {
-            if let ty::Ref(_, ty, _) = cx.typeck_results().expr_ty(inner).kind() {
-                for adj3 in cx.typeck_results().expr_adjustments(e).windows(3) {
-                    if let [Adjustment {
-                        kind: Adjust::Deref(_), ..
-                    }, Adjustment {
-                        kind: Adjust::Deref(_), ..
-                    }, Adjustment {
-                        kind: Adjust::Borrow(_),
-                        ..
-                    }] = *adj3
-                    {
-                        span_lint_and_then(
-                            cx,
-                            NEEDLESS_BORROW,
-                            e.span,
-                            &format!(
-                                "this expression borrows a reference (`&{}`) that is immediately dereferenced \
-                             by the compiler",
-                                ty
-                            ),
-                            |diag| {
-                                if let Some(snippet) = snippet_opt(cx, inner.span) {
-                                    diag.span_suggestion(
-                                        e.span,
-                                        "change this to",
-                                        snippet,
-                                        Applicability::MachineApplicable,
-                                    );
-                                }
-                            },
-                        );
-                    }
-                }
-            }
-        }
-    }
     fn check_pat(&mut self, cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>) {
         if pat.span.from_expansion() || self.derived_item.is_some() {
             return;
diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs
@@ -1788,6 +1788,19 @@ pub fn peel_n_hir_expr_refs(expr: &'a Expr<'a>, count: usize) -> (&'a Expr<'a>,
     f(expr, 0, count)
 }
 
+/// Peels off all references on the type. Returns the underlying type and the number of references
+/// removed.
+pub fn peel_hir_ty_refs(ty: &'tcx hir::Ty<'_>) -> (&'tcx hir::Ty<'tcx>, usize) {
+    fn peel(ty: &'tcx hir::Ty<'_>, count: usize) -> (&'tcx hir::Ty<'tcx>, usize) {
+        if let TyKind::Rptr(_, ty) = &ty.kind {
+            peel(ty.ty, count + 1)
+        } else {
+            (ty, count)
+        }
+    }
+    peel(ty, 0)
+}
+
 /// Peels off all references on the expression. Returns the underlying expression and the number of
 /// references removed.
 pub fn peel_hir_expr_refs(expr: &'a Expr<'a>) -> (&'a Expr<'a>, usize) {
diff --git a/tests/ui/needless_borrow.fixed b/tests/ui/needless_borrow.fixed
@@ -19,15 +19,28 @@ fn main() {
     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 ref_a = &a;
     let garbl = match 42 {
         44 => &a,
         45 => {
             println!("foo");
-            &&a // FIXME: this should lint, too
+            &a
         },
         46 => &a,
+        #[allow(clippy::never_loop)]
+        47 => loop {
+            break ref_a;
+        },
+        48 =>
+        {
+            #[allow(clippy::never_loop)]
+            loop {
+                break ref_a;
+            }
+        },
         _ => panic!(),
     };
+    x(&Box::new(0)); // shouldn't error.
 }
 
 fn f<T: Copy>(y: &T) -> T {
diff --git a/tests/ui/needless_borrow.rs b/tests/ui/needless_borrow.rs
@@ -19,15 +19,28 @@ fn main() {
     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 ref_a = &a;
     let garbl = match 42 {
         44 => &a,
         45 => {
             println!("foo");
-            &&a // FIXME: this should lint, too
+            &&a
         },
         46 => &&a,
+        #[allow(clippy::never_loop)]
+        47 => loop {
+            break &ref_a;
+        },
+        48 =>
+        {
+            #[allow(clippy::never_loop)]
+            loop {
+                break &ref_a;
+            }
+        },
         _ => panic!(),
     };
+    x(&Box::new(0)); // shouldn't error.
 }
 
 fn f<T: Copy>(y: &T) -> T {
diff --git a/tests/ui/needless_borrow.stderr b/tests/ui/needless_borrow.stderr
@@ -13,16 +13,34 @@ 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:13
+   |
+LL |             &&a
+   |             ^^^ help: change this to: `&a`
+
+error: this expression borrows a reference (`&i32`) that is immediately dereferenced by the compiler
+  --> $DIR/needless_borrow.rs:29:15
    |
 LL |         46 => &&a,
    |               ^^^ help: change this to: `&a`
 
+error: this expression borrows a reference (`&i32`) that is immediately dereferenced by the compiler
+  --> $DIR/needless_borrow.rs:32:19
+   |
+LL |             break &ref_a;
+   |                   ^^^^^^ help: change this to: `ref_a`
+
+error: this expression borrows a reference (`&i32`) that is immediately dereferenced by the compiler
+  --> $DIR/needless_borrow.rs:38:23
+   |
+LL |                 break &ref_a;
+   |                       ^^^^^^ help: change this to: `ref_a`
+
 error: this pattern creates a reference to a reference
-  --> $DIR/needless_borrow.rs:51:31
+  --> $DIR/needless_borrow.rs:64: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 7 previous errors
 
