Merge pull request #2117 from sinkuu/improve_take_by_value

Improve needless_pass_by_value

Author: oli-obk

clippy_lints/src/needless_pass_by_value.rs

@@ -1,16 +1,18 @@
 use rustc::hir::*;
 use rustc::hir::intravisit::FnKind;
 use rustc::lint::*;
-use rustc::ty::{self, TypeFoldable};
+use rustc::ty::{self, RegionKind, TypeFoldable};
 use rustc::traits;
 use rustc::middle::expr_use_visitor as euv;
 use rustc::middle::mem_categorization as mc;
 use syntax::ast::NodeId;
 use syntax_pos::Span;
 use syntax::errors::DiagnosticBuilder;
 use utils::{get_trait_def_id, implements_trait, in_macro, is_copy, is_self, match_type, multispan_sugg, paths,
-            snippet, span_lint_and_then};
+            snippet, snippet_opt, span_lint_and_then};
+use utils::ptr::get_spans;
 use std::collections::{HashMap, HashSet};
+use std::borrow::Cow;
 
 /// **What it does:** Checks for functions taking arguments by value, but not
 /// consuming them in its
@@ -73,18 +75,29 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue {
             _ => return,
         }
 
-        // Allows these to be passed by value.
-        let fn_trait = need!(cx.tcx.lang_items().fn_trait());
-        let asref_trait = need!(get_trait_def_id(cx, &paths::ASREF_TRAIT));
+        // Allow `Borrow` or functions to be taken by value
         let borrow_trait = need!(get_trait_def_id(cx, &paths::BORROW_TRAIT));
+        let fn_traits = [
+            need!(cx.tcx.lang_items().fn_trait()),
+            need!(cx.tcx.lang_items().fn_once_trait()),
+            need!(cx.tcx.lang_items().fn_mut_trait()),
+        ];
+
+        let sized_trait = need!(cx.tcx.lang_items().sized_trait());
 
         let fn_def_id = cx.tcx.hir.local_def_id(node_id);
 
-        let preds: Vec<ty::Predicate> = {
-            traits::elaborate_predicates(cx.tcx, cx.param_env.caller_bounds.to_vec())
-                .filter(|p| !p.is_global())
-                .collect()
-        };
+        let preds = traits::elaborate_predicates(cx.tcx, cx.param_env.caller_bounds.to_vec())
+            .filter(|p| !p.is_global())
+            .filter_map(|pred| if let ty::Predicate::Trait(poly_trait_ref) = pred {
+                if poly_trait_ref.def_id() == sized_trait || poly_trait_ref.skip_binder().has_escaping_regions() {
+                    return None;
+                }
+                Some(poly_trait_ref)
+            } else {
+                None
+            })
+            .collect::<Vec<_>>();
 
         // Collect moved variables and spans which will need dereferencings from the
         // function body.
