checkpoint commit

* refactor in prep of using diagnostic items/make using diagnostic items easier
* changes from code review
* add to_os_string and to_path_buf checks

clippy_lints/src/methods/mod.rs

@@ -1518,7 +1518,7 @@ declare_clippy_lint! {
     /// **What it does:** Checks for the usage of `_.to_owned()`, `vec.to_vec()`, or similar on already owned types.
     ///
     /// **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
+    /// to why we are calling `to_vec` on something that is already a `Vec`
     ///
     /// **Known problems:** None.
     ///
@@ -1537,7 +1537,7 @@ declare_clippy_lint! {
     /// ```
     pub OWNED_TO_OWNED,
     style,
-    "using to_owned on already owned type"
+    "using `to_owned` on already owned type"
 }
 
 pub struct Methods {
@@ -1698,9 +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", ..] => owned_to_owned::lint(cx, expr, &paths::TO_OWNED_METHOD, None),
-            ["to_vec", ..] => owned_to_owned::lint(cx, expr, &paths::SLICE_TO_VEC, Some(&paths::VEC)),
-            ["to_path_buf", ..] => owned_to_owned::lint(cx, expr, &paths::PATH_TO_PATH_BUF, Some(&paths::PATH_BUF)),
+            ["to_owned", ..] => owned_to_owned::check(cx, expr, Some(&paths::TO_OWNED), None),
+            ["to_os_string", ..] => owned_to_owned::check(cx, expr, None, Some(&paths::OS_STRING)),
+            ["to_path_buf", ..] => owned_to_owned::check(cx, expr, None, Some(&paths::PATH_BUF)),
+            ["to_vec", ..] => owned_to_owned::check(cx, expr, None, Some(&paths::VEC)),
             _ => {},
         }
 

clippy_lints/src/methods/owned_to_owned.rs

@@ -1,4 +1,4 @@
-use crate::utils::{is_copy, match_def_path, match_type, span_lint_and_sugg, sugg};
+use crate::utils::{match_trait_method, match_type, span_lint_and_sugg};
 use if_chain::if_chain;
 use rustc_errors::Applicability;
 use rustc_hir as hir;
@@ -8,36 +8,28 @@ use rustc_middle::ty::TyS;
 
 use super::OWNED_TO_OWNED;
 
-pub fn lint(cx: &LateContext<'_>, expr: &hir::Expr<'_>, expected_path: &[&str], expected_type: Option<&[&str]>) {
+pub fn check(
+    cx: &LateContext<'_>,
+    expr: &hir::Expr<'_>,
+    trait_path: Option<&[&str]>,
+    expected_type_path: Option<&[&str]>,
+) {
     if_chain! {
         if let ExprKind::MethodCall(method_path, _, [arg], _) = &expr.kind;
-        if let Some(meth_did) = cx.typeck_results().type_dependent_def_id(expr.hir_id);
-        if let Some(snippet) = sugg::Sugg::hir_opt(cx, arg);
         let return_type = cx.typeck_results().expr_ty(&expr);
         let input_type = cx.typeck_results().expr_ty(arg);
+        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 match_def_path(cx, meth_did, expected_path) &&
-            expected_type.map_or(true, |expected_type_path| match_type(cx,input_type,expected_type_path));
+        if trait_path.map_or(true, |path| match_trait_method(cx, expr, path)) &&
+            expected_type_path.map_or(true, |path| match_type(cx, input_type, path));
         then {
-            if is_copy(cx,return_type) {
-                // if return_type is copy, have the suggestion be to replace the call with the variable itself
-                //  this prevents fixing this lint, only to have clone_on_copy complain next.
-                span_lint_and_sugg(
-                    cx,OWNED_TO_OWNED,expr.span,
-                    &format!("using `{}` on an already owned type",method_path.ident.name),
-                    "replace this with",
-                    snippet.to_string(),
-                    Applicability::MaybeIncorrect
-                );
-            } else {
-                span_lint_and_sugg(
-                    cx,OWNED_TO_OWNED,method_path.ident.span,
-                    &format!("using `{}` on an already owned type",method_path.ident.name),
-                    "replace this with",
-                    "clone".to_string(),
-                    Applicability::MaybeIncorrect
-                );
-            }
+            span_lint_and_sugg(
+                cx,OWNED_TO_OWNED,method_path.ident.span,
+                &format!("`{}` called on a `{}`", method_path.ident.name, ty_name),
+                "consider using",
+                "clone".to_string(),
+                Applicability::MaybeIncorrect
+            );
         }
     }
 }

clippy_lints/src/utils/paths.rs

@@ -136,7 +136,6 @@ pub const SERDE_DE_VISITOR: [&str; 3] = ["serde", "de", "Visitor"];
 pub const SLICE_FROM_RAW_PARTS: [&str; 4] = ["core", "slice", "raw", "from_raw_parts"];
 pub const SLICE_FROM_RAW_PARTS_MUT: [&str; 4] = ["core", "slice", "raw", "from_raw_parts_mut"];
 pub const SLICE_INTO_VEC: [&str; 4] = ["alloc", "slice", "<impl [T]>", "into_vec"];
-pub const SLICE_TO_VEC: [&str; 4] = ["alloc", "slice", "<impl [T]>", "to_vec"];
 pub const SLICE_ITER: [&str; 4] = ["core", "slice", "iter", "Iter"];
 pub const STDERR: [&str; 4] = ["std", "io", "stdio", "stderr"];
 pub const STDOUT: [&str; 4] = ["std", "io", "stdio", "stdout"];

tests/ui/owned_to_owned.rs

@@ -1,13 +1,32 @@
 #![warn(clippy::owned_to_owned)]
 #![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()
 }
-fn return_owned_from_ref(val: &Kitten) -> Kitten {
-    val.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)]
@@ -27,23 +46,56 @@ impl ToOwned for BorrowedKitten {
     }
 }
 
