diff --git a/clippy_lints/src/needless_pass_by_value.rs b/clippy_lints/src/needless_pass_by_value.rs index ccdb3c179..3c1f6ef2c 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; @@ -9,8 +9,10 @@ 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 = { - 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::>(); // 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 != 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 - }); + 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::>(); + + ( + 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::>(), + ) + }), + ) + }; 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, ""))))); + 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 +246,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 +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 = 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")); }} - } + }, - _ => {} + _ => {}, } } } - }} + } } } diff --git a/clippy_lints/src/ptr.rs b/clippy_lints/src/ptr.rs index 03c94cbf3..8f42750e1 100644 --- a/clippy_lints/src/ptr.rs +++ b/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, idx: usize, replacements: &'static [(&'static str, &'static str)]) -> Result)>, ()> { - 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)>, ()> { - 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 { - 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)) diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index cf0aaf6db..240301708 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/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>; diff --git a/clippy_lints/src/utils/ptr.rs b/clippy_lints/src/utils/ptr.rs new file mode 100644 index 000000000..7782fb8bc --- /dev/null +++ b/clippy_lints/src/utils/ptr.rs @@ -0,0 +1,83 @@ +use std::borrow::Cow; +use rustc::hir::*; +use rustc::hir::intravisit::{walk_expr, NestedVisitorMap, Visitor}; +use rustc::lint::LateContext; +use syntax::ast::Name; +use syntax::codemap::Span; +use utils::{get_pat_name, match_var, snippet}; + +pub fn get_spans( + cx: &LateContext, + opt_body_id: Option, + idx: usize, + replacements: &'static [(&'static str, &'static str)], +) -> Option)>> { + if let Some(body) = opt_body_id.map(|id| cx.tcx.hir.body(id)) { + get_binding_name(&body.arguments[idx]) + .map_or_else(|| Some(vec![]), |name| extract_clone_suggestions(cx, name, replacements, body)) + } else { + Some(vec![]) + } +} + +fn extract_clone_suggestions<'a, 'tcx: 'a>( + cx: &LateContext<'a, 'tcx>, + name: Name, + replace: &'static [(&'static str, &'static str)], + body: &'tcx Body, +) -> Option)>> { + let mut visitor = PtrCloneVisitor { + cx, + name, + replace, + spans: vec![], + abort: false, + }; + visitor.visit_body(body); + if visitor.abort { + None + } else { + Some(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 { + get_pat_name(&arg.pat) +} diff --git a/tests/ui/needless_pass_by_value.rs b/tests/ui/needless_pass_by_value.rs index 6218bfb09..ac37b0bdd 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,20 @@ 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 issue_2114(s: String, t: String, u: Vec, v: Vec) { + s.capacity(); + let _ = t.clone(); + u.capacity(); + let _ = v.clone(); +} + fn main() {} diff --git a/tests/ui/needless_pass_by_value.stderr b/tests/ui/needless_pass_by_value.stderr index c08157412..f23b0714c 100644 --- a/tests/ui/needless_pass_by_value.stderr +++ b/tests/ui/needless_pass_by_value.stderr @@ -1,58 +1,106 @@ 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` + +error: this argument is passed by value, but not consumed in the function body + --> $DIR/needless_pass_by_value.rs:75:18 + | +75 | fn issue_2114(s: String, t: String, u: Vec, v: Vec) { + | ^^^^^^ 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:75:29 + | +75 | fn issue_2114(s: String, t: String, u: Vec, v: Vec) { + | ^^^^^^ + | +help: consider changing the type to + | +75 | fn issue_2114(s: String, t: &str, u: Vec, v: Vec) { + | ^^^^ +help: change `t.clone()` to + | +77 | let _ = t.to_string(); + | ^^^^^^^^^^^^^ + +error: this argument is passed by value, but not consumed in the function body + --> $DIR/needless_pass_by_value.rs:75:40 + | +75 | fn issue_2114(s: String, t: String, u: Vec, v: Vec) { + | ^^^^^^^^ help: consider taking a reference instead: `&Vec` + +error: this argument is passed by value, but not consumed in the function body + --> $DIR/needless_pass_by_value.rs:75:53 + | +75 | fn issue_2114(s: String, t: String, u: Vec, v: Vec) { + | ^^^^^^^^ + | +help: consider changing the type to + | +75 | fn issue_2114(s: String, t: String, u: Vec, v: &[i32]) { + | ^^^^^^ +help: change `v.clone()` to + | +79 | let _ = v.to_owned(); + | ^^^^^^^^^^^^ +