Auto merge of #6730 - anall:feature/owned_to_owned_methods, r=camsteffen

added new lint `owned_to_owned`

Adding new lint `owned_to_owned`

Creating draft PR to have this looked over.
I think this takes all advice I received into account.

I did have to update the `redundant_clone` test to ignore this lint -- I felt that was the safest action.

closes: #6715
changelog: added new lint `implicit_clone`

CHANGELOG.md

@@ -2104,6 +2104,7 @@ Released 2018-09-13
 [`if_not_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_not_else
 [`if_same_then_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_same_then_else
 [`ifs_same_cond`]: https://rust-lang.github.io/rust-clippy/master/index.html#ifs_same_cond
+[`implicit_clone`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_clone
 [`implicit_hasher`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_hasher
 [`implicit_return`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_return
 [`implicit_saturating_sub`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_saturating_sub

clippy_lints/src/lib.rs

@@ -769,6 +769,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &methods::FLAT_MAP_IDENTITY,
         &methods::FROM_ITER_INSTEAD_OF_COLLECT,
         &methods::GET_UNWRAP,
+        &methods::IMPLICIT_CLONE,
         &methods::INEFFICIENT_TO_STRING,
         &methods::INSPECT_FOR_EACH,
         &methods::INTO_ITER_ON_REF,
@@ -1380,6 +1381,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&matches::SINGLE_MATCH_ELSE),
         LintId::of(&methods::FILTER_MAP),
         LintId::of(&methods::FILTER_MAP_NEXT),
+        LintId::of(&methods::IMPLICIT_CLONE),
         LintId::of(&methods::INEFFICIENT_TO_STRING),
         LintId::of(&methods::MAP_FLATTEN),
         LintId::of(&methods::MAP_UNWRAP_OR),

clippy_lints/src/methods/implicit_clone.rs

@@ -0,0 +1,32 @@
+use crate::utils::span_lint_and_sugg;
+use if_chain::if_chain;
+use rustc_errors::Applicability;
+use rustc_hir as hir;
+use rustc_hir::ExprKind;
+use rustc_lint::LateContext;
+use rustc_middle::ty::TyS;
+use rustc_span::symbol::Symbol;
+
+use super::IMPLICIT_CLONE;
+use clippy_utils::is_diagnostic_assoc_item;
+
+pub fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, trait_diagnostic: Symbol) {
+    if_chain! {
+        if let ExprKind::MethodCall(method_path, _, [arg], _) = &expr.kind;
+        let return_type = cx.typeck_results().expr_ty(&expr);
+        let input_type = cx.typeck_results().expr_ty(arg).peel_refs();
+        if let Some(expr_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id);
+        if let Some(ty_name) = input_type.ty_adt_def().map(|adt_def| cx.tcx.item_name(adt_def.did));
+        if TyS::same_type(return_type, input_type);
+        if is_diagnostic_assoc_item(cx, expr_def_id, trait_diagnostic);
+        then {
+            span_lint_and_sugg(
+                cx,IMPLICIT_CLONE,method_path.ident.span,
+                &format!("implicitly cloning a `{}` by calling `{}` on its dereferenced type", ty_name, method_path.ident.name),
+                "consider using",
+                "clone".to_string(),
+                Applicability::MachineApplicable
+            );
+        }
+    }
+}

clippy_lints/src/methods/mod.rs

@@ -1,6 +1,7 @@
 mod bind_instead_of_map;
 mod bytes_nth;
 mod filter_map_identity;
+mod implicit_clone;
 mod inefficient_to_string;
 mod inspect_for_each;
 mod manual_saturating_arithmetic;
@@ -1513,6 +1514,32 @@ declare_clippy_lint! {
     "replace `.bytes().nth()` with `.as_bytes().get()`"
 }
 
+declare_clippy_lint! {
+    /// **What it does:** Checks for the usage of `_.to_owned()`, `vec.to_vec()`, or similar when calling `_.clone()` would be clearer.
+    ///
+    /// **Why is this bad?** These methods do the same thing as `_.clone()` but may be confusing as
+    /// to why we are calling `to_vec` on something that is already a `Vec` or calling `to_owned` on something that is already owned.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    ///
+    /// ```rust
+    /// let a = vec![1, 2, 3];
+    /// let b = a.to_vec();
+    /// let c = a.to_owned();
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// let a = vec![1, 2, 3];
+    /// let b = a.clone();
+    /// let c = a.clone();
+    /// ```
+    pub IMPLICIT_CLONE,
+    pedantic,
+    "implicitly cloning a value by invoking a function on its dereferenced type"
+}
+
 pub struct Methods {
     msrv: Option<RustcVersion>,
 }
@@ -1579,6 +1606,7 @@ impl_lint_pass!(Methods => [
     MAP_COLLECT_RESULT_UNIT,
     FROM_ITER_INSTEAD_OF_COLLECT,
     INSPECT_FOR_EACH,
+    IMPLICIT_CLONE
 ]);
 
 impl<'tcx> LateLintPass<'tcx> for Methods {
@@ -1670,6 +1698,10 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
             ["ok_or_else", ..] => unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], "ok_or"),
             ["collect", "map"] => lint_map_collect(cx, expr, arg_lists[1], arg_lists[0]),
             ["for_each", "inspect"] => inspect_for_each::lint(cx, expr, method_spans[1]),
+            ["to_owned", ..] => implicit_clone::check(cx, expr, sym::ToOwned),
+            ["to_os_string", ..] => implicit_clone::check(cx, expr, sym::OsStr),
+            ["to_path_buf", ..] => implicit_clone::check(cx, expr, sym::Path),
+            ["to_vec", ..] => implicit_clone::check(cx, expr, sym::slice),
             _ => {},
         }
 

clippy_utils/src/lib.rs

@@ -237,6 +237,22 @@ pub fn match_trait_method(cx: &LateContext<'_>, expr: &Expr<'_>, path: &[&str])
     trt_id.map_or(false, |trt_id| match_def_path(cx, trt_id, path))
 }
 
