Auto merge of #8794 - smoelius:fix-8759, r=llogiq

Address `unnecessary_to_owned` false positive

My proposed fix for #8759 is to revise the conditions that delineate `redundant_clone` and `unnecessary_to_owned`:
```rust
        // Only flag cases satisfying at least one of the following three conditions:
        // * the referent and receiver types are distinct
        // * the referent/receiver type is a copyable array
        // * the method is `Cow::into_owned`
        // This restriction is to ensure there is no overlap between `redundant_clone` and this
        // lint. It also avoids the following false positive:
        //  #8759
        //   Arrays are a bit of a corner case. Non-copyable arrays are handled by
        // `redundant_clone`, but copyable arrays are not.
```
This change causes a few cases that were previously flagged by `unnecessary_to_owned` to no longer be flagged. But one could argue those cases would be better handled by `redundant_clone`.

Closes #8759

changelog: none

clippy_lints/src/methods/unnecessary_to_owned.rs

@@ -65,13 +65,12 @@ fn check_addr_of_expr(
         if let Some(parent) = get_parent_expr(cx, expr);
         if let ExprKind::AddrOf(BorrowKind::Ref, Mutability::Not, _) = parent.kind;
         let adjustments = cx.typeck_results().expr_adjustments(parent).iter().collect::<Vec<_>>();
-        if let Some(target_ty) = match adjustments[..]
-        {
+        if let
             // For matching uses of `Cow::from`
             [
                 Adjustment {
                     kind: Adjust::Deref(None),
-                    ..
+                    target: referent_ty,
                 },
                 Adjustment {
                     kind: Adjust::Borrow(_),
@@ -82,7 +81,7 @@ fn check_addr_of_expr(
             | [
                 Adjustment {
                     kind: Adjust::Deref(None),
-                    ..
+                    target: referent_ty,
                 },
                 Adjustment {
                     kind: Adjust::Borrow(_),
@@ -97,7 +96,7 @@ fn check_addr_of_expr(
             | [
                 Adjustment {
                     kind: Adjust::Deref(None),
-                    ..
+                    target: referent_ty,
                 },
                 Adjustment {
                     kind: Adjust::Deref(Some(OverloadedDeref { .. })),
@@ -107,17 +106,24 @@ fn check_addr_of_expr(
                     kind: Adjust::Borrow(_),
                     target: target_ty,
                 },
-            ] => Some(target_ty),
-            _ => None,
-        };
+            ] = adjustments[..];
         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);
+        let (target_ty, n_target_refs) = peel_mid_ty_refs(*target_ty);
+        let (receiver_ty, n_receiver_refs) = peel_mid_ty_refs(receiver_ty);
+        // Only flag cases satisfying at least one of the following three conditions:
+        // * the referent and receiver types are distinct
+        // * the referent/receiver type is a copyable array
+        // * the method is `Cow::into_owned`
+        // This restriction is to ensure there is no overlap between `redundant_clone` and this
+        // lint. It also avoids the following false positive:
+        //  https://github.com/rust-lang/rust-clippy/issues/8759
+        //   Arrays are a bit of a corner case. Non-copyable arrays are handled by
+        // `redundant_clone`, but copyable arrays are not.
+        if *referent_ty != receiver_ty
+            || (matches!(referent_ty.kind(), ty::Array(..)) && is_copy(cx, *referent_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, n_receiver_refs) = peel_mid_ty_refs(receiver_ty);
             if receiver_ty == target_ty && n_target_refs >= n_receiver_refs {
                 span_lint_and_sugg(
                     cx,
@@ -207,7 +213,11 @@ fn check_into_iter_call_arg(
             if unnecessary_iter_cloned::check_for_loop_iter(cx, parent, method_name, receiver, true) {
                 return true;
             }
-            let cloned_or_copied = if is_copy(cx, item_ty) && meets_msrv(msrv, &msrvs::ITERATOR_COPIED) { "copied" } else { "cloned" };
+            let cloned_or_copied = if is_copy(cx, item_ty) && meets_msrv(msrv, &msrvs::ITERATOR_COPIED) {
+                "copied"
+            } else {
+                "cloned"
+            };
             // The next suggestion may be incorrect because the removal of the `to_owned`-like
             // function could cause the iterator to hold a reference to a resource that is used
             // mutably. See https://github.com/rust-lang/rust-clippy/issues/8148.

tests/ui/recursive_format_impl.stderr

@@ -6,15 +6,6 @@ LL |         write!(f, "{}", self.to_string())
    |
    = note: `-D clippy::recursive-format-impl` implied by `-D warnings`
 
-error: unnecessary use of `to_string`
-  --> $DIR/recursive_format_impl.rs:61:50
-   |
-LL |             Self::E(string) => write!(f, "E {}", string.to_string()),
-   |                                                  ^^^^^^^^^^^^^^^^^^
-   |
-   = note: `-D clippy::unnecessary-to-owned` implied by `-D warnings`
-   = note: this error originates in the macro `$crate::format_args` (in Nightly builds, run with -Z macro-backtrace for more info)
-
 error: using `self` as `Display` in `impl Display` will cause infinite recursion
   --> $DIR/recursive_format_impl.rs:73:9
    |
@@ -87,5 +78,5 @@ LL |         write!(f, "{}", &&**&&*self)
    |
    = note: this error originates in the macro `write` (in Nightly builds, run with -Z macro-backtrace for more info)
 
-error: aborting due to 11 previous errors
+error: aborting due to 10 previous errors
 

tests/ui/unnecessary_to_owned.fixed

@@ -78,10 +78,10 @@ fn main() {
     require_slice(array.as_ref());
     require_slice(array_ref.as_ref());
     require_slice(slice);
-    require_slice(x_ref);
+    require_slice(&x_ref.to_owned()); // No longer flagged because of #8759.
 
     require_x(&Cow::<X>::Owned(x.clone()));
-    require_x(x_ref);
+    require_x(&x_ref.to_owned()); // No longer flagged because of #8759.
 
     require_deref_c_str(c_str);
     require_deref_os_str(os_str);
@@ -152,6 +152,7 @@ fn main() {
     require_os_str(&OsString::from("x"));
     require_path(&std::path::PathBuf::from("x"));
     require_str(&String::from("x"));
+    require_slice(&[String::from("x")]);
 }
 
 fn require_c_str(_: &CStr) {}
@@ -272,3 +273,59 @@ mod issue_8507 {
         Box::new(build(y))
     }
 }
+
+// https://github.com/rust-lang/rust-clippy/issues/8759
+mod issue_8759 {
+    #![allow(dead_code)]
+
+    #[derive(Default)]
+    struct View {}
+
+    impl std::borrow::ToOwned for View {
+        type Owned = View;
+        fn to_owned(&self) -> Self::Owned {
+            View {}
+        }
+    }
+
+    #[derive(Default)]
+    struct RenderWindow {
+        default_view: View,
+    }
+
+    impl RenderWindow {
+        fn default_view(&self) -> &View {
+            &self.default_view
+        }
+        fn set_view(&mut self, _view: &View) {}
+    }
+
+    fn main() {
+        let mut rw = RenderWindow::default();
+        rw.set_view(&rw.default_view().to_owned());
+    }
+}
+
+mod issue_8759_variant {
+    #![allow(dead_code)]
+
+    #[derive(Clone, Default)]
+    struct View {}
+
+    #[derive(Default)]
+    struct RenderWindow {
+        default_view: View,
+    }
+
+    impl RenderWindow {
+        fn default_view(&self) -> &View {
+            &self.default_view
+        }
+        fn set_view(&mut self, _view: &View) {}
+    }
+
+    fn main() {
+        let mut rw = RenderWindow::default();
+        rw.set_view(&rw.default_view().to_owned());
+    }
+}

tests/ui/unnecessary_to_owned.rs

@@ -78,10 +78,10 @@ fn main() {
     require_slice(&array.to_owned());
     require_slice(&array_ref.to_owned());
     require_slice(&slice.to_owned());
-    require_slice(&x_ref.to_owned());
+    require_slice(&x_ref.to_owned()); // No longer flagged because of #8759.
 
     require_x(&Cow::<X>::Owned(x.clone()).into_owned());
-    require_x(&x_ref.to_owned());
+    require_x(&x_ref.to_owned()); // No longer flagged because of #8759.
 
     require_deref_c_str(c_str.to_owned());
     require_deref_os_str(os_str.to_owned());
@@ -152,6 +152,7 @@ fn main() {
     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());
+    require_slice(&[String::from("x")].to_owned());
 }
 
 fn require_c_str(_: &CStr) {}
@@ -272,3 +273,59 @@ mod issue_8507 {
         Box::new(build(y.to_string()))
     }
 }
+
+// https://github.com/rust-lang/rust-clippy/issues/8759
+mod issue_8759 {
+    #![allow(dead_code)]
+
+    #[derive(Default)]
+    struct View {}
+
+    impl std::borrow::ToOwned for View {
+        type Owned = View;
+        fn to_owned(&self) -> Self::Owned {
+            View {}
+        }
+    }
+
+    #[derive(Default)]
+    struct RenderWindow {
+        default_view: View,
+    }
+
+    impl RenderWindow {
+        fn default_view(&self) -> &View {
+            &self.default_view
+        }
+        fn set_view(&mut self, _view: &View) {}
+    }
+
+    fn main() {
+        let mut rw = RenderWindow::default();
+        rw.set_view(&rw.default_view().to_owned());
+    }
+}
+
+mod issue_8759_variant {
+    #![allow(dead_code)]
+
+    #[derive(Clone, Default)]
+    struct View {}
+
+    #[derive(Default)]
+    struct RenderWindow {
+        default_view: View,
+    }
+
+    impl RenderWindow {
+        fn default_view(&self) -> &View {
+            &self.default_view
+        }
+        fn set_view(&mut self, _view: &View) {}
+    }
+
+    fn main() {
+        let mut rw = RenderWindow::default();
+        rw.set_view(&rw.default_view().to_owned());
+    }
+}

tests/ui/unnecessary_to_owned.stderr

@@ -47,6 +47,18 @@ note: this value is dropped without further use
 LL |     require_str(&String::from("x").to_string());
    |                  ^^^^^^^^^^^^^^^^^
 
+error: redundant clone
+  --> $DIR/unnecessary_to_owned.rs:155:39
+   |
+LL |     require_slice(&[String::from("x")].to_owned());
+   |                                       ^^^^^^^^^^^ help: remove this
+   |
+note: this value is dropped without further use
+  --> $DIR/unnecessary_to_owned.rs:155:20
+   |
+LL |     require_slice(&[String::from("x")].to_owned());
+   |                    ^^^^^^^^^^^^^^^^^^^
+
 error: unnecessary use of `into_owned`
   --> $DIR/unnecessary_to_owned.rs:60:36
    |
@@ -151,24 +163,12 @@ error: unnecessary use of `to_owned`
 LL |     require_slice(&slice.to_owned());
    |                   ^^^^^^^^^^^^^^^^^ help: use: `slice`
 
-error: unnecessary use of `to_owned`
-  --> $DIR/unnecessary_to_owned.rs:81:19
-   |
-LL |     require_slice(&x_ref.to_owned());
-   |                   ^^^^^^^^^^^^^^^^^ help: use: `x_ref`
-
 error: unnecessary use of `into_owned`
   --> $DIR/unnecessary_to_owned.rs:83:42
    |
 LL |     require_x(&Cow::<X>::Owned(x.clone()).into_owned());
    |                                          ^^^^^^^^^^^^^ help: remove this
 
-error: unnecessary use of `to_owned`
-  --> $DIR/unnecessary_to_owned.rs:84:15
-   |
-LL |     require_x(&x_ref.to_owned());
-   |               ^^^^^^^^^^^^^^^^^ help: use: `x_ref`
-
 error: unnecessary use of `to_owned`
   --> $DIR/unnecessary_to_owned.rs:86:25
    |
@@ -476,7 +476,7 @@ LL |     let _ = IntoIterator::into_iter([std::path::PathBuf::new()][..].to_owne
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `[std::path::PathBuf::new()][..].iter().cloned()`
 
 error: unnecessary use of `to_vec`
-  --> $DIR/unnecessary_to_owned.rs:197:14
+  --> $DIR/unnecessary_to_owned.rs:198:14
    |
 LL |     for t in file_types.to_vec() {
    |              ^^^^^^^^^^^^^^^^^^^
@@ -492,22 +492,22 @@ LL +         let path = match get_file_path(t) {
    | 
 
 error: unnecessary use of `to_vec`
-  --> $DIR/unnecessary_to_owned.rs:220:14
+  --> $DIR/unnecessary_to_owned.rs:221:14
    |
 LL |     let _ = &["x"][..].to_vec().into_iter();
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `["x"][..].iter().cloned()`
 
 error: unnecessary use of `to_vec`
-  --> $DIR/unnecessary_to_owned.rs:225:14
+  --> $DIR/unnecessary_to_owned.rs:226:14
    |
 LL |     let _ = &["x"][..].to_vec().into_iter();
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `["x"][..].iter().copied()`
 
 error: unnecessary use of `to_string`
-  --> $DIR/unnecessary_to_owned.rs:272:24
+  --> $DIR/unnecessary_to_owned.rs:273:24
    |
 LL |         Box::new(build(y.to_string()))
    |                        ^^^^^^^^^^^^^ help: use: `y`
 
-error: aborting due to 79 previous errors
+error: aborting due to 78 previous errors