diff --git a/clippy_lints/src/needless_pass_by_value.rs b/clippy_lints/src/needless_pass_by_value.rs index ccdb3c179cc1..c16be95587b6 100644 --- a/clippy_lints/src/needless_pass_by_value.rs +++ b/clippy_lints/src/needless_pass_by_value.rs @@ -1,7 +1,7 @@ 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; @@ -73,18 +73,25 @@ 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)); let borrow_trait = need!(get_trait_def_id(cx, &paths::BORROW_TRAIT)); + let fn_trait = need!(cx.tcx.lang_items().fn_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 = { - 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()) + .collect::>(); + let preds = preds + .iter() + .filter_map(|pred| if let ty::Predicate::Trait(ref poly_trait_ref) = *pred { + Some(poly_trait_ref.skip_binder()) + } else { + None + }) + .filter(|t| t.def_id() != sized_trait && !t.has_escaping_regions()) + .collect::>(); // Collect moved variables and spans which will need dereferencings from the // function body. @@ -103,40 +110,44 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue { 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 != ty) specifically. - // This is needed due to the `Borrow 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 - }); + // * Exclude a type that is specifically bounded by `Borrow`. + // * Exclude a type whose reference also fulfills its bound. + // (e.g. `std::borrow::Borrow`, `serde::Serialize`) + let (implements_borrow_trait, all_borrowable_trait) = { + let preds = preds + .iter() + .filter(|t| t.self_ty() == ty) + .collect::>(); + + ( + 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.input_types().skip(1).collect::>(), + ) + }), + ) + }; 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, &[]), !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! {[ @@ -152,33 +163,37 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue { "consider changing the type to", slice_ty); assert!(deref_span.is_none()); - return; // `Vec` and `String` cannot be destructured - no need for `*` suggestion + return; // `Vec` cannot be destructured - no need for `*` suggestion }} if match_type(cx, ty, &paths::STRING) { - db.span_suggestion(input.span, - "consider changing the type to", - "&str".to_string()); + db.span_suggestion(input.span, "consider changing the type to", "&str".to_string()); assert!(deref_span.is_none()); - return; + return; // ditto } 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.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); + 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 +203,7 @@ struct MovedVariablesCtxt<'a, 'tcx: 'a> { cx: &'a LateContext<'a, 'tcx>, moved_vars: HashSet, /// Spans which need to be prefixed with `*` for dereferencing the - /// suggested additional - /// reference. + /// suggested additional reference. spans_need_deref: HashMap>, } @@ -213,9 +227,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 +247,7 @@ impl<'a, 'tcx> MovedVariablesCtxt<'a, 'tcx> { .or_insert_with(HashSet::new) .insert(c.span); } - } + }, map::Node::NodeStmt(s) => { // `let = x;` @@ -251,13 +263,13 @@ impl<'a, 'tcx> MovedVariablesCtxt<'a, 'tcx> { .map(|e| e.span) .expect("`let` stmt without init aren't caught by match_pat")); }} - } + }, - _ => {} + _ => {}, } } } - }} + } } } diff --git a/tests/ui/needless_pass_by_value.rs b/tests/ui/needless_pass_by_value.rs index 6218bfb09206..591676835953 100644 --- a/tests/ui/needless_pass_by_value.rs +++ b/tests/ui/needless_pass_by_value.rs @@ -4,6 +4,9 @@ #![warn(needless_pass_by_value)] #![allow(dead_code, single_match, if_let_redundant_pattern_matching, many_single_char_names)] +use std::borrow::Borrow; +use std::convert::AsRef; + // `v` should be warned // `w`, `x` and `y` are allowed (moved or mutated) fn foo(v: Vec, w: Vec, mut x: Vec, y: Vec) -> Vec { @@ -25,10 +28,11 @@ fn bar(x: String, y: Wrapper) { assert_eq!(y.0.len(), 42); } -// U implements `Borrow`, but should be warned correctly -fn test_borrow_trait, U>(t: T, u: U) { +// V implements `Borrow`, but should be warned correctly +fn test_borrow_trait, U: AsRef, V>(t: T, u: U, v: V) { println!("{}", t.borrow()); - consume(&u); + println!("{}", u.as_ref()); + consume(&v); } // ok @@ -59,4 +63,13 @@ fn test_destructure(x: Wrapper, y: Wrapper, z: Wrapper) { println!("{}", t); } +trait Foo {} + +// `S: Serialize` can be passed by value +trait Serialize {} +impl<'a, T> Serialize for &'a T where T: Serialize {} +impl Serialize for i32 {} + +fn test_blanket_ref(_foo: T, _serializable: S) {} + fn main() {} diff --git a/tests/ui/needless_pass_by_value.stderr b/tests/ui/needless_pass_by_value.stderr index c081574127af..0968b68d82f0 100644 --- a/tests/ui/needless_pass_by_value.stderr +++ b/tests/ui/needless_pass_by_value.stderr @@ -1,58 +1,64 @@ error: this argument is passed by value, but not consumed in the function body - --> $DIR/needless_pass_by_value.rs:9:23 - | -9 | fn foo(v: Vec, w: Vec, mut x: Vec, y: Vec) -> Vec { - | ^^^^^^ help: consider changing the type to: `&[T]` - | - = note: `-D needless-pass-by-value` implied by `-D warnings` + --> $DIR/needless_pass_by_value.rs:12:23 + | +12 | fn foo(v: Vec, w: Vec, mut x: Vec, y: Vec) -> Vec { + | ^^^^^^ help: consider changing the type to: `&[T]` + | + = note: `-D needless-pass-by-value` implied by `-D warnings` error: this argument is passed by value, but not consumed in the function body - --> $DIR/needless_pass_by_value.rs:23:11 + --> $DIR/needless_pass_by_value.rs:26:11 | -23 | fn bar(x: String, y: Wrapper) { +26 | fn bar(x: String, y: Wrapper) { | ^^^^^^ 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:23:22 + --> $DIR/needless_pass_by_value.rs:26:22 | -23 | fn bar(x: String, y: Wrapper) { +26 | fn bar(x: String, y: Wrapper) { | ^^^^^^^ help: consider taking a reference instead: `&Wrapper` error: this argument is passed by value, but not consumed in the function body - --> $DIR/needless_pass_by_value.rs:29:63 + --> $DIR/needless_pass_by_value.rs:32:71 | -29 | fn test_borrow_trait, U>(t: T, u: U) { - | ^ help: consider taking a reference instead: `&U` +32 | fn test_borrow_trait, U: AsRef, V>(t: T, u: U, v: V) { + | ^ help: consider taking a reference instead: `&V` error: this argument is passed by value, but not consumed in the function body - --> $DIR/needless_pass_by_value.rs:40:18 + --> $DIR/needless_pass_by_value.rs:44:18 | -40 | fn test_match(x: Option>, y: Option>) { +44 | fn test_match(x: Option>, y: Option>) { | ^^^^^^^^^^^^^^^^^^^^^^ | help: consider taking a reference instead | -40 | fn test_match(x: &Option>, y: Option>) { -41 | match *x { +44 | fn test_match(x: &Option>, y: Option>) { +45 | match *x { | error: this argument is passed by value, but not consumed in the function body - --> $DIR/needless_pass_by_value.rs:53:24 + --> $DIR/needless_pass_by_value.rs:57:24 | -53 | fn test_destructure(x: Wrapper, y: Wrapper, z: Wrapper) { +57 | fn test_destructure(x: Wrapper, y: Wrapper, z: Wrapper) { | ^^^^^^^ help: consider taking a reference instead: `&Wrapper` error: this argument is passed by value, but not consumed in the function body - --> $DIR/needless_pass_by_value.rs:53:36 + --> $DIR/needless_pass_by_value.rs:57:36 | -53 | fn test_destructure(x: Wrapper, y: Wrapper, z: Wrapper) { +57 | fn test_destructure(x: Wrapper, y: Wrapper, z: Wrapper) { | ^^^^^^^ | help: consider taking a reference instead | -53 | fn test_destructure(x: Wrapper, y: &Wrapper, z: Wrapper) { -54 | let Wrapper(s) = z; // moved -55 | let Wrapper(ref t) = *y; // not moved -56 | let Wrapper(_) = *y; // still not moved +57 | fn test_destructure(x: Wrapper, y: &Wrapper, z: Wrapper) { +58 | let Wrapper(s) = z; // moved +59 | let Wrapper(ref t) = *y; // not moved +60 | let Wrapper(_) = *y; // still not moved | +error: this argument is passed by value, but not consumed in the function body + --> $DIR/needless_pass_by_value.rs:73:49 + | +73 | fn test_blanket_ref(_foo: T, _serializable: S) {} + | ^ help: consider taking a reference instead: `&T` +