diff --git a/clippy_lints/src/needless_pass_by_value.rs b/clippy_lints/src/needless_pass_by_value.rs index c16be95587b..a995593e8e5 100644 --- a/clippy_lints/src/needless_pass_by_value.rs +++ b/clippy_lints/src/needless_pass_by_value.rs @@ -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 @@ -109,7 +111,12 @@ 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) { + 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::borrow::Borrow`, `serde::Serialize`) @@ -152,6 +159,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue { 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") @@ -162,14 +171,44 @@ 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` 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; // ditto + 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, "_")))]; diff --git a/clippy_lints/src/ptr.rs b/clippy_lints/src/ptr.rs index 03c94cbf3fb..8f42750e19b 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 ec0521ce4f2..07caf07ac8d 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 00000000000..7782fb8bc76 --- /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 59167683595..ac37b0bdda1 100644 --- a/tests/ui/needless_pass_by_value.rs +++ b/tests/ui/needless_pass_by_value.rs @@ -72,4 +72,11 @@ 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 0968b68d82f..f23b0714c59 100644 --- a/tests/ui/needless_pass_by_value.stderr +++ b/tests/ui/needless_pass_by_value.stderr @@ -62,3 +62,45 @@ error: this argument is passed by value, but not consumed in the function body 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(); + | ^^^^^^^^^^^^ +