]> git.lizzy.rs Git - rust.git/blobdiff - clippy_lints/src/needless_pass_by_value.rs
Merge pull request #2202 from topecongiro/format
[rust.git] / clippy_lints / src / needless_pass_by_value.rs
index e67dca5d00d24cbe2159148c38100b60d2302608..81edd58af57e320ab32521a5daea663f56378d55 100644 (file)
@@ -1,4 +1,5 @@
 use rustc::hir::*;
+use rustc::hir::map::*;
 use rustc::hir::intravisit::FnKind;
 use rustc::lint::*;
 use rustc::ty::{self, RegionKind, TypeFoldable};
 /// sometimes avoid
 /// unnecessary allocations.
 ///
-/// **Known problems:** Hopefully none.
+/// **Known problems:**
+/// * This lint suggests taking an argument by reference,
+/// however sometimes it is better to let users decide the argument type
+/// (by using `Borrow` trait, for example), depending on how the function is used.
 ///
 /// **Example:**
 /// ```rust
 /// fn foo(v: Vec<i32>) {
 ///     assert_eq!(v.len(), 42);
 /// }
+/// // should be
+/// fn foo(v: &[i32]) {
+///     assert_eq!(v.len(), 42);
+/// }
 /// ```
 declare_lint! {
     pub NEEDLESS_PASS_BY_VALUE,
@@ -64,17 +72,26 @@ fn check_fn(
 
         match kind {
             FnKind::ItemFn(.., attrs) => for a in attrs {
-                if_let_chain!{[
-                    a.meta_item_list().is_some(),
-                    let Some(name) = a.name(),
-                    name == "proc_macro_derive",
-                ], {
-                    return;
-                }}
+                if_chain! {
+                    if a.meta_item_list().is_some();
+                    if let Some(name) = a.name();
+                    if name == "proc_macro_derive";
+                    then {
+                        return;
+                    }
+                }
             },
+            FnKind::Method(..) => (),
             _ => return,
         }
 
+        // Exclude non-inherent impls
+        if let Some(NodeItem(item)) = cx.tcx.hir.find(cx.tcx.hir.get_parent_node(node_id)) {
+            if matches!(item.node, ItemImpl(_, _, _, _, Some(_), _, _) | ItemAutoImpl(..)) {
+                return;
+            }
+        }
+
         // Allow `Borrow` or functions to be taken by value
         let borrow_trait = need!(get_trait_def_id(cx, &paths::BORROW_TRAIT));
         let fn_traits = [
@@ -89,13 +106,15 @@ fn check_fn(
 
         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;
+            .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
                 }
-                Some(poly_trait_ref)
-            } else {
-                None
             })
             .collect::<Vec<_>>();
 
@@ -108,7 +127,8 @@ fn check_fn(
         } = {
             let mut ctx = MovedVariablesCtxt::new(cx);
             let region_scope_tree = &cx.tcx.region_scope_tree(fn_def_id);
-            euv::ExprUseVisitor::new(&mut ctx, cx.tcx, cx.param_env, region_scope_tree, cx.tables, None).consume_body(body);
+            euv::ExprUseVisitor::new(&mut ctx, cx.tcx, cx.param_env, region_scope_tree, cx.tables, None)
+                .consume_body(body);
             ctx
         };
 
@@ -126,6 +146,15 @@ fn check_fn(
                 return;
             }
 
+            // Ignore `self`s.
+            if idx == 0 {
+                if let PatKind::Binding(_, _, name, ..) = arg.pat.node {
+                    if name.node.as_str() == "self" {
+                        continue;
+                    }
+                }
+            }
+
             // * 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`)
@@ -148,101 +177,103 @@ fn check_fn(
                 )
             };
 
-            if_let_chain! {[
-                !is_self(arg),
-                !ty.is_mutable_pointer(),
-                !is_copy(cx, ty),
-                !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),
-            ], {
-                if mode == BindingAnnotation::Mutable || mode == BindingAnnotation::RefMut {
-                    continue;
-                }
+            if_chain! {
+                if !is_self(arg);
+                if !ty.is_mutable_pointer();
+                if !is_copy(cx, ty);
+                if !fn_traits.iter().any(|&t| implements_trait(cx, ty, t, &[]));
+                if !implements_borrow_trait;
+                if !all_borrowable_trait;
+
+                if let PatKind::Binding(mode, canonical_id, ..) = arg.pat.node;
+                if !moved_vars.contains(&canonical_id);
+                then {
+                    if mode == BindingAnnotation::Mutable || mode == BindingAnnotation::RefMut {
+                        continue;
+                    }
 
-                // 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")
-                            .and_then(|ps| ps.parameters.as_ref())
-                            .map(|params| &params.types[0]),
-                    ], {
-                        let slice_ty = format!("&[{}]", snippet(cx, elem_ty.span, "_"));
-                        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()
-                            );
+                    // Dereference suggestion
+                    let sugg = |db: &mut DiagnosticBuilder| {
+                        let deref_span = spans_need_deref.get(&canonical_id);
+                        if_chain! {
+                            if match_type(cx, ty, &paths::VEC);
+                            if let Some(clone_spans) =
+                                get_spans(cx, Some(body.id()), idx, &[("clone", ".to_owned()")]);
+                            if let TyPath(QPath::Resolved(_, ref path)) = input.node;
+                            if let Some(elem_ty) = path.segments.iter()
+                                .find(|seg| seg.name == "Vec")
+                                .and_then(|ps| ps.parameters.as_ref())
+                                .map(|params| &params.types[0]);
+                            then {
+                                let slice_ty = format!("&[{}]", snippet(cx, elem_ty.span, "_"));
+                                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;
+                            }
                         }
 
-                        // cannot be destructured, no need for `*` suggestion
-                        assert!(deref_span.is_none());
-                        return;
-                    }}
-
-                    if match_type(cx, ty, &paths::STRING) {
-                        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(),
-                                );
+                        if match_type(cx, ty, &paths::STRING) {
+                            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;
                             }
-
-                            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.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,
-                );
-            }}
+                        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.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,
+                    );
+                }
+            }
         }
     }
 }
@@ -299,18 +330,19 @@ fn non_moving_pat(&mut self, matched_pat: &Pat, cmt: mc::cmt<'tcx>) {
 
                         map::Node::NodeStmt(s) => {
                             // `let <pat> = x;`
-                            if_let_chain! {[
-                                let StmtDecl(ref decl, _) = s.node,
-                                let DeclLocal(ref local) = decl.node,
-                            ], {
-                                self.spans_need_deref
-                                    .entry(vid)
-                                    .or_insert_with(HashSet::new)
-                                    .insert(local.init
-                                        .as_ref()
-                                        .map(|e| e.span)
-                                        .expect("`let` stmt without init aren't caught by match_pat"));
-                            }}
+                            if_chain! {
+                                if let StmtDecl(ref decl, _) = s.node;
+                                if let DeclLocal(ref local) = decl.node;
+                                then {
+                                    self.spans_need_deref
+                                        .entry(vid)
+                                        .or_insert_with(HashSet::new)
+                                        .insert(local.init
+                                            .as_ref()
+                                            .map(|e| e.span)
+                                            .expect("`let` stmt without init aren't caught by match_pat"));
+                                }
+                            }
                         },
 
                         _ => {},