+mod weird {
+    #[allow(clippy::ptr_arg)]
+    pub fn to_vec(v: &Vec<u32>) -> Vec<u32> {
+        v.clone()
+    }
+}
+
 fn main() {
-    let result = vec![5];
-    let _ = return_owned_from_slice(&result);
-    let _ = result.to_owned();
-    let _ = result.to_vec();
+    let vec = vec![5];
+    let _ = return_owned_from_slice(&vec);
+    let _ = vec.to_owned();
+    let _ = vec.to_vec();
+
+    // we expect no lints for this
+    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 _ = return_owned_from_ref(&kitten);
+    let _ = own_same_from_ref(&kitten);
 
+    // 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/owned_to_owned.stderr

@@ -1,40 +1,52 @@
-error: using `to_owned` on an already owned type
-  --> $DIR/owned_to_owned.rs:33:20
+error: `to_owned` called on a `Vec`
+  --> $DIR/owned_to_owned.rs:59:17
    |
-LL |     let _ = result.to_owned();
-   |                    ^^^^^^^^ help: replace this with: `clone`
+LL |     let _ = vec.to_owned();
+   |                 ^^^^^^^^ help: consider using: `clone`
    |
    = note: `-D clippy::owned-to-owned` implied by `-D warnings`
 
-error: using `to_vec` on an already owned type
-  --> $DIR/owned_to_owned.rs:34:20
+error: `to_vec` called on a `Vec`
+  --> $DIR/owned_to_owned.rs:60:17
    |
-LL |     let _ = result.to_vec();
-   |                    ^^^^^^ help: replace this with: `clone`
+LL |     let _ = vec.to_vec();
+   |                 ^^^^^^ help: consider using: `clone`
 
-error: using `to_owned` on an already owned type
-  --> $DIR/owned_to_owned.rs:37:17
+error: `to_owned` called on a `String`
+  --> $DIR/owned_to_owned.rs:78:17
    |
 LL |     let _ = str.to_owned();
-   |                 ^^^^^^^^ help: replace this with: `clone`
+   |                 ^^^^^^^^ help: consider using: `clone`
 
-error: using `to_owned` on an already owned type
-  --> $DIR/owned_to_owned.rs:40:13
+error: `to_owned` called on a `Kitten`
+  --> $DIR/owned_to_owned.rs:82:20
    |
 LL |     let _ = kitten.to_owned();
-   |             ^^^^^^^^^^^^^^^^^ help: replace this with: `kitten`
+   |                    ^^^^^^^^ help: consider using: `clone`
 
-error: using `to_owned` on an already owned type
-  --> $DIR/owned_to_owned.rs:47:21
+error: `to_owned` called on a `PathBuf`
+  --> $DIR/owned_to_owned.rs:90:21
    |
 LL |     let _ = pathbuf.to_owned();
-   |                     ^^^^^^^^ help: replace this with: `clone`
+   |                     ^^^^^^^^ help: consider using: `clone`
 
-error: using `to_path_buf` on an already owned type
-  --> $DIR/owned_to_owned.rs:48:21
+error: `to_path_buf` called on a `PathBuf`
+  --> $DIR/owned_to_owned.rs:91:21
    |
 LL |     let _ = pathbuf.to_path_buf();
-   |                     ^^^^^^^^^^^ help: replace this with: `clone`
+   |                     ^^^^^^^^^^^ help: consider using: `clone`
 
-error: aborting due to 6 previous errors
+error: `to_owned` called on a `OsString`
+  --> $DIR/owned_to_owned.rs:94:23
+   |
+LL |     let _ = os_string.to_owned();
+   |                       ^^^^^^^^ help: consider using: `clone`
+
+error: `to_os_string` called on a `OsString`
+  --> $DIR/owned_to_owned.rs:95:23
+   |
+LL |     let _ = os_string.to_os_string();
+   |                       ^^^^^^^^^^^^ help: consider using: `clone`
+
+error: aborting due to 8 previous errors