Handle closures with type annotations on args

ThibsG · ThibsG · commit 1176b8e5e9e6 · 2021-11-20T09:40:11.000+01:00
diff --git a/clippy_lints/src/methods/search_is_some.rs b/clippy_lints/src/methods/search_is_some.rs
@@ -7,7 +7,7 @@ use clippy_utils::{get_parent_expr_for_hir, is_trait_method, strip_pat_refs};
 use if_chain::if_chain;
 use rustc_errors::Applicability;
 use rustc_hir as hir;
-use rustc_hir::{self, ExprKind, HirId, PatKind};
+use rustc_hir::{self, ExprKind, HirId, MutTy, PatKind, TyKind};
 use rustc_infer::infer::TyCtxtInferExt;
 use rustc_lint::LateContext;
 use rustc_middle::hir::place::ProjectionKind;
@@ -52,26 +52,12 @@ pub(super) fn check<'tcx>(
                 then {
                     if let hir::PatKind::Ref(..) = closure_arg.pat.kind {
                         Some(search_snippet.replacen('&', "", 1))
-                    } else if let PatKind::Binding(_, binding_id, _, _) = strip_pat_refs(closure_arg.pat).kind {
-                        // this binding is composed of at least two levels of references, so we need to remove one
-                        let binding_type = cx.typeck_results().node_type(binding_id);
-                        let innermost_is_ref = if let ty::Ref(_, inner,_) = binding_type.kind() {
-                            matches!(inner.kind(), ty::Ref(_, innermost, _) if innermost.is_ref())
-                        } else {
-                            false
-                        };
-
+                    } else if let PatKind::Binding(..) = strip_pat_refs(closure_arg.pat).kind {
                         // `find()` provides a reference to the item, but `any` does not,
                         // so we should fix item usages for suggestion
-                        if let Some(closure_sugg) = get_closure_suggestion(cx, search_arg, closure_body) {
+                        if let Some(closure_sugg) = get_closure_suggestion(cx, search_arg) {
                             applicability = closure_sugg.applicability;
-                            if innermost_is_ref {
-                                Some(closure_sugg.suggestion.replacen('&', "", 1))
-                            } else {
-                                Some(closure_sugg.suggestion)
-                            }
-                        } else if innermost_is_ref {
-                            Some(search_snippet.replacen('&', "", 1))
+                            Some(closure_sugg.suggestion)
                         } else {
                             Some(search_snippet.to_string())
                         }
@@ -174,6 +160,7 @@ pub(super) fn check<'tcx>(
     }
 }
 
+#[derive(Debug)]
 struct ClosureSugg {
     applicability: Applicability,
     suggestion: String,
@@ -182,38 +169,45 @@ struct ClosureSugg {
 // Build suggestion gradually by handling closure arg specific usages,
 // such as explicit deref and borrowing cases.
 // Returns `None` if no such use cases have been triggered in closure body
-fn get_closure_suggestion<'tcx>(
-    cx: &LateContext<'_>,
-    search_arg: &'tcx hir::Expr<'_>,
-    closure_body: &hir::Body<'_>,
-) -> Option<ClosureSugg> {
-    let mut visitor = DerefDelegate {
-        cx,
-        closure_span: search_arg.span,
-        next_pos: search_arg.span.lo(),
-        suggestion_start: String::new(),
-        applicability: Applicability::MachineApplicable,
-    };
+fn get_closure_suggestion<'tcx>(cx: &LateContext<'_>, search_expr: &'tcx hir::Expr<'_>) -> Option<ClosureSugg> {
+    if let hir::ExprKind::Closure(_, fn_decl, body_id, ..) = search_expr.kind {
+        let closure_body = cx.tcx.hir().body(body_id);
+        // is closure arg a double reference (i.e.: `|x: &&i32| ...`)
+        let closure_arg_is_double_ref = if let TyKind::Rptr(_, MutTy { ty, .. }) = fn_decl.inputs[0].kind {
+            matches!(ty.kind, TyKind::Rptr(_, MutTy { .. }))
+        } else {
+            false
+        };
+
+        let mut visitor = DerefDelegate {
+            cx,
+            closure_span: search_expr.span,
+            closure_arg_is_double_ref,
+            next_pos: search_expr.span.lo(),
+            suggestion_start: String::new(),
+            applicability: Applicability::MachineApplicable,
+        };
 
-    let fn_def_id = cx.tcx.hir().local_def_id(search_arg.hir_id);
-    cx.tcx.infer_ctxt().enter(|infcx| {
-        ExprUseVisitor::new(&mut visitor, &infcx, fn_def_id, cx.param_env, cx.typeck_results())
-            .consume_body(closure_body);
-    });
+        let fn_def_id = cx.tcx.hir().local_def_id(search_expr.hir_id);
+        cx.tcx.infer_ctxt().enter(|infcx| {
+            ExprUseVisitor::new(&mut visitor, &infcx, fn_def_id, cx.param_env, cx.typeck_results())
+                .consume_body(closure_body);
+        });
 
-    if visitor.suggestion_start.is_empty() {
-        None
-    } else {
-        Some(ClosureSugg {
-            applicability: visitor.applicability,
-            suggestion: visitor.finish(),
-        })
+        if !visitor.suggestion_start.is_empty() {
+            return Some(ClosureSugg {
+                applicability: visitor.applicability,
+                suggestion: visitor.finish(),
+            });
+        }
     }
+    None
 }
 
 struct DerefDelegate<'a, 'tcx> {
     cx: &'a LateContext<'tcx>,
     closure_span: Span,
+    closure_arg_is_double_ref: bool,
     next_pos: BytePos,
     suggestion_start: String,
     applicability: Applicability,
@@ -223,7 +217,12 @@ impl DerefDelegate<'_, 'tcx> {
     pub fn finish(&mut self) -> String {
         let end_span = Span::new(self.next_pos, self.closure_span.hi(), self.closure_span.ctxt(), None);
         let end_snip = snippet_with_applicability(self.cx, end_span, "..", &mut self.applicability);
-        format!("{}{}", self.suggestion_start, end_snip)
+        let sugg = format!("{}{}", self.suggestion_start, end_snip);
+        if self.closure_arg_is_double_ref {
+            sugg.replacen('&', "", 1)
+        } else {
+            sugg
+        }
     }
 
     fn func_takes_arg_by_double_ref(&self, parent_expr: &'tcx hir::Expr<'_>, cmt_hir_id: HirId) -> bool {
@@ -261,6 +260,7 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> {
             if cmt.place.projections.is_empty() {
                 // handle item without any projection, that needs an explicit borrowing
                 // i.e.: suggest `&x` instead of `x`
+                self.closure_arg_is_double_ref = false;
                 self.suggestion_start.push_str(&format!("{}&{}", start_snip, ident_str));
             } else {
                 // cases where a parent `Call` or `MethodCall` is using the item
@@ -272,29 +272,43 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> {
                 // - `self` arguments in the case of `x.is_something()` are also automatically (de)referenced, and
                 //   no projection should be suggested
                 if let Some(parent_expr) = get_parent_expr_for_hir(self.cx, cmt.hir_id) {
-                    if let ExprKind::Call(_, call_args) | ExprKind::MethodCall(_, _, call_args, _) = parent_expr.kind {
-                        let expr = self.cx.tcx.hir().expect_expr(cmt.hir_id);
-                        let arg_ty_kind = self.cx.typeck_results().expr_ty(expr).kind();
-
-                        if matches!(arg_ty_kind, ty::Ref(_, _, Mutability::Not)) {
-                            // suggest ampersand if call function is taking args by double reference
-                            let takes_arg_by_double_ref = self.func_takes_arg_by_double_ref(parent_expr, cmt.hir_id);
-
-                            // do not suggest ampersand if the ident is the method caller
-                            let ident_sugg = if !call_args.is_empty()
-                                && call_args[0].hir_id == cmt.hir_id
-                                && !takes_arg_by_double_ref
-                            {
-                                format!("{}{}", start_snip, ident_str)
-                            } else {
-                                format!("{}&{}", start_snip, ident_str)
-                            };
-                            self.suggestion_start.push_str(&ident_sugg);
+                    match &parent_expr.kind {
+                        // given expression is the self argument and will be handled completely by the compiler
+                        // i.e.: `|x| x.is_something()`
+                        ExprKind::MethodCall(_, _, [self_expr, ..], _) if self_expr.hir_id == cmt.hir_id => {
+                            self.suggestion_start.push_str(&format!("{}{}", start_snip, ident_str));
                             self.next_pos = span.hi();
                             return;
-                        }
+                        },
+                        // item is used in a call
+                        // i.e.: `Call`: `|x| please(x)` or `MethodCall`: `|x| [1, 2, 3].contains(x)`
+                        ExprKind::Call(_, [call_args @ ..]) | ExprKind::MethodCall(_, _, [_, call_args @ ..], _) => {
+                            let expr = self.cx.tcx.hir().expect_expr(cmt.hir_id);
+                            let arg_ty_kind = self.cx.typeck_results().expr_ty(expr).kind();
 
-                        self.applicability = Applicability::Unspecified;
+                            if matches!(arg_ty_kind, ty::Ref(_, _, Mutability::Not)) {
+                                // suggest ampersand if call function is taking args by double reference
+                                let takes_arg_by_double_ref =
+                                    self.func_takes_arg_by_double_ref(parent_expr, cmt.hir_id);
+
+                                // no need to bind again if the function doesn't take arg by double ref
+                                // and if the item is already a double ref
+                                let ident_sugg = if !call_args.is_empty()
+                                    && !takes_arg_by_double_ref
+                                    && self.closure_arg_is_double_ref
+                                {
+                                    format!("{}{}", start_snip, ident_str)
+                                } else {
+                                    format!("{}&{}", start_snip, ident_str)
+                                };
+                                self.suggestion_start.push_str(&ident_sugg);
+                                self.next_pos = span.hi();
+                                return;
+                            }
+
+                            self.applicability = Applicability::Unspecified;
+                        },
+                        _ => (),
                     }
                 }
 
@@ -346,7 +360,9 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> {
 
                 // handle `ProjectionKind::Deref` by removing one explicit deref
                 // if no special case was detected (i.e.: suggest `*x` instead of `**x`)
-                if !projections_handled {
+                if projections_handled {
+                    self.closure_arg_is_double_ref = false;
+                } else {
                     let last_deref = cmt
                         .place
                         .projections
diff --git a/tests/ui/search_is_some_fixable_none.fixed b/tests/ui/search_is_some_fixable_none.fixed
@@ -118,9 +118,12 @@ mod issue7392 {
 
         let v = vec![3, 2, 1, 0];
         let _ = !v.iter().any(|x| deref_enough(*x));
+        let _ = !v.iter().any(|x: &u32| deref_enough(*x));
 
         #[allow(clippy::redundant_closure)]
         let _ = !v.iter().any(|x| arg_no_deref(&x));
+        #[allow(clippy::redundant_closure)]
+        let _ = !v.iter().any(|x: &u32| arg_no_deref(&x));
     }
 
     fn field_index_projection() {
diff --git a/tests/ui/search_is_some_fixable_none.rs b/tests/ui/search_is_some_fixable_none.rs
@@ -122,9 +122,12 @@ mod issue7392 {
 
         let v = vec![3, 2, 1, 0];
         let _ = v.iter().find(|x| deref_enough(**x)).is_none();
+        let _ = v.iter().find(|x: &&u32| deref_enough(**x)).is_none();
 
         #[allow(clippy::redundant_closure)]
         let _ = v.iter().find(|x| arg_no_deref(x)).is_none();
+        #[allow(clippy::redundant_closure)]
+        let _ = v.iter().find(|x: &&u32| arg_no_deref(x)).is_none();
     }
 
     fn field_index_projection() {
diff --git a/tests/ui/search_is_some_fixable_none.stderr b/tests/ui/search_is_some_fixable_none.stderr
@@ -188,13 +188,25 @@ LL |         let _ = v.iter().find(|x| deref_enough(**x)).is_none();
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|x| deref_enough(*x))`
 
 error: called `is_none()` after searching an `Iterator` with `find`
-  --> $DIR/search_is_some_fixable_none.rs:127:17
+  --> $DIR/search_is_some_fixable_none.rs:125:17
+   |
+LL |         let _ = v.iter().find(|x: &&u32| deref_enough(**x)).is_none();
+   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|x: &u32| deref_enough(*x))`
+
+error: called `is_none()` after searching an `Iterator` with `find`
+  --> $DIR/search_is_some_fixable_none.rs:128:17
    |
 LL |         let _ = v.iter().find(|x| arg_no_deref(x)).is_none();
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|x| arg_no_deref(&x))`
 
 error: called `is_none()` after searching an `Iterator` with `find`
-  --> $DIR/search_is_some_fixable_none.rs:147:17
+  --> $DIR/search_is_some_fixable_none.rs:130:17
+   |
+LL |         let _ = v.iter().find(|x: &&u32| arg_no_deref(x)).is_none();
+   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|x: &u32| arg_no_deref(&x))`
+
+error: called `is_none()` after searching an `Iterator` with `find`
+  --> $DIR/search_is_some_fixable_none.rs:150:17
    |
 LL |           let _ = vfoo
    |  _________________^
@@ -210,34 +222,34 @@ LL ~             .iter().any(|v| v.inner_double.bar[0][0] == 2 && v.inner.bar[0]
    |
 
 error: called `is_none()` after searching an `Iterator` with `find`
-  --> $DIR/search_is_some_fixable_none.rs:163:17
+  --> $DIR/search_is_some_fixable_none.rs:166:17
    |
 LL |         let _ = vfoo.iter().find(|v| v.inner[0].bar == 2).is_none();
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!vfoo.iter().any(|v| v.inner[0].bar == 2)`
 
 error: called `is_none()` after searching an `Iterator` with `find`
-  --> $DIR/search_is_some_fixable_none.rs:168:17
+  --> $DIR/search_is_some_fixable_none.rs:171:17
    |
 LL |         let _ = vfoo.iter().find(|x| (**x)[0] == 9).is_none();
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!vfoo.iter().any(|x| (**x)[0] == 9)`
 
 error: called `is_none()` after searching an `Iterator` with `find`
-  --> $DIR/search_is_some_fixable_none.rs:181:17
+  --> $DIR/search_is_some_fixable_none.rs:184:17
    |
 LL |         let _ = vfoo.iter().find(|v| v.by_ref(&v.bar)).is_none();
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!vfoo.iter().any(|v| v.by_ref(&v.bar))`
 
 error: called `is_none()` after searching an `Iterator` with `find`
-  --> $DIR/search_is_some_fixable_none.rs:185:17
+  --> $DIR/search_is_some_fixable_none.rs:188:17
    |
 LL |         let _ = [&(&1, 2), &(&3, 4), &(&5, 4)].iter().find(|(&x, y)| x == *y).is_none();
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `![&(&1, 2), &(&3, 4), &(&5, 4)].iter().any(|(&x, y)| x == *y)`
 
 error: called `is_none()` after searching an `Iterator` with `find`
-  --> $DIR/search_is_some_fixable_none.rs:186:17
+  --> $DIR/search_is_some_fixable_none.rs:189:17
    |
 LL |         let _ = [&(&1, 2), &(&3, 4), &(&5, 4)].iter().find(|&(&x, y)| x == *y).is_none();
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `![&(&1, 2), &(&3, 4), &(&5, 4)].iter().any(|(&x, y)| x == *y)`
 
-error: aborting due to 36 previous errors
+error: aborting due to 38 previous errors
 
diff --git a/tests/ui/search_is_some_fixable_some.fixed b/tests/ui/search_is_some_fixable_some.fixed
@@ -119,9 +119,12 @@ mod issue7392 {
 
         let v = vec![3, 2, 1, 0];
         let _ = v.iter().any(|x| deref_enough(*x));
+        let _ = v.iter().any(|x: &u32| deref_enough(*x));
 
         #[allow(clippy::redundant_closure)]
         let _ = v.iter().any(|x| arg_no_deref(&x));
+        #[allow(clippy::redundant_closure)]
+        let _ = v.iter().any(|x: &u32| arg_no_deref(&x));
     }
 
     fn field_index_projection() {
diff --git a/tests/ui/search_is_some_fixable_some.rs b/tests/ui/search_is_some_fixable_some.rs
@@ -121,9 +121,12 @@ mod issue7392 {
 
         let v = vec![3, 2, 1, 0];
         let _ = v.iter().find(|x| deref_enough(**x)).is_some();
+        let _ = v.iter().find(|x: &&u32| deref_enough(**x)).is_some();
 
         #[allow(clippy::redundant_closure)]
         let _ = v.iter().find(|x| arg_no_deref(x)).is_some();
+        #[allow(clippy::redundant_closure)]
+        let _ = v.iter().find(|x: &&u32| arg_no_deref(x)).is_some();
     }
 
     fn field_index_projection() {
diff --git a/tests/ui/search_is_some_fixable_some.stderr b/tests/ui/search_is_some_fixable_some.stderr
