Merge pull request #2058 from rust-lang-nursery/ptr_arg-vs-clone

oli-obk · web-flow · commit 2bb8efdb4dc9 · 2017-09-17T15:37:21.000+02:00
add suggestions for .clone() in ptr_arg fns
diff --git a/clippy_lints/src/bytecount.rs b/clippy_lints/src/bytecount.rs
@@ -2,7 +2,8 @@ use rustc::hir::*;
 use rustc::lint::*;
 use rustc::ty;
 use syntax::ast::{Name, UintTy};
-use utils::{contains_name, match_type, paths, single_segment_path, snippet, span_lint_and_sugg, walk_ptrs_ty};
+use utils::{contains_name, get_pat_name, match_type, paths, single_segment_path,
+            snippet, span_lint_and_sugg, walk_ptrs_ty};
 
 /// **What it does:** Checks for naive byte counts
 ///
@@ -93,15 +94,6 @@ fn check_arg(name: Name, arg: Name, needle: &Expr) -> bool {
     name == arg && !contains_name(name, needle)
 }
 
-fn get_pat_name(pat: &Pat) -> Option<Name> {
-    match pat.node {
-        PatKind::Binding(_, _, ref spname, _) => Some(spname.node),
-        PatKind::Path(ref qpath) => single_segment_path(qpath).map(|ps| ps.name),
-        PatKind::Box(ref p) | PatKind::Ref(ref p, _) => get_pat_name(&*p),
-        _ => None,
-    }
-}
-
 fn get_path_name(expr: &Expr) -> Option<Name> {
     match expr.node {
         ExprBox(ref e) | ExprAddrOf(_, ref e) | ExprUnary(UnOp::UnDeref, ref e) => get_path_name(e),
diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs
@@ -16,7 +16,7 @@ use utils::sugg;
 use utils::const_to_u64;
 
 use utils::{get_enclosing_block, get_parent_expr, higher, in_external_macro, is_integer_literal, is_refutable,
-            last_path_segment, match_trait_method, match_type, multispan_sugg, snippet, snippet_opt,
+            last_path_segment, match_trait_method, match_type, match_var, multispan_sugg, snippet, snippet_opt,
             span_help_and_lint, span_lint, span_lint_and_sugg, span_lint_and_then};
 use utils::paths;
 
@@ -1310,15 +1310,6 @@ fn pat_is_wild<'tcx>(pat: &'tcx PatKind, body: &'tcx Expr) -> bool {
     }
 }
 
