Address review comments

* Share a list of methods with `implicit_clone`
* Ensure no overlap with `redundant_clone`

6 files changed, 160 insertions, 67 deletions

diff --git a/clippy_lints/src/methods/implicit_clone.rs b/clippy_lints/src/methods/implicit_clone.rs
index 81c42de145f..ff7904ce582 100644
--- a/clippy_lints/src/methods/implicit_clone.rs
+++ b/clippy_lints/src/methods/implicit_clone.rs
@@ -12,15 +12,7 @@ use super::IMPLICIT_CLONE;
 pub fn check(cx: &LateContext<'_>, method_name: &str, expr: &hir::Expr<'_>, recv: &hir::Expr<'_>, span: Span) {
     if_chain! {
         if let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id);
-        if match method_name {
-            "to_os_string" => is_diag_item_method(cx, method_def_id, sym::OsStr),
-            "to_owned" => is_diag_trait_item(cx, method_def_id, sym::ToOwned),
-            "to_path_buf" => is_diag_item_method(cx, method_def_id, sym::Path),
-            "to_vec" => cx.tcx.impl_of_method(method_def_id)
-                .map(|impl_did| Some(impl_did) == cx.tcx.lang_items().slice_alloc_impl())
-                == Some(true),
-            _ => false,
-        };
+        if is_clone_like(cx, method_name, method_def_id);
         let return_type = cx.typeck_results().expr_ty(expr);
         let input_type = cx.typeck_results().expr_ty(recv).peel_refs();
         if let Some(ty_name) = input_type.ty_adt_def().map(|adt_def| cx.tcx.item_name(adt_def.did));
@@ -38,3 +30,19 @@ pub fn check(cx: &LateContext<'_>, method_name: &str, expr: &hir::Expr<'_>, recv
         }
     }
 }
+
+/// Returns true if the named method can be used to clone the receiver.
+pub fn is_clone_like(cx: &LateContext<'_>, method_name: &str, method_def_id: hir::def_id::DefId) -> bool {
+    match method_name {
+        "to_os_string" => is_diag_item_method(cx, method_def_id, sym::OsStr),
+        "to_owned" => is_diag_trait_item(cx, method_def_id, sym::ToOwned),
+        "to_path_buf" => is_diag_item_method(cx, method_def_id, sym::Path),
+        "to_vec" => {
+            cx.tcx
+                .impl_of_method(method_def_id)
+                .map(|impl_did| Some(impl_did) == cx.tcx.lang_items().slice_alloc_impl())
+                == Some(true)
+        },
+        _ => false,
+    }
+}
diff --git a/clippy_lints/src/methods/unnecessary_to_owned.rs b/clippy_lints/src/methods/unnecessary_to_owned.rs
index 307de7bb65a..b380f734bff 100644
--- a/clippy_lints/src/methods/unnecessary_to_owned.rs
+++ b/clippy_lints/src/methods/unnecessary_to_owned.rs
@@ -1,7 +1,8 @@
+use super::implicit_clone::is_clone_like;
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::source::snippet_opt;
 use clippy_utils::ty::{implements_trait, is_copy, peel_mid_ty_refs};
-use clippy_utils::{get_parent_expr, match_def_path, paths};
+use clippy_utils::{get_parent_expr, is_diag_item_method, is_diag_trait_item};
 use rustc_errors::Applicability;
 use rustc_hir::{def_id::DefId, BorrowKind, Expr, ExprKind};
 use rustc_lint::LateContext;
@@ -14,28 +15,17 @@ use std::cmp::max;
 
 use super::UNNECESSARY_TO_OWNED;
 
