]> git.lizzy.rs Git - rust.git/blobdiff - clippy_lints/src/methods/search_is_some.rs
Correct suggestion when dereferencing enough, calling a function
[rust.git] / clippy_lints / src / methods / search_is_some.rs
index ec09bfd743d0a3f3344dcb08c436bcf122b8e92d..98ad7f3866fd5d40a1f1de36711663fc7afda194 100644 (file)
@@ -5,7 +5,7 @@
 use if_chain::if_chain;
 use rustc_errors::Applicability;
 use rustc_hir as hir;
-use rustc_hir::{self, Expr, ExprKind, HirId, PatKind};
+use rustc_hir::{self, ExprKind, HirId, PatKind};
 use rustc_infer::infer::TyCtxtInferExt;
 use rustc_lint::LateContext;
 use rustc_middle::hir::place::ProjectionKind;
@@ -41,6 +41,7 @@ pub(super) fn check<'tcx>(
         if search_snippet.lines().count() <= 1 {
             // suggest `any(|x| ..)` instead of `any(|&x| ..)` for `find(|&x| ..).is_some()`
             // suggest `any(|..| *..)` instead of `any(|..| **..)` for `find(|..| **..).is_some()`
+            let mut applicability = Applicability::MachineApplicable;
             let any_search_snippet = if_chain! {
                 if search_method == "find";
                 if let hir::ExprKind::Closure(_, _, body_id, ..) = search_arg.kind;
@@ -49,11 +50,29 @@ 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(..) = strip_pat_refs(closure_arg.pat).kind {
+                    } 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
+                        };
+
                         // `find()` provides a reference to the item, but `any` does not,
                         // so we should fix item usages for suggestion
-                        get_closure_suggestion(cx, search_arg, closure_body)
-                            .or_else(|| Some(search_snippet.to_string()))
+                        if let Some(closure_sugg) = get_closure_suggestion(cx, search_arg, closure_body) {
+                            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))
+                        } else {
+                            Some(search_snippet.to_string())
+                        }
                     } else {
                         None
                     }
@@ -73,7 +92,7 @@ pub(super) fn check<'tcx>(
                         "any({})",
                         any_search_snippet.as_ref().map_or(&*search_snippet, String::as_str)
                     ),
-                    Applicability::MachineApplicable,
+                    applicability,
                 );
             } else {
                 let iter = snippet(cx, search_recv.span, "..");
@@ -88,7 +107,7 @@ pub(super) fn check<'tcx>(
                         iter,
                         any_search_snippet.as_ref().map_or(&*search_snippet, String::as_str)
                     ),
-                    Applicability::MachineApplicable,
+                    applicability,
                 );
             }
         } else {
@@ -153,6 +172,11 @@ pub(super) fn check<'tcx>(
     }
 }
 
+struct ClosureSugg {
+    applicability: Applicability,
+    suggestion: String,
+}
+
 // 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
@@ -160,13 +184,12 @@ fn get_closure_suggestion<'tcx>(
     cx: &LateContext<'_>,
     search_arg: &'tcx hir::Expr<'_>,
     closure_body: &hir::Body<'_>,
-) -> Option<String> {
+) -> Option<ClosureSugg> {
     let mut visitor = DerefDelegate {
         cx,
         closure_span: search_arg.span,
-        next_pos: None,
+        next_pos: search_arg.span.lo(),
         suggestion_start: String::new(),
-        suggestion_end: String::new(),
         applicability: Applicability::MachineApplicable,
     };
 
@@ -179,19 +202,29 @@ fn get_closure_suggestion<'tcx>(
     if visitor.suggestion_start.is_empty() {
         None
     } else {
-        Some(format!("{}{}", visitor.suggestion_start, visitor.suggestion_end))
+        Some(ClosureSugg {
+            applicability: visitor.applicability,
+            suggestion: visitor.finish(),
+        })
     }
 }
 
 struct DerefDelegate<'a, 'tcx> {
     cx: &'a LateContext<'tcx>,
     closure_span: Span,
-    next_pos: Option<BytePos>,
+    next_pos: BytePos,
     suggestion_start: String,
-    suggestion_end: String,
     applicability: Applicability,
 }
 