@@ -102,45 +115,56 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue {
         let fn_sig = cx.tcx.fn_sig(fn_def_id);
         let fn_sig = cx.tcx.erase_late_bound_regions(&fn_sig);
 
-        for ((input, &ty), arg) in decl.inputs.iter().zip(fn_sig.inputs()).zip(&body.arguments) {
-            // Determines whether `ty` implements `Borrow<U>` (U != ty) specifically.
-            // This is needed due to the `Borrow<T> for T` blanket impl.
-            let implements_borrow_trait = preds
-                .iter()
-                .filter_map(|pred| if let ty::Predicate::Trait(ref poly_trait_ref) = *pred {
-                    Some(poly_trait_ref.skip_binder())
-                } else {
-                    None
-                })
-                .filter(|tpred| tpred.def_id() == borrow_trait && tpred.self_ty() == ty)
-                .any(|tpred| {
-                    tpred
-                        .input_types()
-                        .nth(1)
-                        .expect("Borrow trait must have an parameter") != ty
-                });
+        for (idx, ((input, &ty), arg)) in decl.inputs
+            .iter()
+            .zip(fn_sig.inputs())
+            .zip(&body.arguments)
+            .enumerate()
+        {
+            // * Exclude a type that is specifically bounded by `Borrow`.
+            // * Exclude a type whose reference also fulfills its bound.
+            //   (e.g. `std::convert::AsRef`, `serde::Serialize`)
+            let (implements_borrow_trait, all_borrowable_trait) = {
+                let preds = preds
+                    .iter()
+                    .filter(|t| t.skip_binder().self_ty() == ty)
+                    .collect::<Vec<_>>();
+
+                (
+                    preds.iter().any(|t| t.def_id() == borrow_trait),
+                    !preds.is_empty() && preds.iter().all(|t| {
+                        implements_trait(
+                            cx,
+                            cx.tcx.mk_imm_ref(&RegionKind::ReErased, ty),
+                            t.def_id(),
+                            &t.skip_binder().input_types().skip(1).collect::<Vec<_>>(),
+                        )
+                    }),
+                )
+            };
 
             if_let_chain! {[
                 !is_self(arg),
                 !ty.is_mutable_pointer(),
                 !is_copy(cx, ty),
-                !implements_trait(cx, ty, fn_trait, &[]),
-                !implements_trait(cx, ty, asref_trait, &[]),
+                !fn_traits.iter().any(|&t| implements_trait(cx, ty, t, &[])),
                 !implements_borrow_trait,
+                !all_borrowable_trait,
 
                 let PatKind::Binding(mode, canonical_id, ..) = arg.pat.node,
                 !moved_vars.contains(&canonical_id),
             ], {
-                // Note: `toplevel_ref_arg` warns if `BindByRef`
                 if mode == BindingAnnotation::Mutable || mode == BindingAnnotation::RefMut {
                     continue;
                 }
 
-                // Suggestion logic
+                // Dereference suggestion
                 let sugg = |db: &mut DiagnosticBuilder| {
                     let deref_span = spans_need_deref.get(&canonical_id);
                     if_let_chain! {[
                         match_type(cx, ty, &paths::VEC),
+                        let Some(clone_spans) =
+                            get_spans(cx, Some(body.id()), idx, &[("clone", ".to_owned()")]),
                         let TyPath(QPath::Resolved(_, ref path)) = input.node,
                         let Some(elem_ty) = path.segments.iter()
                             .find(|seg| seg.name == "Vec")
@@ -151,34 +175,68 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue {
                         db.span_suggestion(input.span,
                                         "consider changing the type to",
                                         slice_ty);
+
+                        for (span, suggestion) in clone_spans {
+                            db.span_suggestion(
+                                span,
+                                &snippet_opt(cx, span)
+                                    .map_or(
+                                        "change the call to".into(),
+                                        |x| Cow::from(format!("change `{}` to", x)),
+                                    ),
+                                suggestion.into()
+                            );
+                        }
+
+                        // cannot be destructured, no need for `*` suggestion
                         assert!(deref_span.is_none());
-                        return; // `Vec` and `String` cannot be destructured - no need for `*` suggestion
+                        return;
                     }}
 
                     if match_type(cx, ty, &paths::STRING) {
-                        db.span_suggestion(input.span,
-                                           "consider changing the type to",
-                                           "&str".to_string());
-                        assert!(deref_span.is_none());
-                        return;
+                        if let Some(clone_spans) =
+                            get_spans(cx, Some(body.id()), idx, &[("clone", ".to_string()"), ("as_str", "")]) {
+                            db.span_suggestion(input.span, "consider changing the type to", "&str".to_string());
+
+                            for (span, suggestion) in clone_spans {
+                                db.span_suggestion(
+                                    span,
+                                    &snippet_opt(cx, span)
+                                        .map_or(
+                                            "change the call to".into(),
+                                            |x| Cow::from(format!("change `{}` to", x))
+                                        ),
+                                    suggestion.into(),
+                                );
+                            }
+
+                            assert!(deref_span.is_none());
+                            return;
+                        }
                     }
 
                     let mut spans = vec![(input.span, format!("&{}", snippet(cx, input.span, "_")))];
 
                     // Suggests adding `*` to dereference the added reference.
                     if let Some(deref_span) = deref_span {
-                        spans.extend(deref_span.iter().cloned()
-                                     .map(|span| (span, format!("*{}", snippet(cx, span, "<expr>")))));
+                        spans.extend(
+                            deref_span
+                                .iter()
+                                .cloned()
+                                .map(|span| (span, format!("*{}", snippet(cx, span, "<expr>")))),
+                        );
                         spans.sort_by_key(|&(span, _)| span);
                     }
                     multispan_sugg(db, "consider taking a reference instead".to_string(), spans);
                 };
 
-                span_lint_and_then(cx,
-                          NEEDLESS_PASS_BY_VALUE,
-                          input.span,
-                          "this argument is passed by value, but not consumed in the function body",
-                          sugg);
+                span_lint_and_then(
+                    cx,
+                    NEEDLESS_PASS_BY_VALUE,
+                    input.span,
+                    "this argument is passed by value, but not consumed in the function body",
+                    sugg,
+                );
             }}
         }
     }
@@ -188,8 +246,7 @@ struct MovedVariablesCtxt<'a, 'tcx: 'a> {
     cx: &'a LateContext<'a, 'tcx>,
     moved_vars: HashSet<NodeId>,
     /// Spans which need to be prefixed with `*` for dereferencing the
-    /// suggested additional
-    /// reference.
+    /// suggested additional reference.
     spans_need_deref: HashMap<NodeId, HashSet<Span>>,
 }
 
@@ -213,9 +270,7 @@ impl<'a, 'tcx> MovedVariablesCtxt<'a, 'tcx> {
     fn non_moving_pat(&mut self, matched_pat: &Pat, cmt: mc::cmt<'tcx>) {
         let cmt = unwrap_downcast_or_interior(cmt);
 
-        if_let_chain! {[
-            let mc::Categorization::Local(vid) = cmt.cat,
-        ], {
+        if let mc::Categorization::Local(vid) = cmt.cat {
             let mut id = matched_pat.id;
             loop {
                 let parent = self.cx.tcx.hir.get_parent_node(id);
@@ -235,7 +290,7 @@ impl<'a, 'tcx> MovedVariablesCtxt<'a, 'tcx> {
                                     .or_insert_with(HashSet::new)
                                     .insert(c.span);
                             }
-                        }
+                        },
 
                         map::Node::NodeStmt(s) => {
                             // `let <pat> = x;`
@@ -251,13 +306,13 @@ impl<'a, 'tcx> MovedVariablesCtxt<'a, 'tcx> {
                                         .map(|e| e.span)
                                         .expect("`let` stmt without init aren't caught by match_pat"));
                             }}
-                        }
+                        },
 
-                        _ => {}
+                        _ => {},
                     }
                 }
             }