-const TO_OWNED_LIKE_PATHS: &[&[&str]] = &[
-    &paths::COW_INTO_OWNED,
-    &paths::OS_STR_TO_OS_STRING,
-    &paths::PATH_TO_PATH_BUF,
-    &paths::SLICE_TO_VEC,
-    &paths::TO_OWNED_METHOD,
-    &paths::TO_STRING_METHOD,
-];
-
 pub fn check(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, method_name: Symbol, args: &'tcx [Expr<'tcx>]) {
     if_chain! {
         if let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id);
-        if TO_OWNED_LIKE_PATHS
-            .iter()
-            .any(|path| match_def_path(cx, method_def_id, path));
+        if is_to_owned_like(cx, method_name, method_def_id);
         if let [receiver] = args;
         then {
             // At this point, we know the call is of a `to_owned`-like function. The functions
             // `check_addr_of_expr` and `check_call_arg` determine whether the call is unnecessary
             // based on its context, that is, whether it is a referent in an `AddrOf` expression or
             // an argument in a function call.
-            if check_addr_of_expr(cx, expr, method_name, receiver) {
+            if check_addr_of_expr(cx, expr, method_name, method_def_id, receiver) {
                 return;
             }
             check_call_arg(cx, expr, method_name, receiver);
@@ -45,10 +35,12 @@ pub fn check(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, method_name: Symbol
 
 /// Checks whether `expr` is a referent in an `AddrOf` expression and, if so, determines whether its
 /// call of a `to_owned`-like function is unnecessary.
+#[allow(clippy::too_many_lines)]
 fn check_addr_of_expr(
     cx: &LateContext<'tcx>,
     expr: &'tcx Expr<'tcx>,
     method_name: Symbol,
+    method_def_id: DefId,
     receiver: &'tcx Expr<'tcx>,
 ) -> bool {
     if_chain! {
@@ -100,14 +92,17 @@ fn check_addr_of_expr(
             ] => Some(target_ty),
             _ => None,
         };
+        let receiver_ty = cx.typeck_results().expr_ty(receiver);
+        // Only flag cases where the receiver is copyable or the method is `Cow::into_owned`. This
+        // restriction is to ensure there is not overlap between `redundant_clone` and this lint.
+        if is_copy(cx, receiver_ty) || is_cow_into_owned(cx, method_name, method_def_id);
+        if let Some(receiver_snippet) = snippet_opt(cx, receiver.span);
         then {
             let (target_ty, n_target_refs) = peel_mid_ty_refs(target_ty);
-            let receiver_ty = cx.typeck_results().expr_ty(receiver);
             let (receiver_ty, n_receiver_refs) = peel_mid_ty_refs(receiver_ty);
             if_chain! {
                 if receiver_ty == target_ty;
                 if n_target_refs >= n_receiver_refs;
-                if let Some(receiver_snippet) = snippet_opt(cx, receiver.span);
                 then {
                     span_lint_and_sugg(
                         cx,
@@ -122,21 +117,32 @@ fn check_addr_of_expr(
                 }
             }
             if implements_deref_trait(cx, receiver_ty, target_ty) {
-                span_lint_and_sugg(
-                    cx,
-                    UNNECESSARY_TO_OWNED,
-                    expr.span.with_lo(receiver.span.hi()),
-                    &format!("unnecessary use of `{}`", method_name),
-                    "remove this",
-                    String::new(),
-                    Applicability::MachineApplicable,
-                );
+                if n_receiver_refs > 0 {
+                    span_lint_and_sugg(
+                        cx,
+                        UNNECESSARY_TO_OWNED,
+                        parent.span,
+                        &format!("unnecessary use of `{}`", method_name),
+                        "use",
+                        receiver_snippet,
+                        Applicability::MachineApplicable,
+                    );
+                } else {
+                    span_lint_and_sugg(
+                        cx,
+                        UNNECESSARY_TO_OWNED,
+                        expr.span.with_lo(receiver.span.hi()),
+                        &format!("unnecessary use of `{}`", method_name),
+                        "remove this",
+                        String::new(),
+                        Applicability::MachineApplicable,
+                    );
+                }
                 return true;
             }
             if_chain! {
                 if let Some(as_ref_trait_id) = cx.tcx.get_diagnostic_item(sym::AsRef);
                 if implements_trait(cx, receiver_ty, as_ref_trait_id, &[GenericArg::from(target_ty)]);
-                if let Some(receiver_snippet) = snippet_opt(cx, receiver.span);
                 then {
                     span_lint_and_sugg(
                         cx,
@@ -326,3 +332,21 @@ fn implements_deref_trait(cx: &LateContext<'tcx>, ty: Ty<'tcx>, deref_target_ty:
         }
     }
 }
+
+/// Returns true if the named method can be used to convert the receiver to its "owned"
+/// representation.
+fn is_to_owned_like(cx: &LateContext<'_>, method_name: Symbol, method_def_id: DefId) -> bool {
+    is_clone_like(cx, &*method_name.as_str(), method_def_id)
+        || is_cow_into_owned(cx, method_name, method_def_id)
+        || is_to_string(cx, method_name, method_def_id)
+}
+
+/// Returns true if the named method is `Cow::into_owned`.
+fn is_cow_into_owned(cx: &LateContext<'_>, method_name: Symbol, method_def_id: DefId) -> bool {
+    method_name.as_str() == "into_owned" && is_diag_item_method(cx, method_def_id, sym::Cow)
+}
+
+/// Returns true if the named method is `ToString::to_string`.
+fn is_to_string(cx: &LateContext<'_>, method_name: Symbol, method_def_id: DefId) -> bool {
+    method_name.as_str() == "to_string" && is_diag_trait_item(cx, method_def_id, sym::ToString)
+}
diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs
index 634a452d6f1..6171823abbb 100644
--- a/clippy_utils/src/paths.rs
+++ b/clippy_utils/src/paths.rs
@@ -36,7 +36,6 @@ pub const CLONE_TRAIT_METHOD: [&str; 4] = ["core", "clone", "Clone", "clone"];
 pub const CMP_MAX: [&str; 3] = ["core", "cmp", "max"];
 pub const CMP_MIN: [&str; 3] = ["core", "cmp", "min"];
 pub const COW: [&str; 3] = ["alloc", "borrow", "Cow"];
-pub const COW_INTO_OWNED: [&str; 4] = ["alloc", "borrow", "Cow", "into_owned"];
 pub const CSTRING_AS_C_STR: [&str; 5] = ["std", "ffi", "c_str", "CString", "as_c_str"];
 pub const DEFAULT_TRAIT_METHOD: [&str; 4] = ["core", "default", "Default", "default"];
 pub const DEREF_MUT_TRAIT_METHOD: [&str; 5] = ["core", "ops", "deref", "DerefMut", "deref_mut"];
@@ -171,7 +170,6 @@ pub const SLICE_FROM_RAW_PARTS: [&str; 4] = ["core", "slice", "raw", "from_raw_p
 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_ITER: [&str; 4] = ["core", "slice", "iter", "Iter"];
-pub const SLICE_TO_VEC: [&str; 4] = ["alloc", "slice", "<impl [T]>", "to_vec"];
 pub const STDERR: [&str; 4] = ["std", "io", "stdio", "stderr"];
 pub const STDOUT: [&str; 4] = ["std", "io", "stdio", "stdout"];
 pub const CONVERT_IDENTITY: [&str; 3] = ["core", "convert", "identity"];
diff --git a/tests/ui/unnecessary_to_owned.fixed b/tests/ui/unnecessary_to_owned.fixed
index f1a5dc91a7e..bd9ef1fff42 100644
--- a/tests/ui/unnecessary_to_owned.fixed
+++ b/tests/ui/unnecessary_to_owned.fixed
@@ -1,15 +1,14 @@
 // run-rustfix
 
 #![allow(clippy::ptr_arg)]
-// Some of the expressions that `redundant_clone` flags overlap with ours. Enabling it interferes
-// with `rustfix`.
-#![allow(clippy::redundant_clone)]
-// `needless_borrow` is for checking the fixed code.
+// `needless_borrow` is to ensure there are no needles borrows in the fixed code.
 #![warn(clippy::needless_borrow)]
+// `redundant_clone` is to ensure there is no overlap between that lint and this one.
+#![warn(clippy::redundant_clone)]
 #![warn(clippy::unnecessary_to_owned)]
 
 use std::borrow::Cow;
-use std::ffi::{CStr, OsStr};
+use std::ffi::{CStr, CString, OsStr, OsString};
 use std::ops::Deref;
 
 #[derive(Clone)]
@@ -51,6 +50,7 @@ fn main() {
     let array_ref = &["x"];
     let slice = &["x"][..];
     let x = X(String::from("x"));
+    let x_ref = &x;
 
     require_c_str(&Cow::from(c_str));
     require_c_str(c_str);
@@ -66,17 +66,17 @@ fn main() {
     require_str(s);
     require_str(&Cow::from(s));
     require_str(s);
-    require_str(x.as_ref());
+    require_str(x_ref.as_ref());
 
     require_slice(slice);
     require_slice(&Cow::from(slice));
     require_slice(array.as_ref());
     require_slice(array_ref.as_ref());
     require_slice(slice);
-    require_slice(&x);
+    require_slice(x_ref);
 
     require_x(&Cow::<X>::Owned(x.clone()));
-    require_x(&x);
+    require_x(x_ref);
 
     require_deref_c_str(c_str);
     require_deref_os_str(os_str);
@@ -118,16 +118,23 @@ fn main() {
     require_as_ref_slice_str(array_ref, s);
     require_as_ref_slice_str(slice, s);
 
-    let _ = x.join(&x);
+    let _ = x.join(x_ref);
 
     // negative tests
     require_string(&s.to_string());
     require_string(&Cow::from(s).into_owned());
     require_string(&s.to_owned());
-    require_string(&x.to_string());
+    require_string(&x_ref.to_string());
 
     // `X` isn't copy.
+    require_slice(&x.to_owned());
     require_deref_slice(x.to_owned());
+
+    // The following should be flagged by `redundant_clone`, but not by this lint.
+    require_c_str(&CString::from_vec_with_nul(vec![0]).unwrap());
+    require_os_str(&OsString::from("x"));
+    require_path(&std::path::PathBuf::from("x"));
+    require_str(&String::from("x"));
 }
 
 fn require_c_str(_: &CStr) {}
diff --git a/tests/ui/unnecessary_to_owned.rs b/tests/ui/unnecessary_to_owned.rs
index 081ff635bbd..1bf30e22893 100644
--- a/tests/ui/unnecessary_to_owned.rs
+++ b/tests/ui/unnecessary_to_owned.rs
@@ -1,15 +1,14 @@
 // run-rustfix
 
 #![allow(clippy::ptr_arg)]
-// Some of the expressions that `redundant_clone` flags overlap with ours. Enabling it interferes
-// with `rustfix`.
-#![allow(clippy::redundant_clone)]
-// `needless_borrow` is for checking the fixed code.
+// `needless_borrow` is to ensure there are no needles borrows in the fixed code.
 #![warn(clippy::needless_borrow)]
+// `redundant_clone` is to ensure there is no overlap between that lint and this one.
+#![warn(clippy::redundant_clone)]
 #![warn(clippy::unnecessary_to_owned)]
 
 use std::borrow::Cow;
-use std::ffi::{CStr, OsStr};
+use std::ffi::{CStr, CString, OsStr, OsString};
 use std::ops::Deref;
 
 #[derive(Clone)]
@@ -51,6 +50,7 @@ fn main() {
     let array_ref = &["x"];
     let slice = &["x"][..];
     let x = X(String::from("x"));
+    let x_ref = &x;
 
     require_c_str(&Cow::from(c_str).into_owned());
     require_c_str(&c_str.to_owned());
@@ -66,17 +66,17 @@ fn main() {
     require_str(&s.to_string());
     require_str(&Cow::from(s).into_owned());
     require_str(&s.to_owned());
-    require_str(&x.to_string());
+    require_str(&x_ref.to_string());
 
     require_slice(&slice.to_vec());
     require_slice(&Cow::from(slice).into_owned());
     require_slice(&array.to_owned());
     require_slice(&array_ref.to_owned());
     require_slice(&slice.to_owned());
-    require_slice(&x.to_owned());
+    require_slice(&x_ref.to_owned());
 
     require_x(&Cow::<X>::Owned(x.clone()).into_owned());
-    require_x(&x.to_owned());
+    require_x(&x_ref.to_owned());
 
     require_deref_c_str(c_str.to_owned());
     require_deref_os_str(os_str.to_owned());
@@ -118,16 +118,23 @@ fn main() {
     require_as_ref_slice_str(array_ref.to_owned(), s.to_owned());
     require_as_ref_slice_str(slice.to_owned(), s.to_owned());
 
-    let _ = x.join(&x.to_string());
+    let _ = x.join(&x_ref.to_string());
 
     // negative tests
     require_string(&s.to_string());
     require_string(&Cow::from(s).into_owned());
     require_string(&s.to_owned());
-    require_string(&x.to_string());
+    require_string(&x_ref.to_string());
 
     // `X` isn't copy.
+    require_slice(&x.to_owned());
     require_deref_slice(x.to_owned());
+
+    // The following should be flagged by `redundant_clone`, but not by this lint.
+    require_c_str(&CString::from_vec_with_nul(vec![0]).unwrap().to_owned());
+    require_os_str(&OsString::from("x").to_os_string());
+    require_path(&std::path::PathBuf::from("x").to_path_buf());
+    require_str(&String::from("x").to_string());
 }
 
 fn require_c_str(_: &CStr) {}
diff --git a/tests/ui/unnecessary_to_owned.stderr b/tests/ui/unnecessary_to_owned.stderr
index b15dd4c7ee7..83cc53ef3a2 100644
--- a/tests/ui/unnecessary_to_owned.stderr
+++ b/tests/ui/unnecessary_to_owned.stderr
@@ -1,3 +1,52 @@
+error: redundant clone
+  --> $DIR/unnecessary_to_owned.rs:134:64
+   |
+LL |     require_c_str(&CString::from_vec_with_nul(vec![0]).unwrap().to_owned());
+   |                                                                ^^^^^^^^^^^ help: remove this
+   |
+   = note: `-D clippy::redundant-clone` implied by `-D warnings`
+note: this value is dropped without further use
+  --> $DIR/unnecessary_to_owned.rs:134:20
+   |
+LL |     require_c_str(&CString::from_vec_with_nul(vec![0]).unwrap().to_owned());
+   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: redundant clone
+  --> $DIR/unnecessary_to_owned.rs:135:40
+   |
+LL |     require_os_str(&OsString::from("x").to_os_string());
+   |                                        ^^^^^^^^^^^^^^^ help: remove this
+   |
+note: this value is dropped without further use
+  --> $DIR/unnecessary_to_owned.rs:135:21
+   |
+LL |     require_os_str(&OsString::from("x").to_os_string());
+   |                     ^^^^^^^^^^^^^^^^^^^
+
+error: redundant clone
+  --> $DIR/unnecessary_to_owned.rs:136:48
+   |
+LL |     require_path(&std::path::PathBuf::from("x").to_path_buf());
+   |                                                ^^^^^^^^^^^^^^ help: remove this
+   |
+note: this value is dropped without further use
+  --> $DIR/unnecessary_to_owned.rs:136:19
+   |
+LL |     require_path(&std::path::PathBuf::from("x").to_path_buf());
+   |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: redundant clone
+  --> $DIR/unnecessary_to_owned.rs:137:35
+   |
+LL |     require_str(&String::from("x").to_string());
+   |                                   ^^^^^^^^^^^^ help: remove this
+   |
+note: this value is dropped without further use
+  --> $DIR/unnecessary_to_owned.rs:137:18
+   |
+LL |     require_str(&String::from("x").to_string());
+   |                  ^^^^^^^^^^^^^^^^^
+
 error: unnecessary use of `into_owned`
   --> $DIR/unnecessary_to_owned.rs:55:36
    |
@@ -69,8 +118,8 @@ LL |     require_str(&s.to_owned());
 error: unnecessary use of `to_string`
   --> $DIR/unnecessary_to_owned.rs:69:17
    |
-LL |     require_str(&x.to_string());
-   |                 ^^^^^^^^^^^^^^ help: use: `x.as_ref()`
+LL |     require_str(&x_ref.to_string());
+   |                 ^^^^^^^^^^^^^^^^^^ help: use: `x_ref.as_ref()`
 
 error: unnecessary use of `to_vec`
   --> $DIR/unnecessary_to_owned.rs:71:19
@@ -103,10 +152,10 @@ LL |     require_slice(&slice.to_owned());
    |                   ^^^^^^^^^^^^^^^^^ help: use: `slice`
 
 error: unnecessary use of `to_owned`
-  --> $DIR/unnecessary_to_owned.rs:76:21
+  --> $DIR/unnecessary_to_owned.rs:76:19
    |
-LL |     require_slice(&x.to_owned());
-   |                     ^^^^^^^^^^^ help: remove this
+LL |     require_slice(&x_ref.to_owned());
+   |                   ^^^^^^^^^^^^^^^^^ help: use: `x_ref`
 
 error: unnecessary use of `into_owned`
   --> $DIR/unnecessary_to_owned.rs:78:42
@@ -117,8 +166,8 @@ LL |     require_x(&Cow::<X>::Owned(x.clone()).into_owned());
 error: unnecessary use of `to_owned`
   --> $DIR/unnecessary_to_owned.rs:79:15
    |
-LL |     require_x(&x.to_owned());
-   |               ^^^^^^^^^^^^^ help: use: `&x`
+LL |     require_x(&x_ref.to_owned());
+   |               ^^^^^^^^^^^^^^^^^ help: use: `x_ref`
 
 error: unnecessary use of `to_owned`
   --> $DIR/unnecessary_to_owned.rs:81:25
@@ -375,8 +424,8 @@ LL |     require_as_ref_slice_str(slice.to_owned(), s.to_owned());
 error: unnecessary use of `to_string`
   --> $DIR/unnecessary_to_owned.rs:121:20
    |
-LL |     let _ = x.join(&x.to_string());
-   |                    ^^^^^^^^^^^^^^ help: use: `&x`
+LL |     let _ = x.join(&x_ref.to_string());
+   |                    ^^^^^^^^^^^^^^^^^^ help: use: `x_ref`
 
-error: aborting due to 63 previous errors
+error: aborting due to 67 previous errors