+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());
+        let end_snip = snippet_with_applicability(self.cx, end_span, "..", &mut self.applicability);
+        format!("{}{}", self.suggestion_start, end_snip)
+    }
+}
+
 impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> {
     fn consume(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) {}
 
@@ -200,14 +233,8 @@ fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind) {
             let map = self.cx.tcx.hir();
             let ident_str = map.name(id).to_string();
             let span = map.span(cmt.hir_id);
-            let start_span = if let Some(next_pos) = self.next_pos {
-                Span::new(next_pos, span.lo(), span.ctxt())
-            } else {
-                self.closure_span.until(span)
-            };
-            let start_snip = snippet_with_applicability(self.cx, start_span, "..", &mut self.applicability);
-            let end_span = Span::new(span.hi(), self.closure_span.hi(), span.ctxt());
-            let end_snip = snippet_with_applicability(self.cx, end_span, "..", &mut self.applicability);
+            let start_span = Span::new(self.next_pos, span.lo(), span.ctxt());
+            let mut start_snip = snippet_with_applicability(self.cx, start_span, "..", &mut self.applicability);
 
             if cmt.place.projections.is_empty() {
                 // handle item without any projection, that needs an explicit borrowing
@@ -216,30 +243,26 @@ fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind) {
             } else {
                 // cases where a parent call is using the item
                 // i.e.: suggest `.contains(&x)` for `.find(|x| [1, 2, 3].contains(x)).is_none()`
-                let parent_expr = get_parent_expr_for_hir(self.cx, cmt.hir_id);
-                if let Some(Expr { hir_id: _, kind, .. }) = parent_expr {
-                    if let ExprKind::Call(_, args) | ExprKind::MethodCall(_, _, args, _) = kind {
-                        let args_to_handle = args.iter().filter(|arg| arg.hir_id == cmt.hir_id).collect::<Vec<_>>();
-                        if !args_to_handle.is_empty() {
-                            for arg in &args_to_handle {
-                                let arg_ty_kind = self.cx.typeck_results().expr_ty(arg).kind();
-                                if matches!(arg_ty_kind, ty::Ref(_, _, Mutability::Not)) {
-                                    let start_span = if let Some(next_pos) = self.next_pos {
-                                        Span::new(next_pos, span.lo(), span.ctxt())
-                                    } else {
-                                        self.closure_span.until(span)
-                                    };
-                                    let start_snip =
-                                        snippet_with_applicability(self.cx, start_span, "..", &mut self.applicability);
+                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();
 
-                                    self.suggestion_start.push_str(&format!("{}&{}", start_snip, ident_str));
-                                    self.suggestion_end = end_snip.to_string();
-                                    self.next_pos = Some(span.hi());
-                                } else {
-                                    self.applicability = Applicability::Unspecified;
-                                }
-                            }
+                        if matches!(arg_ty_kind, ty::Ref(_, _, Mutability::Not)) {
+                            let start_span = Span::new(self.next_pos, span.lo(), span.ctxt());
+                            let start_snip =
+                                snippet_with_applicability(self.cx, start_span, "..", &mut self.applicability);
+                            // 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 {
+                                format!("{}{}", start_snip, ident_str)
+                            } else {
+                                format!("{}&{}", start_snip, ident_str)
+                            };
+                            self.suggestion_start.push_str(&ident_sugg);
+                            self.next_pos = span.hi();
                             return;
+                        } else {
+                            self.applicability = Applicability::Unspecified;
                         }
                     }
                 }
@@ -247,28 +270,83 @@ fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind) {
                 // handle item projections by removing one explicit deref
                 // i.e.: suggest `*x` instead of `**x`
                 let mut replacement_str = ident_str;
-                let last_deref = cmt
-                    .place
-                    .projections
-                    .iter()
-                    .rposition(|proj| proj.kind == ProjectionKind::Deref);
 
-                if let Some(pos) = last_deref {
-                    let mut projections = cmt.place.projections.clone();
-                    projections.truncate(pos);
+                // handle index projection first
+                let index_handled = cmt.place.projections.iter().any(|proj| match proj.kind {
+                    // Index projection like `|x| foo[x]`
+                    // the index is dropped so we can't get it to build the suggestion,
+                    // so the span is set-up again to get more code, using `span.hi()` (i.e.: `foo[x]`)
+                    // instead of `span.lo()` (i.e.: `foo`)
+                    ProjectionKind::Index => {
+                        let start_span = Span::new(self.next_pos, span.hi(), span.ctxt());
+                        start_snip = snippet_with_applicability(self.cx, start_span, "..", &mut self.applicability);
+                        replacement_str.clear();
+                        true
+                    },
+                    _ => false,
+                });
 
-                    for item in projections {
-                        if item.kind == ProjectionKind::Deref {
-                            replacement_str = format!("*{}", replacement_str);
+                // looking for projections other that need to be handled differently
+                let other_projections_handled = cmt.place.projections.iter().enumerate().any(|(i, proj)| {
+                    match proj.kind {
+                        // Field projection like `|v| v.foo`
+                        ProjectionKind::Field(idx, variant) => match cmt.place.ty_before_projection(i).kind() {
+                            ty::Adt(def, ..) => {
+                                replacement_str = format!(
+                                    "{}.{}",
+                                    replacement_str,
+                                    def.variants[variant].fields[idx as usize].ident.name.as_str()
+                                );
+                                true
+                            },
+                            ty::Tuple(_) => {
+                                replacement_str = format!("{}.{}", replacement_str, idx);
+                                true
+                            },
+                            _ => false,
+                        },
+                        // handled previously
+                        ProjectionKind::Index |
+                            // note: unable to trigger `Subslice` kind in tests
+                            ProjectionKind::Subslice => false,
+                        ProjectionKind::Deref => {
+                            // explicit deref for arrays should be avoided in the suggestion
+                            // i.e.: `|sub| *sub[1..4].len() == 3` is not expected
+                            match cmt.place.ty_before_projection(i).kind() {
+                                // dereferencing an array (i.e.: `|sub| sub[1..4].len() == 3`)
+                                ty::Ref(_, inner, _) => {
+                                    matches!(inner.kind(), ty::Ref(_, innermost, _) if innermost.is_array())
+                                },
+                                _ => false,
+                            }
+                        },
+                    }
+                });
+
+                // handle `ProjectionKind::Deref` if no special case detected
+                if !index_handled && !other_projections_handled {
+                    let last_deref = cmt
+                        .place
+                        .projections
+                        .iter()
+                        .rposition(|proj| proj.kind == ProjectionKind::Deref);
+
+                    if let Some(pos) = last_deref {
+                        let mut projections = cmt.place.projections.clone();
+                        projections.truncate(pos);
+
+                        for item in projections {
+                            if item.kind == ProjectionKind::Deref {
+                                replacement_str = format!("*{}", replacement_str);
+                            }
                         }
                     }
                 }
 
                 self.suggestion_start
                     .push_str(&format!("{}{}", start_snip, replacement_str));
-                self.suggestion_end = end_snip.to_string();
             }
-            self.next_pos = Some(span.hi());
+            self.next_pos = span.hi();
         }
     }