X-Git-Url: https://git.lizzy.rs/?a=blobdiff_plain;f=clippy_lints%2Fsrc%2Fneedless_pass_by_value.rs;h=81edd58af57e320ab32521a5daea663f56378d55;hb=ed589761e62735ebb803510e01bfd8b278527fb9;hp=e67dca5d00d24cbe2159148c38100b60d2302608;hpb=0e489f322155113fe911352895a8fee68a9751d7;p=rust.git diff --git a/clippy_lints/src/needless_pass_by_value.rs b/clippy_lints/src/needless_pass_by_value.rs index e67dca5d00d..81edd58af57 100644 --- a/clippy_lints/src/needless_pass_by_value.rs +++ b/clippy_lints/src/needless_pass_by_value.rs @@ -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}; @@ -22,13 +23,20 @@ /// 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) { /// 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::>(); @@ -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| ¶ms.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| ¶ms.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, "")))), - ); - 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, "")))), + ); + 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 = 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")); + } + } }, _ => {},