+/// Checks if the method call given in `expr` belongs to a trait or other container with a given
+/// diagnostic item
+pub fn is_diagnostic_assoc_item(cx: &LateContext<'_>, def_id: DefId, diag_item: Symbol) -> bool {
+    cx.tcx
+        .opt_associated_item(def_id)
+        .and_then(|associated_item| match associated_item.container {
+            ty::TraitContainer(assoc_def_id) => Some(assoc_def_id),
+            ty::ImplContainer(assoc_def_id) => match cx.tcx.type_of(assoc_def_id).kind() {
+                ty::Adt(adt, _) => Some(adt.did),
+                ty::Slice(_) => cx.tcx.get_diagnostic_item(sym::slice), // this isn't perfect but it works
+                _ => None,
+            },
+        })
+        .map_or(false, |assoc_def_id| cx.tcx.is_diagnostic_item(diag_item, assoc_def_id))
+}
+
 /// Checks if an expression references a variable of the given name.
 pub fn match_var(expr: &Expr<'_>, var: Symbol) -> bool {
     if let ExprKind::Path(QPath::Resolved(None, ref path)) = expr.kind {

tests/ui/cmp_owned/without_suggestion.rs

@@ -1,4 +1,5 @@
 #[allow(clippy::unnecessary_operation)]
+#[allow(clippy::implicit_clone)]
 
 fn main() {
     let x = &Baz;

tests/ui/cmp_owned/without_suggestion.stderr

@@ -1,5 +1,5 @@
 error: this creates an owned instance just for comparison
-  --> $DIR/without_suggestion.rs:6:5
+  --> $DIR/without_suggestion.rs:7:5
    |
 LL |     y.to_owned() == *x;
    |     ^^^^^^^^^^^^^^^^^^ try implementing the comparison without allocating
@@ -7,13 +7,13 @@ LL |     y.to_owned() == *x;
    = note: `-D clippy::cmp-owned` implied by `-D warnings`
 
 error: this creates an owned instance just for comparison
-  --> $DIR/without_suggestion.rs:10:5
+  --> $DIR/without_suggestion.rs:11:5
    |
 LL |     y.to_owned() == **x;
    |     ^^^^^^^^^^^^^^^^^^^ try implementing the comparison without allocating
 
 error: this creates an owned instance just for comparison
-  --> $DIR/without_suggestion.rs:17:9
+  --> $DIR/without_suggestion.rs:18:9
    |
 LL |         self.to_owned() == *other
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^ try implementing the comparison without allocating

tests/ui/implicit_clone.rs

@@ -0,0 +1,108 @@
+#![warn(clippy::implicit_clone)]
+#![allow(clippy::redundant_clone)]
+use std::borrow::Borrow;
+use std::ffi::{OsStr, OsString};
+use std::path::PathBuf;
+
+fn return_owned_from_slice(slice: &[u32]) -> Vec<u32> {
+    slice.to_owned()
+}
+
+pub fn own_same<T>(v: T) -> T
+where
+    T: ToOwned<Owned = T>,
+{
+    v.to_owned()
+}
+
+pub fn own_same_from_ref<T>(v: &T) -> T
+where
+    T: ToOwned<Owned = T>,
+{
+    v.to_owned()
+}
+
+pub fn own_different<T, U>(v: T) -> U
+where
+    T: ToOwned<Owned = U>,
+{
+    v.to_owned()
+}
+
+#[derive(Copy, Clone)]
+struct Kitten {}
+impl Kitten {
+    // badly named method
+    fn to_vec(self) -> Kitten {
+        Kitten {}
+    }
+}
+impl Borrow<BorrowedKitten> for Kitten {
+    fn borrow(&self) -> &BorrowedKitten {
+        static VALUE: BorrowedKitten = BorrowedKitten {};
+        &VALUE
+    }
+}
+
+struct BorrowedKitten {}
+impl ToOwned for BorrowedKitten {
+    type Owned = Kitten;
+    fn to_owned(&self) -> Kitten {
+        Kitten {}
+    }
+}
+
+mod weird {
+    #[allow(clippy::ptr_arg)]
+    pub fn to_vec(v: &Vec<u32>) -> Vec<u32> {
+        v.clone()
+    }
+}
+
+fn main() {
+    let vec = vec![5];
+    let _ = return_owned_from_slice(&vec);
+    let _ = vec.to_owned();
+    let _ = vec.to_vec();
+
+    let vec_ref = &vec;
+    let _ = return_owned_from_slice(&vec_ref);
+    let _ = vec_ref.to_owned();
+    let _ = vec_ref.to_vec();
+
+    // we expect no lint for this
+    let _ = weird::to_vec(&vec);
+
+    // we expect no lints for this
+    let slice: &[u32] = &[1, 2, 3, 4, 5];
+    let _ = return_owned_from_slice(slice);
+    let _ = slice.to_owned();
+    let _ = slice.to_vec();
+
+    let str = "hello world".to_string();
+    let _ = str.to_owned();
+
+    // testing w/ an arbitrary type
+    let kitten = Kitten {};
+    let _ = kitten.to_owned();
+    let _ = own_same_from_ref(&kitten);
+    // this shouln't lint
+    let _ = kitten.to_vec();
+
+    // we expect no lints for this
+    let borrowed = BorrowedKitten {};
+    let _ = borrowed.to_owned();
+
+    let pathbuf = PathBuf::new();
+    let _ = pathbuf.to_owned();
+    let _ = pathbuf.to_path_buf();
+
+    let os_string = OsString::from("foo");
+    let _ = os_string.to_owned();
+    let _ = os_string.to_os_string();
+
+    // we expect no lints for this
+    let os_str = OsStr::new("foo");
+    let _ = os_str.to_owned();
+    let _ = os_str.to_os_string();
+}

tests/ui/implicit_clone.stderr

@@ -0,0 +1,64 @@
+error: implicitly cloning a `Vec` by calling `to_owned` on its dereferenced type
+  --> $DIR/implicit_clone.rs:65:17
+   |
+LL |     let _ = vec.to_owned();
+   |                 ^^^^^^^^ help: consider using: `clone`
+   |
+   = note: `-D clippy::implicit-clone` implied by `-D warnings`
+
+error: implicitly cloning a `Vec` by calling `to_vec` on its dereferenced type
+  --> $DIR/implicit_clone.rs:66:17
+   |
+LL |     let _ = vec.to_vec();
+   |                 ^^^^^^ help: consider using: `clone`
+
+error: implicitly cloning a `Vec` by calling `to_owned` on its dereferenced type
+  --> $DIR/implicit_clone.rs:70:21
+   |
+LL |     let _ = vec_ref.to_owned();
+   |                     ^^^^^^^^ help: consider using: `clone`
+
+error: implicitly cloning a `Vec` by calling `to_vec` on its dereferenced type
+  --> $DIR/implicit_clone.rs:71:21
+   |
+LL |     let _ = vec_ref.to_vec();
+   |                     ^^^^^^ help: consider using: `clone`
+
+error: implicitly cloning a `String` by calling `to_owned` on its dereferenced type
+  --> $DIR/implicit_clone.rs:83:17
+   |
+LL |     let _ = str.to_owned();
+   |                 ^^^^^^^^ help: consider using: `clone`
+
+error: implicitly cloning a `Kitten` by calling `to_owned` on its dereferenced type
+  --> $DIR/implicit_clone.rs:87:20
+   |
+LL |     let _ = kitten.to_owned();
+   |                    ^^^^^^^^ help: consider using: `clone`
+
+error: implicitly cloning a `PathBuf` by calling `to_owned` on its dereferenced type
+  --> $DIR/implicit_clone.rs:97:21
+   |
+LL |     let _ = pathbuf.to_owned();
+   |                     ^^^^^^^^ help: consider using: `clone`
+
+error: implicitly cloning a `PathBuf` by calling `to_path_buf` on its dereferenced type
+  --> $DIR/implicit_clone.rs:98:21
+   |
+LL |     let _ = pathbuf.to_path_buf();
+   |                     ^^^^^^^^^^^ help: consider using: `clone`
+
+error: implicitly cloning a `OsString` by calling `to_owned` on its dereferenced type
+  --> $DIR/implicit_clone.rs:101:23
+   |
+LL |     let _ = os_string.to_owned();
+   |                       ^^^^^^^^ help: consider using: `clone`
+
+error: implicitly cloning a `OsString` by calling `to_os_string` on its dereferenced type
+  --> $DIR/implicit_clone.rs:102:23
+   |
+LL |     let _ = os_string.to_os_string();
+   |                       ^^^^^^^^^^^^ help: consider using: `clone`
+
+error: aborting due to 10 previous errors
+

tests/ui/redundant_clone.fixed

@@ -1,6 +1,7 @@
 // run-rustfix
 // rustfix-only-machine-applicable
 
+#![allow(clippy::implicit_clone)]
 use std::ffi::OsString;
 use std::path::Path;
 

tests/ui/redundant_clone.rs

@@ -1,6 +1,7 @@
 // run-rustfix
 // rustfix-only-machine-applicable
 
+#![allow(clippy::implicit_clone)]
 use std::ffi::OsString;
 use std::path::Path;
 

tests/ui/redundant_clone.stderr

@@ -1,168 +1,168 @@
 error: redundant clone
-  --> $DIR/redundant_clone.rs:8:42
+  --> $DIR/redundant_clone.rs:9:42
    |
 LL |     let _s = ["lorem", "ipsum"].join(" ").to_string();
    |                                          ^^^^^^^^^^^^ help: remove this
    |
    = note: `-D clippy::redundant-clone` implied by `-D warnings`
 note: this value is dropped without further use
-  --> $DIR/redundant_clone.rs:8:14
+  --> $DIR/redundant_clone.rs:9:14
    |
 LL |     let _s = ["lorem", "ipsum"].join(" ").to_string();
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: redundant clone
-  --> $DIR/redundant_clone.rs:11:15
+  --> $DIR/redundant_clone.rs:12:15
    |
 LL |     let _s = s.clone();
    |               ^^^^^^^^ help: remove this
    |
 note: this value is dropped without further use
-  --> $DIR/redundant_clone.rs:11:14
+  --> $DIR/redundant_clone.rs:12:14
    |
 LL |     let _s = s.clone();
    |              ^
 
 error: redundant clone
-  --> $DIR/redundant_clone.rs:14:15
+  --> $DIR/redundant_clone.rs:15:15
    |
 LL |     let _s = s.to_string();
    |               ^^^^^^^^^^^^ help: remove this
    |
 note: this value is dropped without further use
-  --> $DIR/redundant_clone.rs:14:14
+  --> $DIR/redundant_clone.rs:15:14
    |
 LL |     let _s = s.to_string();
    |              ^
 
 error: redundant clone
-  --> $DIR/redundant_clone.rs:17:15
+  --> $DIR/redundant_clone.rs:18:15
    |
 LL |     let _s = s.to_owned();
    |               ^^^^^^^^^^^ help: remove this
    |
 note: this value is dropped without further use
-  --> $DIR/redundant_clone.rs:17:14
+  --> $DIR/redundant_clone.rs:18:14
    |
 LL |     let _s = s.to_owned();
    |              ^
 
 error: redundant clone
-  --> $DIR/redundant_clone.rs:19:42
+  --> $DIR/redundant_clone.rs:20:42
    |
 LL |     let _s = Path::new("/a/b/").join("c").to_owned();
    |                                          ^^^^^^^^^^^ help: remove this
    |
 note: this value is dropped without further use
-  --> $DIR/redundant_clone.rs:19:14
+  --> $DIR/redundant_clone.rs:20:14
    |
 LL |     let _s = Path::new("/a/b/").join("c").to_owned();
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: redundant clone
-  --> $DIR/redundant_clone.rs:21:42
+  --> $DIR/redundant_clone.rs:22:42
    |
 LL |     let _s = Path::new("/a/b/").join("c").to_path_buf();
    |                                          ^^^^^^^^^^^^^^ help: remove this
    |
 note: this value is dropped without further use
-  --> $DIR/redundant_clone.rs:21:14
+  --> $DIR/redundant_clone.rs:22:14
    |
 LL |     let _s = Path::new("/a/b/").join("c").to_path_buf();
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: redundant clone
-  --> $DIR/redundant_clone.rs:23:29
+  --> $DIR/redundant_clone.rs:24:29
    |
 LL |     let _s = OsString::new().to_owned();
    |                             ^^^^^^^^^^^ help: remove this
    |
 note: this value is dropped without further use
-  --> $DIR/redundant_clone.rs:23:14
+  --> $DIR/redundant_clone.rs:24:14
    |
 LL |     let _s = OsString::new().to_owned();
    |              ^^^^^^^^^^^^^^^
 
 error: redundant clone
-  --> $DIR/redundant_clone.rs:25:29
+  --> $DIR/redundant_clone.rs:26:29
    |
 LL |     let _s = OsString::new().to_os_string();
    |                             ^^^^^^^^^^^^^^^ help: remove this
    |
 note: this value is dropped without further use
-  --> $DIR/redundant_clone.rs:25:14
+  --> $DIR/redundant_clone.rs:26:14
    |
 LL |     let _s = OsString::new().to_os_string();
    |              ^^^^^^^^^^^^^^^
 
 error: redundant clone
-  --> $DIR/redundant_clone.rs:32:19
+  --> $DIR/redundant_clone.rs:33:19
    |
 LL |     let _t = tup.0.clone();
    |                   ^^^^^^^^ help: remove this
    |
 note: this value is dropped without further use
-  --> $DIR/redundant_clone.rs:32:14
+  --> $DIR/redundant_clone.rs:33:14
    |
 LL |     let _t = tup.0.clone();
    |              ^^^^^
 
 error: redundant clone
-  --> $DIR/redundant_clone.rs:62:22
+  --> $DIR/redundant_clone.rs:63:22
    |
 LL |         (a.clone(), a.clone())
    |                      ^^^^^^^^ help: remove this
    |
 note: this value is dropped without further use
-  --> $DIR/redundant_clone.rs:62:21
+  --> $DIR/redundant_clone.rs:63:21
    |
 LL |         (a.clone(), a.clone())
    |                     ^
 
-error: redundant clone
-  --> $DIR/redundant_clone.rs:122:15
-   |
-LL |     let _s = s.clone();
-   |               ^^^^^^^^ help: remove this
-   |
-note: this value is dropped without further use
-  --> $DIR/redundant_clone.rs:122:14
-   |
-LL |     let _s = s.clone();
-   |              ^
-
 error: redundant clone
   --> $DIR/redundant_clone.rs:123:15
    |
-LL |     let _t = t.clone();
+LL |     let _s = s.clone();
    |               ^^^^^^^^ help: remove this
    |
 note: this value is dropped without further use
   --> $DIR/redundant_clone.rs:123:14
    |
+LL |     let _s = s.clone();
+   |              ^
+
+error: redundant clone
+  --> $DIR/redundant_clone.rs:124:15
+   |
+LL |     let _t = t.clone();
+   |               ^^^^^^^^ help: remove this
+   |
+note: this value is dropped without further use
+  --> $DIR/redundant_clone.rs:124:14
+   |
 LL |     let _t = t.clone();
    |              ^
 
 error: redundant clone
-  --> $DIR/redundant_clone.rs:133:19
+  --> $DIR/redundant_clone.rs:134:19
    |
 LL |         let _f = f.clone();
    |                   ^^^^^^^^ help: remove this
    |
 note: this value is dropped without further use
-  --> $DIR/redundant_clone.rs:133:18
+  --> $DIR/redundant_clone.rs:134:18
    |
 LL |         let _f = f.clone();
    |                  ^
 
 error: redundant clone
-  --> $DIR/redundant_clone.rs:145:14
+  --> $DIR/redundant_clone.rs:146:14
    |
 LL |     let y = x.clone().join("matthias");
    |              ^^^^^^^^ help: remove this
    |
 note: cloned value is neither consumed nor mutated
-  --> $DIR/redundant_clone.rs:145:13
+  --> $DIR/redundant_clone.rs:146:13
    |
 LL |     let y = x.clone().join("matthias");
    |             ^^^^^^^^^