-        }}
+        }
     }
 }
 

clippy_lints/src/ptr.rs

@@ -2,16 +2,15 @@
 
 use std::borrow::Cow;
 use rustc::hir::*;
-use rustc::hir::intravisit::{walk_expr, NestedVisitorMap, Visitor};
 use rustc::hir::map::NodeItem;
 use rustc::lint::*;
 use rustc::ty;
-use syntax::ast::{Name, NodeId};
+use syntax::ast::NodeId;
 use syntax::codemap::Span;
 use syntax_pos::MultiSpan;
-use utils::{get_pat_name, match_qpath, match_type, match_var, paths,
-            snippet, snippet_opt, span_lint, span_lint_and_then,
+use utils::{match_qpath, match_type, paths, snippet_opt, span_lint, span_lint_and_then,
             walk_ptrs_hir_ty};
+use utils::ptr::get_spans;
 
 /// **What it does:** This lint checks for function arguments of type `&String`
 /// or `&Vec` unless the references are mutable. It will also suggest you
@@ -164,7 +163,7 @@ fn check_fn(cx: &LateContext, decl: &FnDecl, fn_id: NodeId, opt_body_id: Option<
                 ], {
                     ty_snippet = snippet_opt(cx, parameters.types[0].span);
                 });
-                if let Ok(spans) = get_spans(cx, opt_body_id, idx, &[("clone", ".to_owned()")]) {
+                if let Some(spans) = get_spans(cx, opt_body_id, idx, &[("clone", ".to_owned()")]) {
                     span_lint_and_then(
                         cx,
                         PTR_ARG,
@@ -187,7 +186,7 @@ fn check_fn(cx: &LateContext, decl: &FnDecl, fn_id: NodeId, opt_body_id: Option<
                     );
                 }
             } else if match_type(cx, ty, &paths::STRING) {
-                if let Ok(spans) = get_spans(cx, opt_body_id, idx, &[("clone", ".to_string()"), ("as_str", "")]) {
+                if let Some(spans) = get_spans(cx, opt_body_id, idx, &[("clone", ".to_string()"), ("as_str", "")]) {
                     span_lint_and_then(
                         cx,
                         PTR_ARG,
@@ -234,66 +233,6 @@ fn check_fn(cx: &LateContext, decl: &FnDecl, fn_id: NodeId, opt_body_id: Option<
     }
 }
 
-fn get_spans(cx: &LateContext, opt_body_id: Option<BodyId>, idx: usize, replacements: &'static [(&'static str, &'static str)]) -> Result<Vec<(Span, Cow<'static, str>)>, ()> {
-    if let Some(body) = opt_body_id.map(|id| cx.tcx.hir.body(id)) {
-        get_binding_name(&body.arguments[idx]).map_or_else(|| Ok(vec![]),
-                                                |name| extract_clone_suggestions(cx, name, replacements, body))
-    } else {
-        Ok(vec![])
-    }
-}
-
-fn extract_clone_suggestions<'a, 'tcx: 'a>(cx: &LateContext<'a, 'tcx>, name: Name, replace: &'static [(&'static str, &'static str)], body: &'tcx Body) -> Result<Vec<(Span, Cow<'static, str>)>, ()> {
-    let mut visitor = PtrCloneVisitor {
-        cx,
-        name,
-        replace,
-        spans: vec![],
-        abort: false,
-    };
-    visitor.visit_body(body);
-    if visitor.abort { Err(()) } else { Ok(visitor.spans) }
-}
-
-struct PtrCloneVisitor<'a, 'tcx: 'a> {
-    cx: &'a LateContext<'a, 'tcx>,
-    name: Name,
-    replace: &'static [(&'static str, &'static str)],
-    spans: Vec<(Span, Cow<'static, str>)>,
-    abort: bool,
-}
-
-impl<'a, 'tcx: 'a> Visitor<'tcx> for PtrCloneVisitor<'a, 'tcx> {
-    fn visit_expr(&mut self, expr: &'tcx Expr) {
-        if self.abort { return; }
-        if let ExprMethodCall(ref seg, _, ref args) = expr.node {
-            if args.len() == 1 && match_var(&args[0], self.name) {
-                if seg.name == "capacity" {
-                    self.abort = true;
-                    return;
-                }
-                for &(fn_name, suffix) in self.replace {
-                    if seg.name == fn_name {
-                        self.spans.push((expr.span, snippet(self.cx, args[0].span, "_") + suffix));
-                        return;
-                    }
-                }
-            }
-            return;
-        }
-        walk_expr(self, expr);
-    }
-
-    fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> {
-        NestedVisitorMap::None
-    }
-
-}
-
-fn get_binding_name(arg: &Arg) -> Option<Name> {
-    get_pat_name(&arg.pat)
-}
-
 fn get_rptr_lm(ty: &Ty) -> Option<(&Lifetime, Mutability, Span)> {
     if let Ty_::TyRptr(ref lt, ref m) = ty.node {
         Some((lt, m.mutbl, ty.span))

clippy_lints/src/utils/mod.rs

@@ -32,6 +32,7 @@ pub mod sugg;
 pub mod inspector;
 pub mod internal_lints;
 pub mod author;
+pub mod ptr;
 pub use self::hir_utils::{SpanlessEq, SpanlessHash};
 
 pub type MethodArgs = HirVec<P<Expr>>;