diff --git a/clippy_lints/src/needless_pass_by_value.rs b/clippy_lints/src/needless_pass_by_value.rs index e00938fb3ef1..ac965a59bd51 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 @@ use std::borrow::Cow; /// 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, @@ -73,9 +81,17 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue { } } }, + 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(_), _, _) | ItemDefaultImpl(..)) { + 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 = [ @@ -109,7 +125,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue { } = { 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 }; @@ -127,6 +144,15 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue { 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`) @@ -163,7 +189,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue { if mode == BindingAnnotation::Mutable || mode == BindingAnnotation::RefMut { continue; } - + // Dereference suggestion let sugg = |db: &mut DiagnosticBuilder| { let deref_span = spans_need_deref.get(&canonical_id); @@ -181,7 +207,7 @@ 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, @@ -193,18 +219,18 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue { suggestion.into() ); } - + // 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, @@ -216,14 +242,14 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue { 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( @@ -236,7 +262,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue { } multispan_sugg(db, "consider taking a reference instead".to_string(), spans); }; - + span_lint_and_then( cx, NEEDLESS_PASS_BY_VALUE, diff --git a/tests/ui/needless_pass_by_value.rs b/tests/ui/needless_pass_by_value.rs index ac37b0bdda16..307acb45bcb7 100644 --- a/tests/ui/needless_pass_by_value.rs +++ b/tests/ui/needless_pass_by_value.rs @@ -65,7 +65,7 @@ fn test_destructure(x: Wrapper, y: Wrapper, z: Wrapper) { trait Foo {} -// `S: Serialize` can be passed by value +// `S: Serialize` is allowed to be passed by value, since a caller can pass `&S` instead trait Serialize {} impl<'a, T> Serialize for &'a T where T: Serialize {} impl Serialize for i32 {} @@ -79,4 +79,27 @@ fn issue_2114(s: String, t: String, u: Vec, v: Vec) { let _ = v.clone(); } +struct S(T, U); + +impl S { + fn foo( + self, // taking `self` by value is always allowed + s: String, + t: String, + ) -> usize { + s.len() + t.capacity() + } + + fn bar( + _t: T, // Ok, since `&T: Serialize` too + ) { + } + + fn baz( + &self, + _u: U, + ) { + } +} + fn main() {} diff --git a/tests/ui/needless_pass_by_value.stderr b/tests/ui/needless_pass_by_value.stderr index f23b0714c599..3e4d0c7e44f6 100644 --- a/tests/ui/needless_pass_by_value.stderr +++ b/tests/ui/needless_pass_by_value.stderr @@ -104,3 +104,21 @@ help: change `v.clone()` to 79 | let _ = v.to_owned(); | ^^^^^^^^^^^^ +error: this argument is passed by value, but not consumed in the function body + --> $DIR/needless_pass_by_value.rs:87:12 + | +87 | s: String, + | ^^^^^^ help: consider changing the type to: `&str` + +error: this argument is passed by value, but not consumed in the function body + --> $DIR/needless_pass_by_value.rs:88:12 + | +88 | t: String, + | ^^^^^^ help: consider taking a reference instead: `&String` + +error: this argument is passed by value, but not consumed in the function body + --> $DIR/needless_pass_by_value.rs:100:13 + | +100 | _u: U, + | ^ help: consider taking a reference instead: `&U` +