Duplicate ptr_arg's suggestion logic

sinkuu · sinkuu · commit 2be624517994 · 2017-10-08T17:51:44.000+09:00
diff --git 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
@@ -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<BodyId>, idx: usize, replacements: &'static [(&'static str, &'static str)]) -> Result<Vec<(Span, Cow<'static, str>)>, ()> {
-    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<Vec<(Span, Cow<'static, str>)>, ()> {
-    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<Name> {
-    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
@@ -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<P<Expr>>;
diff --git a/clippy_lints/src/utils/ptr.rs 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<BodyId>,
+    idx: usize,
+    replacements: &'static [(&'static str, &'static str)],
+) -> Option<Vec<(Span, Cow<'static, str>)>> {
+    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<Vec<(Span, Cow<'static, str>)>> {
+    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<Name> {
+    get_pat_name(&arg.pat)
+}
diff --git 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<T: Foo, S: Serialize>(_foo: T, _serializable: S) {}
 
+fn issue_2114(s: String, t: String, u: Vec<i32>, v: Vec<i32>) {
+    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
@@ -62,3 +62,45 @@ error: this argument is passed by value, but not consumed in the function body
 73 | fn test_blanket_ref<T: Foo, S: Serialize>(_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<i32>, v: Vec<i32>) {
+   |                  ^^^^^^ 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<i32>, v: Vec<i32>) {
+   |                             ^^^^^^
+   |
+help: consider changing the type to
+   |
+75 | fn issue_2114(s: String, t: &str, u: Vec<i32>, v: Vec<i32>) {
+   |                             ^^^^
+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<i32>, v: Vec<i32>) {
+   |                                        ^^^^^^^^ help: consider taking a reference instead: `&Vec<i32>`
+
+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<i32>, v: Vec<i32>) {
+   |                                                     ^^^^^^^^
+   |
+help: consider changing the type to
+   |
+75 | fn issue_2114(s: String, t: String, u: Vec<i32>, v: &[i32]) {
+   |                                                     ^^^^^^
+help: change `v.clone()` to
+   |
+79 |     let _ = v.to_owned();
+   |             ^^^^^^^^^^^^
+