-fn match_var(expr: &Expr, var: Name) -> bool {
-    if let ExprPath(QPath::Resolved(None, ref path)) = expr.node {
-        if path.segments.len() == 1 && path.segments[0].name == var {
-            return true;
-        }
-    }
-    false
-}
-
 struct UsedVisitor {
     var: ast::Name, // var to look for
     used: bool, // has the var been used otherwise?
diff --git a/clippy_lints/src/ptr.rs b/clippy_lints/src/ptr.rs
@@ -1,26 +1,42 @@
 //! Checks for usage of  `&Vec[_]` and `&String`.
 
 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::NodeId;
+use syntax::ast::{Name, NodeId};
 use syntax::codemap::Span;
 use syntax_pos::MultiSpan;
-use utils::{match_qpath, match_type, paths, snippet_opt, span_lint, span_lint_and_then,
-            span_lint_and_sugg, walk_ptrs_hir_ty};
+use utils::{get_pat_name, match_qpath, match_type, match_var, paths,
+            snippet, snippet_opt, span_lint, span_lint_and_then,
+            walk_ptrs_hir_ty};
 
 /// **What it does:** This lint checks for function arguments of type `&String`
-/// or `&Vec` unless
-/// the references are mutable.
+/// or `&Vec` unless the references are mutable. It will also suggest you
+/// replace `.clone()` calls with the appropriate `.to_owned()`/`to_string()`
+/// calls.
 ///
 /// **Why is this bad?** Requiring the argument to be of the specific size
-/// makes the function less
-/// useful for no benefit; slices in the form of `&[T]` or `&str` usually
-/// suffice and can be
-/// obtained from other types, too.
+/// makes the function less useful for no benefit; slices in the form of `&[T]`
+/// or `&str` usually suffice and can be obtained from other types, too.
 ///
-/// **Known problems:** None.
+/// **Known problems:** The lint does not follow data. So if you have an
+/// argument `x` and write `let y = x; y.clone()` the lint will not suggest
+/// changing that `.clone()` to `.to_owned()`.
+///
+/// Other functions called from this function taking a `&String` or `&Vec`
+/// argument may also fail to compile if you change the argument. Applying
+/// this lint on them will fix the problem, but they may be in other crates.
+///
+/// Also there may be `fn(&Vec)`-typed references pointing to your function.
+/// If you have them, you will get a compiler error after applying this lint's
+/// suggestions. You then have the choice to undo your changes or change the
+/// type of the reference.
+///
+/// Note that if the function is part of your public interface, there may be
+/// other crates referencing it you may not be aware. Carefully deprecate the
+/// function before applying the lint suggestions in this case.
 ///
 /// **Example:**
 /// ```rust
@@ -87,25 +103,26 @@ impl LintPass for PointerPass {
 
 impl<'a, 'tcx> LateLintPass<'a, 'tcx> for PointerPass {
     fn check_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx Item) {
-        if let ItemFn(ref decl, _, _, _, _, _) = item.node {
-            check_fn(cx, decl, item.id);
+        if let ItemFn(ref decl, _, _, _, _, body_id) = item.node {
+            check_fn(cx, decl, item.id, Some(body_id));
         }
     }
 
     fn check_impl_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx ImplItem) {
-        if let ImplItemKind::Method(ref sig, _) = item.node {
+        if let ImplItemKind::Method(ref sig, body_id) = item.node {
             if let Some(NodeItem(it)) = cx.tcx.hir.find(cx.tcx.hir.get_parent(item.id)) {
                 if let ItemImpl(_, _, _, _, Some(_), _, _) = it.node {
                     return; // ignore trait impls
                 }
             }
-            check_fn(cx, &sig.decl, item.id);
+            check_fn(cx, &sig.decl, item.id, Some(body_id));
         }
     }
 
     fn check_trait_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx TraitItem) {
-        if let TraitItemKind::Method(ref sig, _) = item.node {
-            check_fn(cx, &sig.decl, item.id);
+        if let TraitItemKind::Method(ref sig, ref trait_method) = item.node {
+            let body_id = if let TraitMethod::Provided(b) = *trait_method { Some(b) } else { None };
+            check_fn(cx, &sig.decl, item.id, body_id);
         }
     }
 
@@ -123,12 +140,12 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for PointerPass {
     }
 }
 
-fn check_fn(cx: &LateContext, decl: &FnDecl, fn_id: NodeId) {
+fn check_fn(cx: &LateContext, decl: &FnDecl, fn_id: NodeId, opt_body_id: Option<BodyId>) {
     let fn_def_id = cx.tcx.hir.local_def_id(fn_id);
     let sig = cx.tcx.fn_sig(fn_def_id);
     let fn_ty = sig.skip_binder();
 
-    for (arg, ty) in decl.inputs.iter().zip(fn_ty.inputs()) {
+    for (idx, (arg, ty)) in decl.inputs.iter().zip(fn_ty.inputs()).enumerate() {
         if let ty::TyRef(
             _,
             ty::TypeAndMut {
@@ -146,7 +163,7 @@ fn check_fn(cx: &LateContext, decl: &FnDecl, fn_id: NodeId) {
                 ], {
                     ty_snippet = snippet_opt(cx, parameters.types[0].span);
                 });
-                //TODO: Suggestion
+                let spans = get_spans(cx, opt_body_id, idx, "to_owned");
                 span_lint_and_then(
                     cx,
                     PTR_ARG,
@@ -159,16 +176,30 @@ fn check_fn(cx: &LateContext, decl: &FnDecl, fn_id: NodeId) {
                                                "change this to",
                                                format!("&[{}]", snippet));
                         }
+                        for (clonespan, suggestion) in spans {
+                            db.span_suggestion(clonespan,
+                                               "change the `.clone()` to",
+                                               suggestion);
+                        }
                     }
                 );
             } else if match_type(cx, ty, &paths::STRING) {
-                span_lint_and_sugg(
+                let spans = get_spans(cx, opt_body_id, idx, "to_string");
+                span_lint_and_then(
                     cx,
                     PTR_ARG,
                     arg.span,
                     "writing `&String` instead of `&str` involves a new object where a slice will do.",
-                    "change this to",
-                    "&str".to_string()
+                    |db| {
+                        db.span_suggestion(arg.span,
+                                           "change this to",
+                                           "&str".into());
+                        for (clonespan, suggestion) in spans {
+                            db.span_suggestion_short(clonespan,
+                                               "change the `.clone` to ",
+                                               suggestion);
+                        }
+                    }
                 );
             }
         }
@@ -198,6 +229,54 @@ fn check_fn(cx: &LateContext, decl: &FnDecl, fn_id: NodeId) {
     }
 }
 
+fn get_spans(cx: &LateContext, opt_body_id: Option<BodyId>, idx: usize, fn_name: &'static str) -> Vec<(Span, String)> {
+    if let Some(body) = opt_body_id.map(|id| cx.tcx.hir.body(id)) {
+        get_binding_name(&body.arguments[idx]).map_or_else(Vec::new,
+                                                |name| extract_clone_suggestions(cx, name, fn_name, body))
+    } else {
+        vec![]
+    }
+}
+
+fn extract_clone_suggestions<'a, 'tcx: 'a>(cx: &LateContext<'a, 'tcx>, name: Name, fn_name: &'static str, body: &'tcx Body) -> Vec<(Span, String)> {
+    let mut visitor = PtrCloneVisitor {
+        cx,
+        name,
+        fn_name,
+        spans: vec![]
+    };
+    visitor.visit_body(body);
+    visitor.spans
+}
+
+struct PtrCloneVisitor<'a, 'tcx: 'a> {
+    cx: &'a LateContext<'a, 'tcx>,
+    name: Name,
+    fn_name: &'static str,
+    spans: Vec<(Span, String)>,
+}
+
+impl<'a, 'tcx: 'a> Visitor<'tcx> for PtrCloneVisitor<'a, 'tcx> {
+    fn visit_expr(&mut self, expr: &'tcx Expr) {
+        if let ExprMethodCall(ref seg, _, ref args) = expr.node {
+            if args.len() == 1 && match_var(&args[0], self.name) && seg.name == "clone" {
+                self.spans.push((expr.span, format!("{}.{}()", snippet(self.cx, args[0].span, "_"), self.fn_name)));
+            }
+            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
@@ -218,6 +218,17 @@ pub fn match_trait_method(cx: &LateContext, expr: &Expr, path: &[&str]) -> bool
     }
 }
 
+/// Check if an expression references a variable of the given name.
+pub fn match_var(expr: &Expr, var: Name) -> bool {
+    if let ExprPath(QPath::Resolved(None, ref path)) = expr.node {
+        if path.segments.len() == 1 && path.segments[0].name == var {
+            return true;
+        }
+    }
+    false
+}
+
+
 pub fn last_path_segment(path: &QPath) -> &PathSegment {
     match *path {
         QPath::Resolved(_, ref path) => path.segments
@@ -393,6 +404,16 @@ pub fn get_item_name(cx: &LateContext, expr: &Expr) -> Option<Name> {
     }
 }
 
+/// Get the name of a `Pat`, if any
+pub fn get_pat_name(pat: &Pat) -> Option<Name> {
+    match pat.node {
+        PatKind::Binding(_, _, ref spname, _) => Some(spname.node),
+        PatKind::Path(ref qpath) => single_segment_path(qpath).map(|ps| ps.name),
+        PatKind::Box(ref p) | PatKind::Ref(ref p, _) => get_pat_name(&*p),
+        _ => None,
+    }
+}
+
 struct ContainsName {
     name: Name,
     result: bool,
diff --git a/tests/ui/ptr_arg.rs b/tests/ui/ptr_arg.rs
@@ -1,6 +1,6 @@
 #![feature(plugin)]
 #![plugin(clippy)]
-#![allow(unused)]
+#![allow(unused, many_single_char_names)]
 #![warn(ptr_arg)]
 
 fn do_vec(x: &Vec<i64>) {
@@ -34,5 +34,24 @@ struct Bar;
 impl Foo for Bar {
     type Item = Vec<u8>;
     fn do_vec(x: &Vec<i64>) {}
-    fn do_item(x: &Vec<u8>) {}  
+    fn do_item(x: &Vec<u8>) {}
+}
+
+fn cloned(x: &Vec<u8>) -> Vec<u8> {
+    let e = x.clone();
+    let f = e.clone(); // OK
+    let g = x;
+    let h = g.clone(); // Alas, we cannot reliably detect this without following data.
+    let i = (e).clone();
+    x.clone()
+}
+
+fn str_cloned(x: &String) -> String {
+    let a = x.clone();
+    let b = x.clone();
+    let c = b.clone();
+    let d = a.clone()
+             .clone()
+             .clone();
+    x.clone()
 }
diff --git a/tests/ui/ptr_arg.stderr b/tests/ui/ptr_arg.stderr
@@ -18,5 +18,47 @@ error: writing `&Vec<_>` instead of `&[_]` involves one more reference and canno
 27 |     fn do_vec(x: &Vec<i64>);
    |                  ^^^^^^^^^ help: change this to: `&[i64]`
 
-error: aborting due to 3 previous errors
+error: writing `&Vec<_>` instead of `&[_]` involves one more reference and cannot be used with non-Vec-based slices.
+  --> $DIR/ptr_arg.rs:40:14
+   |
+40 | fn cloned(x: &Vec<u8>) -> Vec<u8> {
+   |              ^^^^^^^^
+   |
+help: change this to
+   |
+40 | fn cloned(x: &[u8]) -> Vec<u8> {
+   |              ^^^^^
+help: change the `.clone()` to
+   |
+41 |     let e = x.to_owned();
+   |             ^^^^^^^^^^^^
+help: change the `.clone()` to
+   |
+46 |     x.to_owned()
+   |     ^^^^^^^^^^^^
+
+error: writing `&String` instead of `&str` involves a new object where a slice will do.
+  --> $DIR/ptr_arg.rs:49:18
+   |
+49 | fn str_cloned(x: &String) -> String {
+   |                  ^^^^^^^
+   |
+help: change this to
+   |
+49 | fn str_cloned(x: &str) -> String {
+   |                  ^^^^
+help: change the `.clone` to 
+   |
+50 |     let a = x.to_string();
+   |             ^^^^^^^^^^^^^
+help: change the `.clone` to 
+   |
+51 |     let b = x.to_string();
+   |             ^^^^^^^^^^^^^
+help: change the `.clone` to 
+   |
+56 |     x.to_string()
+   |     ^^^^^^^^^^^^^
+
+error: aborting due to 5 previous errors
 
